[MPlayer-dev-eng] [ml] Re: [PATCH] Fix incomplete loading of playlists of certain sizes

[Joel Klinghed]

On Fri, 31 Jul 2015 22:17:30 +0200
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> On Fri, Jul 31, 2015 at 12:43:14AM +0200, Joel Klinghed wrote:
> > stream_read() ends up calling stream_fill_buffer() as the stream
> > buffer is empty. stream_fill_buffer() calls
> > stream_read_internal(STREAM_BUFFER_SIZE=4096) which returns 2022
> > bytes read.
> > stream_fill_buffer() then calls
> > stream_read_internal(STREAM_BUFFER_MIN=2048) as the length returned
> > by the first call to stream_read_internal returned less than
> > STREAM_BUFFER_MIN. The second call to stream_read_internal sets
> > stream->eof to true as there is no data left in the file.
> > stream_fill_buffer() now returns as the last call
> > to stream_read_internal() returned 0.
> 
> There's the bug.
> EOF should only be triggered if a read external to the
> stream layer triggered a buffer fill and we could not
> read _any_ data to fill the buffer.
> This was a case missed by a bugfix for something else (r37310).
> Your patch may well fix the playlist parser but leaves
> all other users of the stream layer affected still.
> Something like below patch should hopefully fix it properly
> (not sure if it's exactly the best way though):
> -- a/stream/stream.c
> +++ b/stream/stream.c
> @@ -366,6 +366,9 @@ int stream_fill_buffer(stream_t *s){
>        break;
>      s->buf_len += len;
>    }
> +  // since the first read succeeded we are
> +  // definitely not at EOF yet
> +  s->eof = 0;
>  //  printf("[%d]",len);fflush(stdout);
>    if (s->capture_file)
>      stream_capture_do(s);

I can confirm that your fix also fixes the problem I was having. A nice
fix would probably be to control s->eof in one single place
(stream_fill_buffer seems like a good place) but I'm not
feeling that brave after greeping for s->eof assignments ;)

Regards, Joel Klinghed