[FFmpeg-devel] Fix for aviobuf.c::fill_buffer

[code bythepound]

Here is what seems to trigger this bug:

1) You have to be using an AVIOContext buffer size larger than
aviobuf.c::IO_BUFFER_SIZE (32768) or otherwise larger than
AVIOContext::max_packet_size.
2) Your AVIOContext can not provide a seek method (ie: not seekable)
3) You have to be using a file format that calls ffio_ensure_seekback()
(for instance, mpeg-ts) which resizes the AVIOContext buffer to some
internal large size.

1,2 above are false when using default programs like ffplay, ffmpeg, etc.

The following modification to the avio_reading example will trigger the
assert if you attempt to load an mpeg-ts file.  It keeps returning the same
initial block of data until the assert happens.  With a buffer size <=
32768 the assert won't be triggered.

I'm not really sure what the purpose of the max_buffer_size calculation is,
at the top of fill_buffer, so there may be a better fix than the previous
one I suggested.

Thanks.

diff --git a/doc/examples/avio_reading.c b/doc/examples/avio_reading.c
index 02474e9..3665d19 100644
--- a/doc/examples/avio_reading.c
+++ b/doc/examples/avio_reading.c
@@ -48,8 +48,8 @@ static int read_packet(void *opaque, uint8_t *buf, int
buf_size)

     /* copy internal buffer data to buf */
     memcpy(buf, bd->ptr, buf_size);
-    bd->ptr  += buf_size;
-    bd->size -= buf_size;
+    //bd->ptr  += buf_size;
+    //bd->size -= buf_size;

     return buf_size;
 }
@@ -59,7 +59,7 @@ int main(int argc, char *argv[])
     AVFormatContext *fmt_ctx = NULL;
     AVIOContext *avio_ctx = NULL;
     uint8_t *buffer = NULL, *avio_ctx_buffer = NULL;
-    size_t buffer_size, avio_ctx_buffer_size = 4096;
+    size_t buffer_size, avio_ctx_buffer_size = 65535
     char *input_filename = NULL;
     int ret = 0;
     struct buffer_data bd = { 0 };
@@ -116,6 +116,12 @@ int main(int argc, char *argv[])

     av_dump_format(fmt_ctx, 0, input_filename, 0);

+    AVPacket packet;
+    while( av_read_frame( fmt_ctx, &packet ) >= 0 )
+    {
+        av_free_packet( &packet );
+    }
+
 end:
     avformat_close_input(&fmt_ctx);
     /* note: the internal buffer could have changed, and be !=
avio_ctx_buffer */


On Thu, Jul 21, 2016 at 11:20 AM code bythepound <codebythepound at gmail.com>
wrote:

> I will see if I can put together a test case that exposes the problem.
> The comment for this block says "make buffer smaller in case it ended up
> large after probing" - I'm not 100% certain what that means.  I put prints
> inside this block and after the ffio_set_buf_size() call, this block is
> never executed again.  In my custom case I am streaming an MPEG.TS file and
> I don't know if this involves this extra "probing" or not.
>
> Even so, there is nothing in the code above that would limit 'len' to be
> greater than the size of s->orig_buffer_size, so the assert doesn't seem
> like the right way to handle this here.
>
> On Wed, Jul 20, 2016 at 4:41 PM Michael Niedermayer <michael at niedermayer.cc>
> wrote:
>
>> On Wed, Jul 20, 2016 at 03:59:39PM +0000, code bythepound wrote:
>> > My app seems to hit the av_assert0(len >= s->orig_buffer_size) (line
>> 535 in
>> > my sources) in aviobuf.c::fill_buffer.  FWIW I have registered a custom
>> > AVIOContext that will consistently not fill the entire buffer when the
>> read
>> > callback is called.
>> >
>> > The outcome is that 'len' in fill_buffer is decremented with each call
>> > until it is less than s->orig_buffer_size and will assert fail.  It
>> seems
>> > that instead of the assertion failure, the method should be:
>> >
>> > if( len >= s->orig_buffer_size )
>> >     len = s->orig_buffer_size
>> > // otherwise, len is < than orig_buffer_size, but is sized correctly to
>> > fill remainder of buffer.
>> >
>> > On the next iteration, the previous clause (dst == s->buffer) is
>> executed
>> > and the buffer is reset.  After that, this block is never executed
>> again.
>> >
>> > With the above fix in, my app seems to work fine.
>> >
>> > Since the fill_buffer code has been the same for many releases of
>> ffmpeg,
>> > I'm wondering how I could be the first to notice it, and if there is
>> > something else I could be doing wrong to cause this?
>>
>> is there some easy way to reproduce this ?
>> i tried
>> diff --git a/libavformat/file.c b/libavformat/file.c
>> index 264542a..529a975 100644
>> --- a/libavformat/file.c
>> +++ b/libavformat/file.c
>> @@ -108,6 +108,7 @@ static int file_read(URLContext *h, unsigned char
>> *buf, int size)
>>  {
>>      FileContext *c = h->priv_data;
>>      int ret;
>> +    size = FFMIN(size, size/5 + 1);
>>      size = FFMIN(size, c->blocksize);
>>      ret = read(c->fd, buf, size);
>>      if (ret == 0 && c->follow)
>>
>> but it doesnt trigger the assert
>>
>>
>> [...]
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> Modern terrorism, a quick summary: Need oil, start war with country that
>> has oil, kill hundread thousand in war. Let country fall into chaos,
>> be surprised about raise of fundamantalists. Drop more bombs, kill more
>> people, be surprised about them taking revenge and drop even more bombs
>> and strip your own citizens of their rights and freedoms. to be continued
>>
>