[FFmpeg-devel] [PATCH] avcodec/qsvenc: use color description for h264 and hevc

[Mark Thompson]

On 10/10/2020 18:27, Zivkovic, Milos wrote:
> Hello,
> 
> I've attached a patch that forwards the color description from
> AVCodecContext to mfxExtVideoSignalInfo. It has been discussed on the
> #ffmpeg and #ffmpeg-devel IRC channel a couple of times in the last
> few days.
>  > From 724c8e64f9ca9356464f1596bba42fc533905cd8 Mon Sep 17 00:00:00 2001
 > From: Milos Zivkovic <zivkovic at teralogics.com>
 > Date: Sat, 10 Oct 2020 16:34:47 +0000
 > Subject: [PATCH] avcodec/qsvenc: use color description for h264 and hevc
 >
 > Signed-off-by: Milos Zivkovic <zivkovic at teralogics.com>
 > ---
 >  libavcodec/qsvenc.c | 16 ++++++++++++++++
 >  libavcodec/qsvenc.h |  7 ++++++-
 >  2 files changed, 22 insertions(+), 1 deletion(-)
 >
 > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
 > index 1ed8f5d..0f48756 100644
 > --- a/libavcodec/qsvenc.c
 > +++ b/libavcodec/qsvenc.c
 > @@ -783,6 +783,22 @@ FF_ENABLE_DEPRECATION_WARNINGS
 >  #endif
 >          q->extparam_internal[q->nb_extparam_internal++] = (mfxExtBuffer *)&q->extco3;
 >  #endif
 > +#if QSV_HAVE_VSI
 > +        if (avctx->codec_id == AV_CODEC_ID_H264 || avctx->codec_id == AV_CODEC_ID_HEVC)
 > +        {
 > +            q->extvsi.Header.BufferId = MFX_EXTBUFF_VIDEO_SIGNAL_INFO;
 > +            q->extvsi.Header.BufferSz = sizeof(q->extvsi);
 > +            q->extvsi.VideoFormat = 5; // unspecified
 > +            q->extvsi.VideoFullRange = 0;
 > +            q->extvsi.ColourPrimaries = avctx->color_primaries;
 > +            q->extvsi.TransferCharacteristics = avctx->color_trc;
 > +            q->extvsi.MatrixCoefficients = avctx->colorspace;
 > +            q->extvsi.ColourDescriptionPresent = 1;
 > +
 > +            q->extparam_internal[q->nb_extparam_internal++] = (mfxExtBuffer *)&q->extvsi;
 > +        }
 > +#endif

Setting this unconditionally doesn't seem right - you want video_signal_type_present_flag to be zero if you aren't actually filling anything here, so that an incorrect value here doesn't accidentally take precedence over something provided externally (in the container, say).

Forcing video_full_range_flag to always be zero seems like it has the same problem, too.

 > +
 >      }
 >
 >  #if QSV_HAVE_EXT_VP9_PARAM
 > diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h
 > index 4f579d1..664c36f 100644
 > --- a/libavcodec/qsvenc.h
 > +++ b/libavcodec/qsvenc.h
 > @@ -53,6 +53,8 @@
 >
 >  #define QSV_HAVE_GPB    QSV_VERSION_ATLEAST(1, 18)
 >
 > +#define QSV_HAVE_VSI    QSV_VERSION_ATLEAST(1, 3)
 > +
 >  #if defined(_WIN32) || defined(__CYGWIN__)
 >  #define QSV_HAVE_AVBR   QSV_VERSION_ATLEAST(1, 3)
 >  #define QSV_HAVE_ICQ    QSV_VERSION_ATLEAST(1, 8)
 > @@ -134,12 +136,15 @@ typedef struct QSVEncContext {
 >  #if QSV_HAVE_EXT_VP9_PARAM
 >      mfxExtVP9Param  extvp9param;
 >  #endif
 > +#if QSV_HAVE_VSI
 > +    mfxExtVideoSignalInfo   extvsi;
 > +#endif
 >
 >      mfxExtOpaqueSurfaceAlloc opaque_alloc;
 >      mfxFrameSurface1       **opaque_surfaces;
 >      AVBufferRef             *opaque_alloc_buf;
 >
 > -    mfxExtBuffer  *extparam_internal[2 + QSV_HAVE_CO2 + QSV_HAVE_CO3 + (QSV_HAVE_MF * 2)];
 > +    mfxExtBuffer  *extparam_internal[2 + QSV_HAVE_CO2 + QSV_HAVE_CO3 + (QSV_HAVE_MF * 2) + QSV_HAVE_VSI];
 >      int         nb_extparam_internal;
 >
 >      mfxExtBuffer **extparam;
 > --
 > 1.8.3.1
 >

So, probably ok if suitably conditioned so that it isn't always set.

- Mark