[FFmpeg-devel] [PATCH] HLS problem

[Martin Storsjö]

On Wed, 12 Jan 2011, Reimar D?ffinger wrote:

> On Wed, Jan 12, 2011 at 04:43:07PM +0100, Michael Niedermayer wrote:
> > On Wed, Dec 15, 2010 at 08:38:52PM +0100, Reimar D?ffinger wrote:
> > > On Wed, Dec 15, 2010 at 01:34:26PM +0200, aviad rozenhek wrote:
> > > > On Tue, Dec 14, 2010 at 12:44, aviad rozenhek <aviadr1 at gmail.com> wrote:
> > > > > On Thu, Dec 9, 2010 at 18:49, aviad rozenhek <aviadr1 at gmail.com> wrote:
> > > > >> On Wed, Dec 8, 2010 at 20:10, Reimar D?ffinger <Reimar.Doeffinger at gmx.de>wrote:
> > > > >>> On Wed, Dec 08, 2010 at 12:18:14PM +0200, Martin Storsj? wrote:
> > > > >>> > On Wed, 8 Dec 2010, aviad rozenhek wrote:
> > > > >>> >
> > > > >>> > > you are probably correct that the playback problems i'm seeing are
> > > > >>> due to
> > > > >>> > > buffering issues, but still the slew of aac decoding error logs is
> > > > >>> > > worrisome. do you think its a bug in the aac encoder, missing data,
> > > > >>> or
> > > > >>> > > what?!?
> > > > >>> > > my tests show that the aac error logs still happen when I let ffplay
> > > > >>> buffer
> > > > >>> > > for a long time before playing.
> > > > >>> >
> > > > >>> > Not sure what these come from. You could download all the mpegts
> > > > >>> segment
> > > > >>> > files from a playlist, concatenate them to a long .ts file, and play
> > > > >>> back
> > > > >>> > that, and you probably get the same errors. If you hand that file to
> > > > >>> > someone more knowledgeable on AAC and mpegts, they may be able to give
> > > > >>> a
> > > > >>> > better verdict on the issue.
> > > > >>>
> > > > >>> There is a roundup bug with patch for that.
> > > > >>> The issue is that some code uses url_read instead of url_read_complete,
> > > > >>> at least in some cases with http that means the code ends up using
> > > > >>> unintialized data and worse that uninitialized data will even appear
> > > > >>> in addition to valid data so not even the file offsets make any sense
> > > > >>> anymore (at least it looked like that to me, though the first part
> > > > >>> alone is bad enough).
> > > > >>>
> > > > >>>
> > > > what is the bug number? what's holding the commit?
> > > > We're pledging a 100$ bounty to fix this issue.
> > > 
> > > 
> > > Issue 2282.
> > > Patch is this
> > > Index: libavformat/aviobuf.c
> > > ===================================================================
> > > --- libavformat/aviobuf.c       (revision 25928)
> > > +++ libavformat/aviobuf.c       (working copy)
> > > @@ -603,7 +603,7 @@
> > >  
> > >      if (init_put_byte(*s, buffer, buffer_size,
> > >                        (h->flags & URL_WRONLY || h->flags & URL_RDWR), h,
> > > -                      url_read, url_write, url_seek) < 0) {
> > > +                      url_read_complete, url_write, url_seek) < 0) {
> > >          av_free(buffer);
> > >          av_freep(s);
> > >          return AVERROR(EIO);
> > > 
> > > 
> > > Needs someone to review and/or test it properly.
> > 
> > This patch is wrong, it would cause network streams to block and increase
> > latency unreasonable
> 
> Uh, none of the ByeIOContext functions I saw handle incomplete reads
> even remotely sanely, so your argument is it increases latency instead
> of not playing at all?

As far as I know, all ByteIOContext functions handle short reads just 
fine.
There are two functions reading data, get_buffer and fill_buffer. 
get_buffer only returns once the requested amount has been read, and
re-reads if the read function didn't return enough data. fill_buffer
will return once at least one byte has been read (while it requests
the full buffer from the underlying layer), so for networks, it will
return as soon as one packet is received. 

Other functions, such as get_le32() and similar all boil down to 
calling fill_buffer as many times as necessary to get the desired data.

It'd be interesting to know in detail why changing it to
url_read_complete does fix any bug.

// Martin