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

[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.

- Andreas