diff mbox series

[FFmpeg-devel] lavf/utils: reset event_flags if extradata is not extracted correctly

Message ID 20210124150556.1089358-1-linjie.justin.fu@gmail.com
State New
Headers show
Series [FFmpeg-devel] lavf/utils: reset event_flags if extradata is not extracted correctly
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

Linjie Fu Jan. 24, 2021, 3:05 p.m. UTC
Regression since 87f0c8280.

If the extradata of a stream could not be extracted correctly,
codec_info_nb_frames would remain zero, while st->event_flag would still
be marked as AVSTREAM_EVENT_FLAG_NEW_PACKETS.

The two expressions could be different in this case, hence reset
event_flags and calculate the correct score.

Fix #9029.

Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
---
 libavformat/utils.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Anton Khirnov Jan. 24, 2021, 4:49 p.m. UTC | #1
Quoting Linjie Fu (2021-01-24 16:05:56)
> Regression since 87f0c8280.
> 
> If the extradata of a stream could not be extracted correctly,
> codec_info_nb_frames would remain zero, while st->event_flag would still
> be marked as AVSTREAM_EVENT_FLAG_NEW_PACKETS.
> 
> The two expressions could be different in this case, hence reset
> event_flags and calculate the correct score.
> 
> Fix #9029.

The ticket mentions ffplay, but ffplay does not access event_flags.
Linjie Fu Jan. 25, 2021, 3:49 a.m. UTC | #2
On Mon, Jan 25, 2021 at 12:49 AM Anton Khirnov <anton@khirnov.net> wrote:
>
> Quoting Linjie Fu (2021-01-24 16:05:56)
> > Regression since 87f0c8280.
> >
> > If the extradata of a stream could not be extracted correctly,
> > codec_info_nb_frames would remain zero, while st->event_flag would still
> > be marked as AVSTREAM_EVENT_FLAG_NEW_PACKETS.
> >
> > The two expressions could be different in this case, hence reset
> > event_flags and calculate the correct score.
> >
> > Fix #9029.
>
> The ticket mentions ffplay, but ffplay does not access event_flags.
>

You are right, this helps ffmpeg cmdline to copy and dump:
$ ffmpeg -i sample_cut.flv -c copy -y dump.mp4

Before 87f0c8280 and after this patch:

audio streams could be dumped and playable, while video streams still not.

87f0c8280..master:
Error reporting and quits.

[mp4 @ 0x7fb276809a00] dimensions not set
Could not write header for output file #0 (incorrect codec parameters
?): Invalid argument
Error initializing output stream 0:1 --

I'd change the commit message to "#9029 related" for this, since the
ffplay issue remains the same.

- linjie
Anton Khirnov Jan. 25, 2021, 1:08 p.m. UTC | #3
Quoting Linjie Fu (2021-01-25 04:49:21)
> On Mon, Jan 25, 2021 at 12:49 AM Anton Khirnov <anton@khirnov.net> wrote:
> >
> > Quoting Linjie Fu (2021-01-24 16:05:56)
> > > Regression since 87f0c8280.
> > >
> > > If the extradata of a stream could not be extracted correctly,
> > > codec_info_nb_frames would remain zero, while st->event_flag would still
> > > be marked as AVSTREAM_EVENT_FLAG_NEW_PACKETS.
> > >
> > > The two expressions could be different in this case, hence reset
> > > event_flags and calculate the correct score.
> > >
> > > Fix #9029.
> >
> > The ticket mentions ffplay, but ffplay does not access event_flags.
> >
> 
> You are right, this helps ffmpeg cmdline to copy and dump:
> $ ffmpeg -i sample_cut.flv -c copy -y dump.mp4
> 
> Before 87f0c8280 and after this patch:
> 
> audio streams could be dumped and playable, while video streams still not.
> 
> 87f0c8280..master:
> Error reporting and quits.
> 
> [mp4 @ 0x7fb276809a00] dimensions not set
> Could not write header for output file #0 (incorrect codec parameters
> ?): Invalid argument
> Error initializing output stream 0:1 --
> 
> I'd change the commit message to "#9029 related" for this, since the
> ffplay issue remains the same.

I am still not convinced this is really a regression. You can still
process the file by explicitly mapping only the audio stream.

This file is damaged and the video stream is not readable. IMO it is
better to report an error and let the user decide what to do with it
than silently skip the video stream while pretending everything is fine.
Linjie Fu Jan. 30, 2021, 6:44 a.m. UTC | #4
On Mon, Jan 25, 2021 at 9:08 PM Anton Khirnov <anton@khirnov.net> wrote:
>
> Quoting Linjie Fu (2021-01-25 04:49:21)
> > On Mon, Jan 25, 2021 at 12:49 AM Anton Khirnov <anton@khirnov.net> wrote:
> > >
> > > Quoting Linjie Fu (2021-01-24 16:05:56)
> > > > Regression since 87f0c8280.
> > > >
> > > > If the extradata of a stream could not be extracted correctly,
> > > > codec_info_nb_frames would remain zero, while st->event_flag would still
> > > > be marked as AVSTREAM_EVENT_FLAG_NEW_PACKETS.
> > > >
> > > > The two expressions could be different in this case, hence reset
> > > > event_flags and calculate the correct score.
> > > >
> > > > Fix #9029.
> > >
> > > The ticket mentions ffplay, but ffplay does not access event_flags.
> > >
> >
> > You are right, this helps ffmpeg cmdline to copy and dump:
> > $ ffmpeg -i sample_cut.flv -c copy -y dump.mp4
> >
> > Before 87f0c8280 and after this patch:
> >
> > audio streams could be dumped and playable, while video streams still not.
> >
> > 87f0c8280..master:
> > Error reporting and quits.
> >
> > [mp4 @ 0x7fb276809a00] dimensions not set
> > Could not write header for output file #0 (incorrect codec parameters
> > ?): Invalid argument
> > Error initializing output stream 0:1 --
> >
> > I'd change the commit message to "#9029 related" for this, since the
> > ffplay issue remains the same.
>
> I am still not convinced this is really a regression. You can still
> process the file by explicitly mapping only the audio stream.
Okay, I'm fine with this if it won't trigger failures for to different paths.

> This file is damaged and the video stream is not readable. IMO it is
> better to report an error and let the user decide what to do with it
> than silently skip the video stream while pretending everything is fine.
VLC is able to decode/play this correctly, hence maybe need to
investigate more into this.

- linjie
Anton Khirnov Feb. 1, 2021, 10:27 a.m. UTC | #5
Quoting Linjie Fu (2021-01-30 07:44:09)
> On Mon, Jan 25, 2021 at 9:08 PM Anton Khirnov <anton@khirnov.net> wrote:
> >
> > Quoting Linjie Fu (2021-01-25 04:49:21)
> > > On Mon, Jan 25, 2021 at 12:49 AM Anton Khirnov <anton@khirnov.net> wrote:
> > > >
> > > > Quoting Linjie Fu (2021-01-24 16:05:56)
> > > > > Regression since 87f0c8280.
> > > > >
> > > > > If the extradata of a stream could not be extracted correctly,
> > > > > codec_info_nb_frames would remain zero, while st->event_flag would still
> > > > > be marked as AVSTREAM_EVENT_FLAG_NEW_PACKETS.
> > > > >
> > > > > The two expressions could be different in this case, hence reset
> > > > > event_flags and calculate the correct score.
> > > > >
> > > > > Fix #9029.
> > > >
> > > > The ticket mentions ffplay, but ffplay does not access event_flags.
> > > >
> > >
> > > You are right, this helps ffmpeg cmdline to copy and dump:
> > > $ ffmpeg -i sample_cut.flv -c copy -y dump.mp4
> > >
> > > Before 87f0c8280 and after this patch:
> > >
> > > audio streams could be dumped and playable, while video streams still not.
> > >
> > > 87f0c8280..master:
> > > Error reporting and quits.
> > >
> > > [mp4 @ 0x7fb276809a00] dimensions not set
> > > Could not write header for output file #0 (incorrect codec parameters
> > > ?): Invalid argument
> > > Error initializing output stream 0:1 --
> > >
> > > I'd change the commit message to "#9029 related" for this, since the
> > > ffplay issue remains the same.
> >
> > I am still not convinced this is really a regression. You can still
> > process the file by explicitly mapping only the audio stream.
> Okay, I'm fine with this if it won't trigger failures for to different paths.
> 
> > This file is damaged and the video stream is not readable. IMO it is
> > better to report an error and let the user decide what to do with it
> > than silently skip the video stream while pretending everything is fine.
> VLC is able to decode/play this correctly, hence maybe need to
> investigate more into this.

VLC can also play only the audio stream. ffmpeg handles it as I
described above, which seems appropriate to me.

I suppose ffplay could be changed to be more lenient about decoding
errors, but then I see ffplay mainly as a testing/development tool
rather than a serious player.
diff mbox series

Patch

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 6f100294a1..de397a209e 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -3895,8 +3895,10 @@  FF_ENABLE_DEPRECATION_WARNINGS
         }
         if (!st->internal->avctx->extradata) {
             ret = extract_extradata(st, pkt);
-            if (ret < 0)
+            if (ret < 0) {
+                st->event_flags = 0;
                 goto unref_then_goto_end;
+            }
         }
 
         /* If still no information, we try to open the codec and to