| Message ID | CAO5RS13CP40rCLd3DBpKw8KJ3aFJt4dHCECu6H=asiVdU-aaCA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/qsvenc: use color description for h264 and hevc | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | warning | Make failed |
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.
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
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 --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;
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(-)