[FFmpeg-devel] [PATCHv2] avformat/mp3dec, rmdec: check return value of ffio_ensure_seekback

Ganesh Ajjanagadde gajjanag at mit.edu
Wed Jan 6 06:51:15 CET 2016


On Tue, Jan 5, 2016 at 4:57 PM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Tue, Jan 05, 2016 at 01:57:09PM -0800, Ganesh Ajjanagadde wrote:
>> On Tue, Jan 5, 2016 at 11:01 AM, wm4 <nfxjfg at googlemail.com> wrote:
>> > On Tue, 5 Jan 2016 08:32:02 -0800
>> > Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>> >
>> >> On Tue, Jan 5, 2016 at 5:29 AM, wm4 <nfxjfg at googlemail.com> wrote:
>> >> > On Mon,  4 Jan 2016 17:50:01 -0800
>> >> > Ganesh Ajjanagadde <gajjanagadde at gmail.com> wrote:
>> >> >
>> >> >> ffio_ensure_seekback can fail due to e.g ENOMEM. This return value is
>> >> >> checked here and a diagnostic is logged.
>> >> >>
>> >> >> All usage of ffio_ensure_seekback in the codebase now has the return value checked.
>> >> >>
>> >> >> Reviewed-by: wm4 <nfxjfg at googlemail.com>
>> >> >> Reviewed-by: Ronald S. Bultje <rsbultje at gmail.com>
>> >> >> Reviewed-by: Michael Niedermayer <michael at niedermayer.cc>
>> >> >> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> >> >> ---
>> >> >>  libavformat/mp3dec.c | 13 +++++++++++--
>> >> >>  libavformat/rmdec.c  |  3 ++-
>> >> >>  2 files changed, 13 insertions(+), 3 deletions(-)
>> >> >>
>> >> >> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
>> >> >> index c76b21e..57ebcc8 100644
>> >> >> --- a/libavformat/mp3dec.c
>> >> >> +++ b/libavformat/mp3dec.c
>> >> >> @@ -372,11 +372,19 @@ static int mp3_read_header(AVFormatContext *s)
>> >> >>          uint32_t header, header2;
>> >> >>          int frame_size;
>> >> >>          if (!(i&1023))
>> >> >> -            ffio_ensure_seekback(s->pb, i + 1024 + 4);
>> >> >> +            if ((ret = ffio_ensure_seekback(s->pb, i + 1024 + 4)) < 0) {
>> >> >> +                av_log(s, AV_LOG_WARNING,
>> >> >> +                       "initial junk detection and skipping impossible due to: %s\n", av_err2str(ret));
>> >> >> +                goto skip_fail;
>> >> >> +            }
>> >> >>          frame_size = check(s->pb, off + i, &header);
>> >> >>          if (frame_size > 0) {
>> >> >>              avio_seek(s->pb, off, SEEK_SET);
>> >> >> -            ffio_ensure_seekback(s->pb, i + 1024 + frame_size + 4);
>> >> >> +            if ((ret = ffio_ensure_seekback(s->pb, i + 1024 + frame_size + 4)) < 0) {
>> >> >> +                av_log(s, AV_LOG_WARNING,
>> >> >> +                       "initial junk detection and skipping impossible due to: %s\n", av_err2str(ret));
>> >> >> +                goto skip_fail;
>> >> >> +            }
>> >> >>              if (check(s->pb, off + i + frame_size, &header2) >= 0 &&
>> >> >>                  (header & SAME_HEADER_MASK) == (header2 & SAME_HEADER_MASK))
>> >> >>              {
>> >> >> @@ -387,6 +395,7 @@ static int mp3_read_header(AVFormatContext *s)
>> >> >>          }
>> >> >>          avio_seek(s->pb, off, SEEK_SET);
>> >> >>      }
>> >> >> +skip_fail:
>> >> >>
>> >> >>      // the seek index is relative to the end of the xing vbr headers
>> >> >>      for (i = 0; i < st->nb_index_entries; i++)
>> >> >> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>> >> >> index 4e46a3d..470e227 100644
>> >> >> --- a/libavformat/rmdec.c
>> >> >> +++ b/libavformat/rmdec.c
>> >> >> @@ -618,7 +618,8 @@ static int rm_read_header(AVFormatContext *s)
>> >> >>              size = avio_rb32(pb);
>> >> >>              codec_pos = avio_tell(pb);
>> >> >>
>> >> >> -            ffio_ensure_seekback(pb, 4);
>> >> >> +            if ((ret = ffio_ensure_seekback(pb, 4)) < 0)
>> >> >> +                av_log(s, AV_LOG_WARNING, "seeking back impossible due to: %s\n", av_err2str(ret));
>> >> >>              v = avio_rb32(pb);
>> >> >>              if (v == MKBETAG('M', 'L', 'T', 'I')) {
>> >> >>                  ret = rm_read_multi(s, s->pb, st, mime);
>> >> >
>> >> > I maintain that this should not be done, because it makes the code
>> >> > verbose for no reason.
>> >>
>> >> Michael has pointed out that when seekback fails, one should really
>> >> break out of the junk detection loop in mp3dec. Your idea fails to
>> >> achieve it.
>> >
>> > Why? It's not really harmful. You could even argue that this will make
>> > the common case (skipping jumk and finding a valid frame in a seekable
>> > file) work in low memory situations, while your patch breaks it.
>
> ffio_ensure_seekback() never fails for seekable files
> it checks s->seekable and return 0 if true before attempting to
> allocate anything
>
>
>>
>> I assumed Michael had a good reason for it, but from what you say here
>> and my examination just now, I would at a first glance agree with you.
>> Ronald had concerns about repetition of messages, so if he (and
>> Michael) are fine with logging within ffio_ensure_seekback, then I
>> will log it there.
>
> as the code is written currently, if ffio_ensure_seekback() fails
> and that being ignored, the url_seek() to return to the begin will
> fail too quite likely while forward seeks will succeed.
> this would result in random amounts of data to be lost at the begin
> of the file
> it may print errors or warnings if such are added but nothing (like
> the applications return code) would indicate a problem, the user though
> could end with a output file that is missing the first few seconds of
> audio.
>
> if we break out on a ffio_ensure_seekback() failure then problems
> might still occur on files that have junk at te begin but files that
> do not have junk (which is what a valid file should look like)
> should work fine

Maybe, but many mp3's streamed over http have junk at the beginning,
so such broken files are not rare.

>
> all above is assuming iam not missing something ...
>
>
>> If that is not fine, then the patch is dropped.
>
> i think ignoring the error and printing a warning is still better
> than leaving things as they are

Definitely agreed. So I will repost with error logging purely within
ffio_ensure_seekback. That makes you, me, and wm4 on board with it.
Hopefully Ronald agrees with it as well...
[...]


More information about the ffmpeg-devel mailing list