Re: [FFmpeg-devel] [PATCH 3/3] libavutil/hwcontext_opencl: fix a bug for mapping qsv frame to opencl

[Mark Thompson <sw@jkqxz.net>]

On 28/12/2021 01:17, Soft Works wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
>> Thompson
>> Sent: Tuesday, December 28, 2021 12:46 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 3/3] libavutil/hwcontext_opencl: fix a bug
>> for mapping qsv frame to opencl
>>
>> On 27/12/2021 20:31, Soft Works wrote:>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Mark
>>>> Thompson
>>>> Sent: Monday, December 27, 2021 7:51 PM
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Subject: Re: [FFmpeg-devel] [PATCH 3/3] libavutil/hwcontext_opencl: fix a
>> bug
>>>> for mapping qsv frame to opencl
>>>>
>>>> On 16/11/2021 08:16, Wenbin Chen wrote:
>>>>> From: nyanmisaka <nst799610810@gmail.com>
>>>>>
>>>>> mfxHDLPair was added to qsv, so modify qsv->opencl map function as well.
>>>>> Now the following commandline works:
>>>>>
>>>>> ffmpeg -v verbose -init_hw_device vaapi=va:/dev/dri/renderD128 \
>>>>> -init_hw_device qsv=qs@va -init_hw_device opencl=ocl@va -filter_hw_device
>>>> ocl \
>>>>> -hwaccel qsv -hwaccel_output_format qsv -hwaccel_device qs -c:v h264_qsv
>> \
>>>>> -i input.264 -vf
>> "hwmap=derive_device=opencl,format=opencl,avgblur_opencl,
>>>> \
>>>>> hwmap=derive_device=qsv:reverse=1:extra_hw_frames=32,format=qsv" \
>>>>> -c:v h264_qsv output.264
>>>>>
>>>>> Signed-off-by: nyanmisaka <nst799610810@gmail.com>
>>>>> Signed-off-by: Wenbin Chen <wenbin.chen@intel.com>
>>>>> ---
>>>>>     libavutil/hwcontext_opencl.c | 3 ++-
>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavutil/hwcontext_opencl.c b/libavutil/hwcontext_opencl.c
>>>>> index 26a3a24593..4b6e74ff6f 100644
>>>>> --- a/libavutil/hwcontext_opencl.c
>>>>> +++ b/libavutil/hwcontext_opencl.c
>>>>> @@ -2249,7 +2249,8 @@ static int opencl_map_from_qsv(AVHWFramesContext
>>>> *dst_fc, AVFrame *dst,
>>>>>     #if CONFIG_LIBMFX
>>>>>         if (src->format == AV_PIX_FMT_QSV) {
>>>>>             mfxFrameSurface1 *mfx_surface = (mfxFrameSurface1*)src-
>>> data[3];
>>>>> -        va_surface = *(VASurfaceID*)mfx_surface->Data.MemId;
>>>>> +        mfxHDLPair *pair = (mfxHDLPair*)mfx_surface->Data.MemId;
>>>>> +        va_surface = *(VASurfaceID*)pair->first;
>>>>>         } else
>>>>>     #endif
>>>>>             if (src->format == AV_PIX_FMT_VAAPI) {
>>>>
>>>> Since these frames can be user-supplied, this implies that the user-facing
>>>> API/ABI for AV_PIX_FMT_QSV has changed.
>>>>
>>>> It looks like this was broken by using HDLPairs when D3D11 was introduced,
>>>> which silently changed the existing API for DXVA2 and VAAPI as well.
>>>>
>>>> Could someone related to that please document it properly (clearly not all
>>>> possible valid mfxFrameSurface1s are allowed), and note in APIchanges when
>>>> the API change happened?
>>>
>>> Hi Mark,
>>>
>>> QSV contexts always need to be backed by a child context, which can be
>> DXVA2,
>>> D3D11VA or VAAPI. You can create a QSV context either by deriving from one
>> of
>>> those contexts or when create a new QSV context, it automatically creates
>> an
>>> appropriate child context - either implicitly (auto mode) or explicitly,
>> like
>>> the ffmpeg implementation does in most cases.
>>
>> ... or by using the one the user supplies when they create it.
>>
>>> When working with "user-supplied" frames on Linux, you need to create a
>> VAAPI
>>> context with those frames and derive a QSV context from that context.
>>>
>>> There is no way to create or supply QSV frames directly.
>>
>> ???  The ability for the user to set up their own version of these things is
>> literally the whole point of the split alloc/init API.
>>
>>
>> // Some user stuff involving libmfx - has a D3D or VAAPI backing, but this
>> code doesn't need to care about it.
>>
>> // It has a session and creates some surfaces to use with MemId filled
>> compatible with ffmpeg.
>> user_session = ...;
>> user_surfaces = ...;
>>
>> // No ffmpeg involved before this, now we want to pass these surfaces we've
>> got into ffmpeg.
>>
>> // Create a device context using the existing session.
>>
>> mfx_ctx = av_hwdevice_ctx_alloc(MFX);
>>
>> dc = mfx_ctx->data;
>> mfx_dc = dc->hwctx;
>> mfx_dc->session = user_session;
>>
>> av_hwdevice_ctx_init(mfx_ctx);
>>
>> // Create a frames context out of the surfaces we've got.
>>
>> mfx_frames = av_hwframes_ctx_alloc(mfx_ctx);
>>
>> fc = mfx_frames->data;
>> fc.pool = user_surfaces.allocator;
>> fc.width = user_surfaces.width;
>> // etc.
>>
>> mfx_fc = fc->hwctx;
>> mfx_fc.surfaces = user_surfaces.array;
>> mfx_fc.nb_surfaces = user_surfaces.count;
>> mfx_fc.frame_type = user_surfaces.memtype;
>>
>> av_hwframe_ctx_init(frames);
>>
>> // Do stuff with frames.
> 
> I wouldn't consider an mfxSession as an entity that could or should be
> shared between implementations. IMO, this is not a valid use case.
> A consumer of the mfx API needs to make certain choices regarding
> the usage of the API, one of which is the way how frames are allocated
> and managed.
> This is not something that is meant to be shared between implementations.
> Even inside ffmpeg, we don't use a single mfx session. We use separate
> sessions for decoding, encoding and filtering that are joined together
> via MFXJoinSession.
> When an (ffmpeg-)API consumer is creating its own MFX session and its
> own frame allocator implementation, it shouldn't be and allowed
> scenario to create an ffmpeg hw context using this session.

The user is also aware of the thread safety rules.  I was giving the example above entirely serially to avoid that, but if they were using libmfx elsewhere in parallel with the above then indeed they would need a bit more care (create another session, some sort of locking to ensure serialisation) when passing it to ffmpeg.

> This shouldn't be considered a public API of ffmpeg because it doesn't
> make sense to share an mfx session like this.

So, to clarify, your opinion is that none of hwcontext_qsv.h should be public API?  If it were actually removed (not visible in installed headers) then I agree that would fix the problem.

> Besides that, is there any ffmpeg documentation indicating that
> the memId field of mfxFrameSurface1 could be casted to VASurfaceId?

It is user-visible API, and it matched the behaviour of the libmfx examples for D3D9 (identical pointer to IDirect3DSurface9) and VAAPI (a compatible pointer to a structure with the VASurfaceID as the first member) so doing the obvious thing worked.

- Mark
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".