| Message ID | 20230707094847.25324-13-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/22] lavc/encode: print separate messages for unknown and unsupported formats | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| yinshiyou/make_loongarch64 | warning | New warnings during build |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_x86 | warning | New warnings during build |
On Fri, Jul 07, 2023 at 11:48:38AM +0200, Anton Khirnov wrote: > When the user explicitly specifies a pixel format that is not supported > by the encoder, ffmpeg CLI will currently use some heuristics to pick > another supported format. This is wrong and the correct action here is > to fail. > > Surprisingly, a number of FATE tests are affected by this and actually > use a different pixel format than is specified in the makefiles. > --- > fftools/ffmpeg_filter.c | 36 ++----------------- > tests/fate/fits.mak | 6 ++-- > tests/fate/lavf-video.mak | 2 +- > tests/fate/vcodec.mak | 4 +-- > .../{fitsdec-gbrap16le => fitsdec-gbrap16be} | 4 +-- > .../fate/{fitsdec-gbrp16 => fitsdec-gbrp16be} | 4 +-- > tests/ref/lavf/gif | 2 +- > 7 files changed, 13 insertions(+), 45 deletions(-) > rename tests/ref/fate/{fitsdec-gbrap16le => fitsdec-gbrap16be} (79%) > rename tests/ref/fate/{fitsdec-gbrp16 => fitsdec-gbrp16be} (79%) breaks png ./ffmpeg -y -i lena.pnm -s 696x300 -pix_fmt rgb48 -y out2.png Error while opening encoder - maybe incorrect parameters such as bit_rate, rate, width or height. Conversion failed! before ./ffprobe out2.png ... Stream #0:0: Video: png, rgb48be(pc, gbr/unknown/unknown), 696x300, 25 fps, 25 tbr, 25 tbn Yes internally its BE vs LE but thats not what the user wrote on the command line thx [...]
On Fri, Jul 07, 2023 at 11:48:38AM +0200, Anton Khirnov wrote: > When the user explicitly specifies a pixel format that is not supported > by the encoder, ffmpeg CLI will currently use some heuristics to pick > another supported format. This is wrong and the correct action here is > to fail. > > Surprisingly, a number of FATE tests are affected by this and actually > use a different pixel format than is specified in the makefiles. > --- > fftools/ffmpeg_filter.c | 36 ++----------------- > tests/fate/fits.mak | 6 ++-- > tests/fate/lavf-video.mak | 2 +- > tests/fate/vcodec.mak | 4 +-- > .../{fitsdec-gbrap16le => fitsdec-gbrap16be} | 4 +-- > .../fate/{fitsdec-gbrp16 => fitsdec-gbrp16be} | 4 +-- > tests/ref/lavf/gif | 2 +- > 7 files changed, 13 insertions(+), 45 deletions(-) > rename tests/ref/fate/{fitsdec-gbrap16le => fitsdec-gbrap16be} (79%) > rename tests/ref/fate/{fitsdec-gbrp16 => fitsdec-gbrp16be} (79%) This also seems to break the user interface and seems to violates what is documented: (unless iam too tired and mix something up here) @item -pix_fmt[:@var{stream_specifier}] @var{format} (@emph{input/output,per-stream}) Set pixel format. Use @code{-pix_fmts} to show all the supported pixel formats. If the selected pixel format can not be selected, ffmpeg will print a warning and select the best pixel format supported by the encoder. If @var{pix_fmt} is prefixed by a @code{+}, ffmpeg will exit with an error if the requested pixel format can not be selected, and automatic conversions inside filtergraphs are disabled. If @var{pix_fmt} is a single @code{+}, ffmpeg selects the same pixel format as the input (or graph output) and automatic conversions are disabled. [...]
Quoting Michael Niedermayer (2023-07-09 00:15:30) > On Fri, Jul 07, 2023 at 11:48:38AM +0200, Anton Khirnov wrote: > > When the user explicitly specifies a pixel format that is not supported > > by the encoder, ffmpeg CLI will currently use some heuristics to pick > > another supported format. This is wrong and the correct action here is > > to fail. > > > > Surprisingly, a number of FATE tests are affected by this and actually > > use a different pixel format than is specified in the makefiles. > > --- > > fftools/ffmpeg_filter.c | 36 ++----------------- > > tests/fate/fits.mak | 6 ++-- > > tests/fate/lavf-video.mak | 2 +- > > tests/fate/vcodec.mak | 4 +-- > > .../{fitsdec-gbrap16le => fitsdec-gbrap16be} | 4 +-- > > .../fate/{fitsdec-gbrp16 => fitsdec-gbrp16be} | 4 +-- > > tests/ref/lavf/gif | 2 +- > > 7 files changed, 13 insertions(+), 45 deletions(-) > > rename tests/ref/fate/{fitsdec-gbrap16le => fitsdec-gbrap16be} (79%) > > rename tests/ref/fate/{fitsdec-gbrp16 => fitsdec-gbrp16be} (79%) > > breaks png > > ./ffmpeg -y -i lena.pnm -s 696x300 -pix_fmt rgb48 -y out2.png > > Error while opening encoder - maybe incorrect parameters such as bit_rate, rate, width or height. > Conversion failed! > > before > ./ffprobe out2.png > ... > Stream #0:0: Video: png, rgb48be(pc, gbr/unknown/unknown), 696x300, 25 fps, 25 tbr, 25 tbn > > Yes internally its BE vs LE but thats not what the user wrote on the command line The pixel format that is printed in your "before" is different from what the user wrote as well, so the new behavior is correct IMO.
diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c index caf85194c5..c283ee2b7a 100644 --- a/fftools/ffmpeg_filter.c +++ b/fftools/ffmpeg_filter.c @@ -277,43 +277,12 @@ static const enum AVPixelFormat *get_compliance_normal_pix_fmts(const AVCodec *c } } -static enum AVPixelFormat -choose_pixel_fmt(const AVCodec *codec, enum AVPixelFormat target, - int strict_std_compliance) -{ - if (codec && codec->pix_fmts) { - const enum AVPixelFormat *p = codec->pix_fmts; - const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(target); - //FIXME: This should check for AV_PIX_FMT_FLAG_ALPHA after PAL8 pixel format without alpha is implemented - int has_alpha = desc ? desc->nb_components % 2 == 0 : 0; - enum AVPixelFormat best= AV_PIX_FMT_NONE; - - if (strict_std_compliance > FF_COMPLIANCE_UNOFFICIAL) { - 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); - if (*p == target) - break; - } - if (*p == AV_PIX_FMT_NONE) { - if (target != AV_PIX_FMT_NONE) - av_log(NULL, AV_LOG_WARNING, - "Incompatible pixel format '%s' for codec '%s', auto-selecting format '%s'\n", - av_get_pix_fmt_name(target), - codec->name, - av_get_pix_fmt_name(best)); - return best; - } - } - return target; -} - /* May return NULL (no pixel format found), a static string or a string * backed by the bprint. Nothing has been written to the AVBPrint in case * NULL is returned. The AVBPrint provided should be clean. */ static const char *choose_pix_fmts(OutputFilter *ofilter, AVBPrint *bprint) { + OutputFilterPriv *ofp = ofp_from_ofilter(ofilter); OutputStream *ost = ofilter->ost; AVCodecContext *enc = ost->enc_ctx; const AVDictionaryEntry *strict_dict = av_dict_get(ost->encoder_opts, "strict", NULL, 0); @@ -327,8 +296,7 @@ static const char *choose_pix_fmts(OutputFilter *ofilter, AVBPrint *bprint) return av_get_pix_fmt_name(ost->enc_ctx->pix_fmt); } if (ost->enc_ctx->pix_fmt != AV_PIX_FMT_NONE) { - return av_get_pix_fmt_name(choose_pixel_fmt(enc->codec, enc->pix_fmt, - ost->enc_ctx->strict_std_compliance)); + return av_get_pix_fmt_name(enc->pix_fmt); } else if (enc->codec->pix_fmts) { const enum AVPixelFormat *p; diff --git a/tests/fate/fits.mak b/tests/fate/fits.mak index b9e99d97ee..d85946bc1a 100644 --- a/tests/fate/fits.mak +++ b/tests/fate/fits.mak @@ -8,8 +8,8 @@ tests/data/fits-multi.fits: ffmpeg$(PROGSSUF)$(EXESUF) | tests/data # TODO: Use an actual 64bit input file and fix the gbrp16 test on big-endian fits-png-map-gray := gray8 fits-png-map-gbrp := rgb24 -fits-png-map-gbrp16 := rgb48 -fits-png-map-gbrap16le := rgba64 +fits-png-map-gbrp16be := rgb48 +fits-png-map-gbrap16be := rgba64 FATE_FITS_DEC-$(call FRAMECRC, FITS, FITS, SCALE_FILTER) += fate-fitsdec-ext_data_min_max fate-fitsdec-ext_data_min_max: CMD = framecrc -i $(TARGET_SAMPLES)/fits/x0cj010ct_d0h.fit -pix_fmt gray16le -vf scale @@ -30,7 +30,7 @@ fate-fitsdec-multi: CMD = framecrc -i $(TARGET_PATH)/tests/data/fits-multi.fits fate-fitsdec%: PIXFMT = $(word 3, $(subst -, ,$(@))) fate-fitsdec%: CMD = transcode image2 $(TARGET_SAMPLES)/png1/lena-$(fits-png-map-$(PIXFMT)).png fits "-vf scale -pix_fmt $(PIXFMT)" -FATE_FITS_DEC_PIXFMT = gray gbrp gbrp16 gbrap16le +FATE_FITS_DEC_PIXFMT = gray gbrp gbrp16be gbrap16be FATE_FITS_DEC-$(call TRANSCODE, FITS, FITS, IMAGE2_DEMUXER PNG_DECODER SCALE_FILTER) += $(FATE_FITS_DEC_PIXFMT:%=fate-fitsdec-%) FATE_FITS += $(FATE_FITS_DEC-yes) diff --git a/tests/fate/lavf-video.mak b/tests/fate/lavf-video.mak index e73f8f203b..da3b114bc8 100644 --- a/tests/fate/lavf-video.mak +++ b/tests/fate/lavf-video.mak @@ -27,7 +27,7 @@ fate-lavf-gbrp.fits: CMD = lavf_video "-pix_fmt gbrp" fate-lavf-gbrap.fits: CMD = lavf_video "-pix_fmt gbrap" fate-lavf-gbrp16be.fits: CMD = lavf_video "-pix_fmt gbrp16be" fate-lavf-gbrap16be.fits: CMD = lavf_video "-pix_fmt gbrap16be" -fate-lavf-gif: CMD = lavf_video "-pix_fmt rgb24" +fate-lavf-gif: CMD = lavf_video "-pix_fmt rgb8" FATE_AVCONV += $(FATE_LAVF_VIDEO) fate-lavf-video fate-lavf: $(FATE_LAVF_VIDEO) diff --git a/tests/fate/vcodec.mak b/tests/fate/vcodec.mak index 2839e54de8..e32d28c556 100644 --- a/tests/fate/vcodec.mak +++ b/tests/fate/vcodec.mak @@ -210,9 +210,9 @@ fate-vsynth%-h263p: ENCOPTS = -qscale 2 -flags +aic -umv 1 -aiv 1 - FATE_VCODEC_SCALE-$(call ENCDEC, HUFFYUV, AVI) += huffyuv huffyuvbgr24 huffyuvbgra fate-vsynth%-huffyuv: ENCOPTS = -c:v huffyuv -pix_fmt yuv422p -sws_flags neighbor fate-vsynth%-huffyuv: DECOPTS = -sws_flags neighbor -fate-vsynth%-huffyuvbgr24: ENCOPTS = -c:v huffyuv -pix_fmt bgr24 -sws_flags neighbor +fate-vsynth%-huffyuvbgr24: ENCOPTS = -c:v huffyuv -pix_fmt rgb24 -sws_flags neighbor fate-vsynth%-huffyuvbgr24: DECOPTS = -sws_flags neighbor -fate-vsynth%-huffyuvbgra: ENCOPTS = -c:v huffyuv -pix_fmt bgr32 -sws_flags neighbor +fate-vsynth%-huffyuvbgra: ENCOPTS = -c:v huffyuv -pix_fmt rgb32 -sws_flags neighbor fate-vsynth%-huffyuvbgra: DECOPTS = -sws_flags neighbor FATE_VCODEC_SCALE-$(call ENCDEC, JPEGLS, AVI) += jpegls diff --git a/tests/ref/fate/fitsdec-gbrap16le b/tests/ref/fate/fitsdec-gbrap16be similarity index 79% rename from tests/ref/fate/fitsdec-gbrap16le rename to tests/ref/fate/fitsdec-gbrap16be index 53ef980b13..1174a0f1d8 100644 --- a/tests/ref/fate/fitsdec-gbrap16le +++ b/tests/ref/fate/fitsdec-gbrap16be @@ -1,5 +1,5 @@ -64526d8da12d1fa07ceea5725647076f *tests/data/fate/fitsdec-gbrap16le.fits -135360 tests/data/fate/fitsdec-gbrap16le.fits +64526d8da12d1fa07ceea5725647076f *tests/data/fate/fitsdec-gbrap16be.fits +135360 tests/data/fate/fitsdec-gbrap16be.fits #tb 0: 1/1 #media_type 0: video #codec_id 0: rawvideo diff --git a/tests/ref/fate/fitsdec-gbrp16 b/tests/ref/fate/fitsdec-gbrp16be similarity index 79% rename from tests/ref/fate/fitsdec-gbrp16 rename to tests/ref/fate/fitsdec-gbrp16be index 9250690e9b..ff4ca9e65c 100644 --- a/tests/ref/fate/fitsdec-gbrp16 +++ b/tests/ref/fate/fitsdec-gbrp16be @@ -1,5 +1,5 @@ -2078208c93ba417d3fe150ba42bf5a30 *tests/data/fate/fitsdec-gbrp16.fits -103680 tests/data/fate/fitsdec-gbrp16.fits +2078208c93ba417d3fe150ba42bf5a30 *tests/data/fate/fitsdec-gbrp16be.fits +103680 tests/data/fate/fitsdec-gbrp16be.fits #tb 0: 1/1 #media_type 0: video #codec_id 0: rawvideo diff --git a/tests/ref/lavf/gif b/tests/ref/lavf/gif index fc94b9df3d..7f353df286 100644 --- a/tests/ref/lavf/gif +++ b/tests/ref/lavf/gif @@ -1,3 +1,3 @@ e35f5ea283bbcb249818e0078ec72664 *tests/data/lavf/lavf.gif 2011766 tests/data/lavf/lavf.gif -tests/data/lavf/lavf.gif CRC=0x2429faff +tests/data/lavf/lavf.gif CRC=0x37f4d323
When the user explicitly specifies a pixel format that is not supported by the encoder, ffmpeg CLI will currently use some heuristics to pick another supported format. This is wrong and the correct action here is to fail. Surprisingly, a number of FATE tests are affected by this and actually use a different pixel format than is specified in the makefiles. --- fftools/ffmpeg_filter.c | 36 ++----------------- tests/fate/fits.mak | 6 ++-- tests/fate/lavf-video.mak | 2 +- tests/fate/vcodec.mak | 4 +-- .../{fitsdec-gbrap16le => fitsdec-gbrap16be} | 4 +-- .../fate/{fitsdec-gbrp16 => fitsdec-gbrp16be} | 4 +-- tests/ref/lavf/gif | 2 +- 7 files changed, 13 insertions(+), 45 deletions(-) rename tests/ref/fate/{fitsdec-gbrap16le => fitsdec-gbrap16be} (79%) rename tests/ref/fate/{fitsdec-gbrp16 => fitsdec-gbrp16be} (79%)