[FFmpeg-devel] [PATCHv2] avformat/mp3dec, rmdec: check return value of ffio_ensure_seekback
Michael Niedermayer
michael at niedermayer.cc
Thu Jan 7 23:34:15 CET 2016
On Wed, Jan 06, 2016 at 08:31:38PM -0800, Ganesh Ajjanagadde wrote:
> On Wed, Jan 6, 2016 at 1:25 PM, Michael Niedermayer
> <michael at niedermayer.cc> wrote:
> > On Tue, Jan 05, 2016 at 09:51:15PM -0800, Ganesh Ajjanagadde wrote:
> >> 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.
> >
> > what kind of junk is that ?
> > the mp3 code should resync on its own very quickly if its started
>
> Seems to resync very quickly. It was not really a problem, just gave
> as an illustration of junk occurence.
>
> > from a wrong byte offset
> > OTOH failure to seekback would loose several seconds of audio i
> > suspect
>
> Seems to be the case, if you are interested this is a sample stream I
> was referring to: http://stream.radiosai.net:8008.
>
> >
> > do you have an example file where (if the backward url_seek is
> > commented out the) the results are better with the loop executed
> > than without ?
>
> Sorry, I see only an avio_seek here in lavf/mp3dec. Can you please
> point out the exact line? Alternatively it may be easier for you to
> test with this link as you understand the code better.
i meant url_seek(), yes, any that moves the pointer backward could
fail after a ffio_ensure_seekback failure
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No great genius has ever existed without some touch of madness. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160107/750f1443/attachment.sig>
More information about the ffmpeg-devel
mailing list