On Wed, Nov 18, 2015 at 8:26 AM, Ganesh Ajjanagadde <gajja...@mit.edu> wrote:
> On Wed, Nov 18, 2015 at 7:31 AM, wm4 <nfx...@gmail.com> wrote:
>> On Tue, 17 Nov 2015 17:39:31 -0500
>> 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.
>>>
>>> Signed-off-by: Ganesh Ajjanagadde <gajjanaga...@gmail.com>
>>> ---
>>> libavformat/mp3dec.c | 6 ++++--
>>> libavformat/rmdec.c | 3 ++-
>>> 2 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
>>> index 32ca00c..a14bccd 100644
>>> --- a/libavformat/mp3dec.c
>>> +++ b/libavformat/mp3dec.c
>>> @@ -380,11 +380,13 @@ 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, "ffio_ensure_seekback(): %s\n",
>>> av_err2str(ret));
>>> 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, "ffio_ensure_seekback(): %s\n",
>>> av_err2str(ret));
>>> if (check(s->pb, off + i + frame_size, &header2) >= 0 &&
>>> (header & SAME_HEADER_MASK) == (header2 &
>>> SAME_HEADER_MASK))
>>> {
>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>> index 4ec78ef..1cf0548 100644
>>> --- a/libavformat/rmdec.c
>>> +++ b/libavformat/rmdec.c
>>> @@ -576,7 +576,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, "ffio_ensure_seekback(): %s\n",
>>> av_err2str(ret));
>>> v = avio_rb32(pb);
>>> if (v == MKBETAG('M', 'L', 'T', 'I')) {
>>> int number_of_streams = avio_rb16(pb);
>>
>> So why do you not just add the message to the function itself?
>
> Because a client may not like the generic message, see e.g the message
> printed in avformat/apngdec. Handling of the error may vary from
> context to context, again see the apngdec example where the client
> does something else in addition to error printing.
>
>>
>> I also question the usefulness of the message. In such cases of OOM
>> everything goes to hell anyway. No reason to bloat the code with error
>> printing.
>
> Taking that logic a bit further, why bother returning ENOMEM? If the
> ENOMEM being returned by ensure_seekback is not checked, the
> information is just silently discarded or garbled with further errors
> down the road.
>
> Anyway, I do not consider it a bloat: the buffer allocated can be big
> (see in mp3dec > 1024 bytes). A user has a right to know when things
> "go to hell". Envisioning what may happen, ffmpeg crashes due to OOM,
> upon which a coredump takes place. That crash happens in a somewhat
> random location, since OOM handling is a heuristic and determining
> which process should be killed is an intractable problem. Thus at the
> time when the core dump is written out, it is not guaranteed that the
> context is the one where the allocation failed first. Then there are
> issues of overcommit where malloc may return a non-NULL pointer but
> OOM happens upon a dereference. Such issues are beyond the scope of
> error handling, as nothing can be done in such cases anyway.
>
> Warning the user when a reasonably large allocation failed in my view
> is not bloat. It is giving them as much information as we can. Also
> think of it from a debugging perspective: it can help us find places
> where the alloc size was unreasonable.
Another reason related to the randomness of the process killed for
OOM: ffmpeg/some other client may not be the process killed when OOM
occurs, and the malloc's here fail. Thus, seek back, something a user
does care about, fails, but without the error message, there is no
information whatsoever that such a thing failed and why. We should
make a "best effort" in such cases, especially since the alloc may be
non-trivial in size and has sufficient client impact.
Anyway, I by no means claim above patch is great, but I strongly
disagree with wm4 here.
>
>> _______________________________________________
>> 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
