| Message ID | 20210124144113.51841-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/utils: always preserve container dimensions for all streams | expand |
| 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 |
On Sun, Jan 24, 2021 at 11:41:13AM -0300, James Almer wrote: > If a decoder is used for probing it may change the dimensions reported by the > demuxer, either by the lowres factor or because of assorted frames reporting > different dimensions, and in a codec copy scenario, the last dimensions > arbitrarily set by it could end up being propagated to the muxer. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/utils.c | 10 ++++++---- > tests/ref/fate/cbs-vp9-vp90-2-05-resize | 2 +- > tests/ref/fate/redcode-demux | 2 +- > tests/ref/fate/wtv-demux | 4 ++-- > 4 files changed, 10 insertions(+), 8 deletions(-) > breaks: ./ffmpeg -i tickets/2892/MPEG_tbn_test.mov -c:v copy -c:a copy -vtag mx3n -timecode 10:00:00:00 -vframes 3 file.mov ... [mov @ 0x558785108e00] D-10/IMX must use 720x608 or 720x512 video resolution Could not write header for output file #0 (incorrect codec parameters ?): Invalid argument Error initializing output stream 0:1 -- [...]
On 1/25/2021 7:46 PM, Michael Niedermayer wrote: > On Sun, Jan 24, 2021 at 11:41:13AM -0300, James Almer wrote: >> If a decoder is used for probing it may change the dimensions reported by the >> demuxer, either by the lowres factor or because of assorted frames reporting >> different dimensions, and in a codec copy scenario, the last dimensions >> arbitrarily set by it could end up being propagated to the muxer. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/utils.c | 10 ++++++---- >> tests/ref/fate/cbs-vp9-vp90-2-05-resize | 2 +- >> tests/ref/fate/redcode-demux | 2 +- >> tests/ref/fate/wtv-demux | 4 ++-- >> 4 files changed, 10 insertions(+), 8 deletions(-) >> > > breaks: > > ./ffmpeg -i tickets/2892/MPEG_tbn_test.mov -c:v copy -c:a copy -vtag mx3n -timecode 10:00:00:00 -vframes 3 file.mov > ... > [mov @ 0x558785108e00] D-10/IMX must use 720x608 or 720x512 video resolution So the source mov is faulty and reports a wrong resolution? And trying to pass it through instead of letting a decoder change it makes it fail to mux because the muxer refuses to create non compliant files. That means if there's no decoder in the build, you'd get the same result as without this patch. The mere existence of this code here means that letting decoders set the resolution in a codec copy scenario is not ideal, as shown by the fact one can tell it to make up an arbitrary resolution like it's the case of lowres, or it can just pick up one from an arbitrary frame. Hence prioritizing what the demuxer reads from the container feels like the correct thing to do. But of course, broken files are a thing. I don't know, maybe a new AVFMT_FLAG_ flag to choose between giving priority to what the container reports or what a decoder sets, if any is present, could work around this? Or maybe it's the library user the one that should look at what the demuxer read from the container and decide if a parser or decoder should give its opinion about it. > Could not write header for output file #0 (incorrect codec parameters ?): Invalid argument > Error initializing output stream 0:1 -- > > [...] > > > _______________________________________________ > 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". >
Quoting James Almer (2021-01-26 00:17:51) > On 1/25/2021 7:46 PM, Michael Niedermayer wrote: > > On Sun, Jan 24, 2021 at 11:41:13AM -0300, James Almer wrote: > >> If a decoder is used for probing it may change the dimensions reported by the > >> demuxer, either by the lowres factor or because of assorted frames reporting > >> different dimensions, and in a codec copy scenario, the last dimensions > >> arbitrarily set by it could end up being propagated to the muxer. > >> > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> libavformat/utils.c | 10 ++++++---- > >> tests/ref/fate/cbs-vp9-vp90-2-05-resize | 2 +- > >> tests/ref/fate/redcode-demux | 2 +- > >> tests/ref/fate/wtv-demux | 4 ++-- > >> 4 files changed, 10 insertions(+), 8 deletions(-) > >> > > > > breaks: > > > > ./ffmpeg -i tickets/2892/MPEG_tbn_test.mov -c:v copy -c:a copy -vtag mx3n -timecode 10:00:00:00 -vframes 3 file.mov > > ... > > [mov @ 0x558785108e00] D-10/IMX must use 720x608 or 720x512 video resolution > > So the source mov is faulty and reports a wrong resolution? And trying > to pass it through instead of letting a decoder change it makes it fail > to mux because the muxer refuses to create non compliant files. > That means if there's no decoder in the build, you'd get the same result > as without this patch. > > The mere existence of this code here means that letting decoders set the > resolution in a codec copy scenario is not ideal, as shown by the fact > one can tell it to make up an arbitrary resolution like it's the case of > lowres, or it can just pick up one from an arbitrary frame. Hence > prioritizing what the demuxer reads from the container feels like the > correct thing to do. But of course, broken files are a thing. > > I don't know, maybe a new AVFMT_FLAG_ flag to choose between giving > priority to what the container reports or what a decoder sets, if any is > present, could work around this? I'd prefer an ffmpeg.c flag if anything. It is not lavf's job to set policy on what information is more trustworthy. The demuxer should export what is written in the container, the decoder/parser should export what is written in the codec. The user then decides which gets used.
On Wed, Jan 27, 2021 at 2:56 PM Anton Khirnov <anton@khirnov.net> wrote: > > Quoting James Almer (2021-01-26 00:17:51) > > On 1/25/2021 7:46 PM, Michael Niedermayer wrote: > > > On Sun, Jan 24, 2021 at 11:41:13AM -0300, James Almer wrote: > > >> If a decoder is used for probing it may change the dimensions reported by the > > >> demuxer, either by the lowres factor or because of assorted frames reporting > > >> different dimensions, and in a codec copy scenario, the last dimensions > > >> arbitrarily set by it could end up being propagated to the muxer. > > >> > > >> Signed-off-by: James Almer <jamrial@gmail.com> > > >> --- > > >> libavformat/utils.c | 10 ++++++---- > > >> tests/ref/fate/cbs-vp9-vp90-2-05-resize | 2 +- > > >> tests/ref/fate/redcode-demux | 2 +- > > >> tests/ref/fate/wtv-demux | 4 ++-- > > >> 4 files changed, 10 insertions(+), 8 deletions(-) > > >> > > > > > > breaks: > > > > > > ./ffmpeg -i tickets/2892/MPEG_tbn_test.mov -c:v copy -c:a copy -vtag mx3n -timecode 10:00:00:00 -vframes 3 file.mov > > > ... > > > [mov @ 0x558785108e00] D-10/IMX must use 720x608 or 720x512 video resolution > > > > So the source mov is faulty and reports a wrong resolution? And trying > > to pass it through instead of letting a decoder change it makes it fail > > to mux because the muxer refuses to create non compliant files. > > That means if there's no decoder in the build, you'd get the same result > > as without this patch. > > > > The mere existence of this code here means that letting decoders set the > > resolution in a codec copy scenario is not ideal, as shown by the fact > > one can tell it to make up an arbitrary resolution like it's the case of > > lowres, or it can just pick up one from an arbitrary frame. Hence > > prioritizing what the demuxer reads from the container feels like the > > correct thing to do. But of course, broken files are a thing. > > > > I don't know, maybe a new AVFMT_FLAG_ flag to choose between giving > > priority to what the container reports or what a decoder sets, if any is > > present, could work around this? > > I'd prefer an ffmpeg.c flag if anything. > > It is not lavf's job to set policy on what information is more > trustworthy. > The demuxer should export what is written in the container, the > decoder/parser should export what is written in the codec. The user then > decides which gets used. > I concur with that approach. User code (including ffmpeg.c) should set a preference if required, avformat should expose container information if present, otherwise information is lost and not recoverable. - Hendrik
diff --git a/libavformat/utils.c b/libavformat/utils.c index 6f100294a1..bfc8436fc9 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -4105,6 +4105,7 @@ FF_ENABLE_DEPRECATION_WARNINGS st = ic->streams[i]; if (st->internal->avctx_inited) { + AVRational orig_sar = st->codecpar->sample_aspect_ratio; int orig_w = st->codecpar->width; int orig_h = st->codecpar->height; ret = avcodec_parameters_from_context(st->codecpar, st->internal->avctx); @@ -4113,13 +4114,14 @@ FF_ENABLE_DEPRECATION_WARNINGS ret = add_coded_side_data(st, st->internal->avctx); if (ret < 0) goto find_stream_info_err; -#if FF_API_LOWRES - // The decoder might reduce the video size by the lowres factor. - if (st->internal->avctx->lowres && orig_w) { + + // The decoder might change the video size. + if (orig_w && orig_h) { + if (st->codecpar->width != orig_w && st->codecpar->height != orig_h) + st->codecpar->sample_aspect_ratio = orig_sar; st->codecpar->width = orig_w; st->codecpar->height = orig_h; } -#endif } #if FF_API_LAVF_AVCTX diff --git a/tests/ref/fate/cbs-vp9-vp90-2-05-resize b/tests/ref/fate/cbs-vp9-vp90-2-05-resize index 8f036bba81..37a37ff1ea 100644 --- a/tests/ref/fate/cbs-vp9-vp90-2-05-resize +++ b/tests/ref/fate/cbs-vp9-vp90-2-05-resize @@ -1 +1 @@ -6838422ebb45df353a2bad62b9aff8e9 +1c39300b93fe110e1db30974e5d3479d diff --git a/tests/ref/fate/redcode-demux b/tests/ref/fate/redcode-demux index 45119ec71e..c6e0b6de5c 100644 --- a/tests/ref/fate/redcode-demux +++ b/tests/ref/fate/redcode-demux @@ -1,7 +1,7 @@ #tb 0: 1/240000 #media_type 0: video #codec_id 0: jpeg2000 -#dimensions 0: 2048x1152 +#dimensions 0: 4096x2304 #sar 0: 0/1 #tb 1: 1/240000 #media_type 1: audio diff --git a/tests/ref/fate/wtv-demux b/tests/ref/fate/wtv-demux index abe85a4ab6..388ba796d3 100644 --- a/tests/ref/fate/wtv-demux +++ b/tests/ref/fate/wtv-demux @@ -3,8 +3,8 @@ #tb 0: 1/10000000 #media_type 0: video #codec_id 0: mpeg2video -#dimensions 0: 720x576 -#sar 0: 64/45 +#dimensions 0: 704x480 +#sar 0: 0/1 #tb 1: 1/10000000 #media_type 1: audio #codec_id 1: mp2
If a decoder is used for probing it may change the dimensions reported by the demuxer, either by the lowres factor or because of assorted frames reporting different dimensions, and in a codec copy scenario, the last dimensions arbitrarily set by it could end up being propagated to the muxer. Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/utils.c | 10 ++++++---- tests/ref/fate/cbs-vp9-vp90-2-05-resize | 2 +- tests/ref/fate/redcode-demux | 2 +- tests/ref/fate/wtv-demux | 4 ++-- 4 files changed, 10 insertions(+), 8 deletions(-)