[FFmpeg-devel] [PATCH v11 07/14] avcodec/vaapi_encode: extract the init and close function to base layer

[Wu, Tong1]

>From: Lynne <dev at lynne.ee>
>Sent: Monday, May 27, 2024 10:04 AM
>To: Wu, Tong1 <tong1.wu at intel.com>; FFmpeg development discussions and
>patches <ffmpeg-devel at ffmpeg.org>
>Subject: Re: [FFmpeg-devel] [PATCH v11 07/14] avcodec/vaapi_encode: extract
>the init and close function to base layer
>
>On 27/05/2024 02:35, Wu, Tong1 wrote:
>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>Lynne
>>> via ffmpeg-devel
>>> Sent: Saturday, May 25, 2024 10:07 PM
>>> To: ffmpeg-devel at ffmpeg.org
>>> Cc: Lynne <dev at lynne.ee>
>>> Subject: Re: [FFmpeg-devel] [PATCH v11 07/14] avcodec/vaapi_encode:
>extract
>>> the init and close function to base layer
>>>
>>> On 25/05/2024 12:30, tong1.wu-at-intel.com at ffmpeg.org wrote:
>>>> From: Tong Wu <tong1.wu at intel.com>
>>>>
>>>> Related parameters such as device context, frame context are also moved
>>>> to base layer.
>>>>
>>>> Signed-off-by: Tong Wu <tong1.wu at intel.com>
>>>> ---
>>>>    libavcodec/hw_base_encode.c     | 49 ++++++++++++++++++
>>>>    libavcodec/hw_base_encode.h     | 17 +++++++
>>>>    libavcodec/vaapi_encode.c       | 90 +++++++++++----------------------
>>>>    libavcodec/vaapi_encode.h       | 10 ----
>>>>    libavcodec/vaapi_encode_av1.c   |  2 +-
>>>>    libavcodec/vaapi_encode_h264.c  |  2 +-
>>>>    libavcodec/vaapi_encode_h265.c  |  2 +-
>>>>    libavcodec/vaapi_encode_mjpeg.c |  6 ++-
>>>>    8 files changed, 102 insertions(+), 76 deletions(-)
>>>>
>>>> diff --git a/libavcodec/hw_base_encode.c b/libavcodec/hw_base_encode.c
>>>> index 16afaa37be..c4789380b6 100644
>>>> --- a/libavcodec/hw_base_encode.c
>>>> +++ b/libavcodec/hw_base_encode.c
>>>> @@ -592,3 +592,52 @@ end:
>>>>
>>>>        return 0;
>>>>    }
>>>> +
>>>> +int ff_hw_base_encode_init(AVCodecContext *avctx)
>>>> +{
>>>> +    FFHWBaseEncodeContext *ctx = avctx->priv_data;
>>>
>>> This is the issue I was talking about, this requires that
>>> FFHWBaseEncodeContext is always the main context.
>>>
>>> Could you change it so everything takes FFHWBaseEncodeContext as an
>>> argument, rather than AVCodecContext (apart from where the function
>>> absolutely must read some data from it)?
>>
>> I'm trying to understand it more.
>>
>> In ff_hw_base_encode_init we also have the following code:
>>
>>      if (!avctx->hw_frames_ctx) {
>>          av_log(avctx, AV_LOG_ERROR, "A hardware frames reference is "
>>                 "required to associate the encoding device.\n");
>>          return AVERROR(EINVAL);
>>      }
>>
>> In this scenario we still need avctx right? So maybe this is the "must read data
>from it" situation and we keep AVCodecContext as the main context?
>
>Yup. My point is that FFHWBaseEncodeContext doesn't become the primary
>context that must be used, but a separate component other users can use
>standalone.
>
>> Plus I have this av_log concern which is there's indeed some function that
>the only use of avctx is its av_log's context. Do you think I should instead use
>FFHWBaseEncodeContext as the main context and pass it to av_log for this
>situation while keeping AVCodecContext as the main context for other
>functions which actually read from AVCodecContext. That might lead to
>different av_log context in the same file.
>
>Just save a pointer to avctx on init in FFHWBaseEncodeContext. That's
>what most of our code does.
>
>> Do you think the callbacks in FFHWEncodePictureOperation should be
>changed too? Or only all the functions in hw_base_encode.c should be
>concerned. Thank you.
>
>No, the callbacks should stay as-is. Users need to retrieve their own
>context via avctx. That's also a reason why you should keep a pointer to
>avctx on init.
>

I've updated v12 and made this change with a separate(15th) patch. Please see if that is what you wanted. Thank you.

-Tong