On Tue, Jan 5, 2016 at 11:01 AM, wm4 <nfx...@googlemail.com> wrote:
> On Tue, 5 Jan 2016 08:32:02 -0800
> Ganesh Ajjanagadde <gajja...@mit.edu> wrote:
>
>> On Tue, Jan 5, 2016 at 5:29 AM, wm4 <nfx...@googlemail.com> wrote:
>> > On Mon, 4 Jan 2016 17:50:01 -0800
>> > Ganesh Ajjanagadde <gajjanaga...@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 <nfx...@googlemail.com>
>> >> Reviewed-by: Ronald S. Bultje <rsbul...@gmail.com>
>> >> Reviewed-by: Michael Niedermayer <mich...@niedermayer.cc>
>> >> Signed-off-by: Ganesh Ajjanagadde <gajjanaga...@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.
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.
If that is not fine, then the patch is dropped.
Thanks.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
