[FFmpeg-devel] [PATCH 2/2] lavc/qsvenc_av1: accept HDR metadata if have

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Andreas Rheinhardt:
>> Xiang, Haihao:
>>> From: Haihao Xiang <haihao.xiang at intel.com>
>>>
>>> The sdk av1 encoder can accept HDR metadata via mfxEncodeCtrl::ExtParam.
>>>
>>> Signed-off-by: Haihao Xiang <haihao.xiang at intel.com>
>>> ---
>>>  libavcodec/qsvenc_av1.c | 75 +++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 75 insertions(+)
>>>
>>> diff --git a/libavcodec/qsvenc_av1.c b/libavcodec/qsvenc_av1.c
>>> index 33727bb07e..a7d35d7bef 100644
>>> --- a/libavcodec/qsvenc_av1.c
>>> +++ b/libavcodec/qsvenc_av1.c
>>> @@ -26,6 +26,8 @@
>>>  
>>>  #include "libavutil/common.h"
>>>  #include "libavutil/opt.h"
>>> +#include "libavutil/mem.h"
>>> +#include "libavutil/mastering_display_metadata.h"
>>
>> Breaks alphabetical ordering
>>
>>>  
>>>  #include "avcodec.h"
>>>  #include "codec_internal.h"
>>> @@ -39,6 +41,77 @@ typedef struct QSVAV1EncContext {
>>>      QSVEncContext qsv;
>>>  } QSVAV1EncContext;
>>>  
>>> +static int qsv_av1_set_encode_ctrl(AVCodecContext *avctx,
>>> +                                   const AVFrame *frame, mfxEncodeCtrl *enc_ctrl)
>>> +{
>>> +    QSVAV1EncContext *q = avctx->priv_data;
>>> +    AVFrameSideData *sd;
>>> +
>>> +    if (!frame || !QSV_RUNTIME_VERSION_ATLEAST(q->qsv.ver, 2, 11))
>>> +        return 0;
>>> +
>>> +    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
>>> +    if (sd) {
>>> +        AVMasteringDisplayMetadata *mdm = (AVMasteringDisplayMetadata *)sd->data;
>>> +
>>> +        if (mdm->has_primaries && mdm->has_luminance) {
>>> +            const int chroma_den   = 1 << 16;
>>> +            const int max_luma_den = 1 << 8;
>>> +            const int min_luma_den = 1 << 14;
>>> +            int i;
>>
>> Should have loop-scope
>>
>>> +            mfxExtMasteringDisplayColourVolume *mdcv = av_mallocz(sizeof(mfxExtMasteringDisplayColourVolume));
>>> +            if (!mdcv)
>>> +                return AVERROR(ENOMEM);
>>> +
>>> +            mdcv->Header.BufferId = MFX_EXTBUFF_MASTERING_DISPLAY_COLOUR_VOLUME;
>>> +            mdcv->Header.BufferSz = sizeof(*mdcv);
>>> +
>>> +            for (i = 0; i < 3; i++) {
>>> +                mdcv->DisplayPrimariesX[i] =
>>> +                    av_rescale(mdm->display_primaries[i][0].num, chroma_den,
>>> +                               mdm->display_primaries[i][0].den);
>>> +                mdcv->DisplayPrimariesY[i] =
>>> +                    av_rescale(mdm->display_primaries[i][1].num, chroma_den,
>>> +                               mdm->display_primaries[i][1].den);
>>> +            }
>>> +
>>> +            mdcv->WhitePointX =
>>> +                av_rescale(mdm->white_point[0].num, chroma_den,
>>> +                           mdm->white_point[0].den);
>>> +            mdcv->WhitePointY =
>>> +                av_rescale(mdm->white_point[1].num, chroma_den,
>>> +                           mdm->white_point[1].den);
>>> +
>>> +            mdcv->MaxDisplayMasteringLuminance =
>>> +                av_rescale(mdm->max_luminance.num, max_luma_den,
>>> +                           mdm->max_luminance.den);
>>> +            mdcv->MinDisplayMasteringLuminance =
>>> +                av_rescale(mdm->min_luminance.num, min_luma_den,
>>> +                           mdm->min_luminance.den);
>>> +
>>> +            enc_ctrl->ExtParam[enc_ctrl->NumExtParam++] = (mfxExtBuffer *)mdcv;
>>> +        }
>>> +    }
>>> +
>>> +    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
>>> +    if (sd) {
>>> +        AVContentLightMetadata *clm = (AVContentLightMetadata *)sd->data;
>>> +        mfxExtContentLightLevelInfo * clli = av_mallocz(sizeof(mfxExtContentLightLevelInfo));
>>
>> sizeof(*clli)
>>
>> But actually, both allocations seem to be unnecessary: This callback is
>> only called when there is a QSVFrame and it already contains a
>> mfxExtContentLightLevelInfo and mfxExtMasteringDisplayColourVolume (used
>> by the decoder), so why not simply reuse that? The type of
>> set_encode_ctrl_cb would need to be changed of course.
>>
> 
> I know see that this would necessitate more changes because the ExtParam
> pointers are currently ownership pointers. So ignore this suggestion.
> 

s/know/now/

- Andreas