diff mbox series

[FFmpeg-devel] avformat/utils: always preserve container dimensions for all streams

Message ID 20210124144113.51841-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/utils: always preserve container dimensions for all streams
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

James Almer Jan. 24, 2021, 2:41 p.m. UTC
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(-)

Comments

Michael Niedermayer Jan. 25, 2021, 10:46 p.m. UTC | #1
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 -- 

[...]
James Almer Jan. 25, 2021, 11:17 p.m. UTC | #2
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".
>
Anton Khirnov Jan. 27, 2021, 1:56 p.m. UTC | #3
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.
Hendrik Leppkes Jan. 27, 2021, 3:01 p.m. UTC | #4
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 mbox series

Patch

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