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

[Zivkovic, Milos]

Hello,

Thanks for taking the time to review the patch.

Regarding the VideoFullRange flag, it's actually set conditionally in
the updated patch (not appearing on patchwork though).
As for the ColourDescriptionPresent flag, I'll update the patch so
it's set only if color_primaries, color_trc and colorspace are not
unspecified.
Additionally, I'll add it as mfxExtBuffer to q->extparam_internal only
if ColourDescriptionPresent is set.

Regards,
Miloš

On Wed, Oct 28, 2020 at 9:02 PM Mark Thompson <sw at jkqxz.net> wrote:
>
> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".