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

[Lynne via ffmpeg-devel <ffmpeg-devel@ffmpeg.org>]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 3357 bytes --]

On 27/05/2024 02:35, Wu, Tong1 wrote:
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Lynne
>> via ffmpeg-devel
>> Sent: Saturday, May 25, 2024 10:07 PM
>> To: ffmpeg-devel@ffmpeg.org
>> Cc: Lynne <dev@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@ffmpeg.org wrote:
>>> From: Tong Wu <tong1.wu@intel.com>
>>>
>>> Related parameters such as device context, frame context are also moved
>>> to base layer.
>>>
>>> Signed-off-by: Tong Wu <tong1.wu@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.

Thanks

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 637 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".