diff mbox series

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

Message ID CAO5RS13CP40rCLd3DBpKw8KJ3aFJt4dHCECu6H=asiVdU-aaCA@mail.gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/qsvenc: use color description for h264 and hevc
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make warning Make failed

Commit Message

Zivkovic, Milos Oct. 10, 2020, 5:27 p.m. UTC
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.
Subject: [PATCH] avcodec/qsvenc: use color description for h264 and hevc

Signed-off-by: Milos Zivkovic <zivkovic@teralogics.com>
---
 libavcodec/qsvenc.c | 16 ++++++++++++++++
 libavcodec/qsvenc.h |  7 ++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

Comments

Zivkovic, Milos Oct. 28, 2020, 1:40 p.m. UTC | #1
Hello,

I've attached an updated patch that properly sets the
mfxExtVideoSignalInfo::VideoFullRange flag.

On Sat, Oct 10, 2020 at 7:27 PM Zivkovic, Milos <zivkovic@teralogics.com> 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.
Mark Thompson Oct. 28, 2020, 8:01 p.m. UTC | #2
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@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@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
Zivkovic, Milos Oct. 28, 2020, 8:25 p.m. UTC | #3
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@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@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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

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
+
     }
 
 #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;