diff mbox series

[FFmpeg-devel] fftools/ffmpeg_filter: Fix check for mjpeg encoder

Message ID HE1PR0301MB215459BE4AF81C7678900B638F4F9@HE1PR0301MB2154.eurprd03.prod.outlook.com
State Accepted
Commit 4e39cd67b7275dc525684e54d0bd651ef4ee149b
Headers show
Series [FFmpeg-devel] fftools/ffmpeg_filter: Fix check for mjpeg encoder
Related show

Checks

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

Commit Message

Andreas Rheinhardt April 13, 2021, 5:31 p.m. UTC
The MJPEG encoder supports some pixel format/color range combinations
only when strictness is set to unofficial or less. Before commit
059fc2d9da5364627613fb3e6424079e14dbdfd3 said encoder's pix_fmts array
only included the pixel formats supported with default strictness.
When strictness was <= unofficial, fftools/ffmpeg_filter.c used
an extended list of pixel formats instead of the encoder's including
the pixel formats only supported when strictness <= unofficial.

Said commit turned the logic around: The encoder's pix_fmts array now
included all pixel formats and fftools/ffmpeg_filter.c instead used
a small list of all pixel formats supported when strictness is >
unofficial and the encoder's pixel formats instead. In particular,
the codec's pix_fmt is not used when strictness is normal.

This works for the mjpeg encoder; yet it did not work for other
(hardware-based) mjpeg encoders, because the check for whether one is
using the MJPEG encoder is wrong: It just checks the codec id.
So if one used strict unofficial with a hardware-accelerated MJPEG
encoder before commit 059fc2d9da53, the unofficial (non-hardware)
pixel formats of the MJPEG encoder would be used; since said commit
the codec's pixel formats are overridden at ordinary strictness
by the ordinary MJPEG pixel formats. This leads to format conversion
errors lateron which were reported in #9186.

The solution to this is to check AVCodec.name instead of its id.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 fftools/ffmpeg_filter.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Eoff, Ullysses A April 13, 2021, 7:54 p.m. UTC | #1
Fixes #9186 for me.

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas Rheinhardt
> Sent: Tuesday, April 13, 2021 10:32 AM
> To: ffmpeg-devel@ffmpeg.org
> Cc: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> Subject: [FFmpeg-devel] [PATCH] fftools/ffmpeg_filter: Fix check for mjpeg encoder
> 
> The MJPEG encoder supports some pixel format/color range combinations
> only when strictness is set to unofficial or less. Before commit
> 059fc2d9da5364627613fb3e6424079e14dbdfd3 said encoder's pix_fmts array
> only included the pixel formats supported with default strictness.
> When strictness was <= unofficial, fftools/ffmpeg_filter.c used
> an extended list of pixel formats instead of the encoder's including
> the pixel formats only supported when strictness <= unofficial.
> 
> Said commit turned the logic around: The encoder's pix_fmts array now
> included all pixel formats and fftools/ffmpeg_filter.c instead used
> a small list of all pixel formats supported when strictness is >
> unofficial and the encoder's pixel formats instead. In particular,
> the codec's pix_fmt is not used when strictness is normal.
> 
> This works for the mjpeg encoder; yet it did not work for other
> (hardware-based) mjpeg encoders, because the check for whether one is
> using the MJPEG encoder is wrong: It just checks the codec id.
> So if one used strict unofficial with a hardware-accelerated MJPEG
> encoder before commit 059fc2d9da53, the unofficial (non-hardware)
> pixel formats of the MJPEG encoder would be used; since said commit
> the codec's pixel formats are overridden at ordinary strictness
> by the ordinary MJPEG pixel formats. This leads to format conversion
> errors lateron which were reported in #9186.
> 
> The solution to this is to check AVCodec.name instead of its id.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  fftools/ffmpeg_filter.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
> index 400f5a4188..e7c05eb3f9 100644
> --- a/fftools/ffmpeg_filter.c
> +++ b/fftools/ffmpeg_filter.c
> @@ -41,13 +41,13 @@
> 
>  // FIXME: YUV420P etc. are actually supported with full color range,
>  // yet the latter information isn't available here.
> -static const enum AVPixelFormat *get_compliance_normal_pix_fmts(enum AVCodecID codec_id, const enum AVPixelFormat
> default_formats[])
> +static const enum AVPixelFormat *get_compliance_normal_pix_fmts(const AVCodec *codec, const enum AVPixelFormat
> default_formats[])
>  {
>      static const enum AVPixelFormat mjpeg_formats[] =
>          { AV_PIX_FMT_YUVJ420P, AV_PIX_FMT_YUVJ422P, AV_PIX_FMT_YUVJ444P,
>            AV_PIX_FMT_NONE };
> 
> -    if (codec_id == AV_CODEC_ID_MJPEG) {
> +    if (!strcmp(codec->name, "mjpeg")) {
>          return mjpeg_formats;
>      } else {
>          return default_formats;
> @@ -65,7 +65,7 @@ static enum AVPixelFormat choose_pixel_fmt(AVStream *st, AVCodecContext *enc_ctx
>          enum AVPixelFormat best= AV_PIX_FMT_NONE;
> 
>          if (enc_ctx->strict_std_compliance > FF_COMPLIANCE_UNOFFICIAL) {
> -            p = get_compliance_normal_pix_fmts(enc_ctx->codec_id, p);
> +            p = get_compliance_normal_pix_fmts(codec, p);
>          }
>          for (; *p != AV_PIX_FMT_NONE; p++) {
>              best = av_find_best_pix_fmt_of_2(best, *p, target, has_alpha, NULL);
> @@ -113,7 +113,7 @@ static char *choose_pix_fmts(OutputFilter *ofilter)
> 
>          p = ost->enc->pix_fmts;
>          if (ost->enc_ctx->strict_std_compliance > FF_COMPLIANCE_UNOFFICIAL) {
> -            p = get_compliance_normal_pix_fmts(ost->enc_ctx->codec_id, p);
> +            p = get_compliance_normal_pix_fmts(ost->enc, p);
>          }
> 
>          for (; *p != AV_PIX_FMT_NONE; p++) {
> --
> 2.27.0
> 
> _______________________________________________
> 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/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
index 400f5a4188..e7c05eb3f9 100644
--- a/fftools/ffmpeg_filter.c
+++ b/fftools/ffmpeg_filter.c
@@ -41,13 +41,13 @@ 
 
 // FIXME: YUV420P etc. are actually supported with full color range,
 // yet the latter information isn't available here.
-static const enum AVPixelFormat *get_compliance_normal_pix_fmts(enum AVCodecID codec_id, const enum AVPixelFormat default_formats[])
+static const enum AVPixelFormat *get_compliance_normal_pix_fmts(const AVCodec *codec, const enum AVPixelFormat default_formats[])
 {
     static const enum AVPixelFormat mjpeg_formats[] =
         { AV_PIX_FMT_YUVJ420P, AV_PIX_FMT_YUVJ422P, AV_PIX_FMT_YUVJ444P,
           AV_PIX_FMT_NONE };
 
-    if (codec_id == AV_CODEC_ID_MJPEG) {
+    if (!strcmp(codec->name, "mjpeg")) {
         return mjpeg_formats;
     } else {
         return default_formats;
@@ -65,7 +65,7 @@  static enum AVPixelFormat choose_pixel_fmt(AVStream *st, AVCodecContext *enc_ctx
         enum AVPixelFormat best= AV_PIX_FMT_NONE;
 
         if (enc_ctx->strict_std_compliance > FF_COMPLIANCE_UNOFFICIAL) {
-            p = get_compliance_normal_pix_fmts(enc_ctx->codec_id, p);
+            p = get_compliance_normal_pix_fmts(codec, p);
         }
         for (; *p != AV_PIX_FMT_NONE; p++) {
             best = av_find_best_pix_fmt_of_2(best, *p, target, has_alpha, NULL);
@@ -113,7 +113,7 @@  static char *choose_pix_fmts(OutputFilter *ofilter)
 
         p = ost->enc->pix_fmts;
         if (ost->enc_ctx->strict_std_compliance > FF_COMPLIANCE_UNOFFICIAL) {
-            p = get_compliance_normal_pix_fmts(ost->enc_ctx->codec_id, p);
+            p = get_compliance_normal_pix_fmts(ost->enc, p);
         }
 
         for (; *p != AV_PIX_FMT_NONE; p++) {