[PATCH] Fix incomplete loading of playlists of certain sizes
Hi, the bug this patches fixes appears when you have playlist files of a size larger than the buffer size used by playtreeparser.c but smaller than the buffer size used by stream.c. What happens is that the whole playlist file is read by stream.c in and put in the stream buffer by stream_fill_buffer() and stream->eof is set to true. However, playtreeparser.c only fills it's own buffer and then upon seeing stream->eof being true, treats the playlist as ended even tho the stream buffer still contains data. My solution is not to use stream->eof at all but instead react to stream_read() returning == 0 as a marker for when the stream has ended Regards, Joel Klinghed
On Thu, Jul 30, 2015 at 12:52:09AM +0200, Joel Klinghed wrote:
Hi,
the bug this patches fixes appears when you have playlist files of a size larger than the buffer size used by playtreeparser.c but smaller than the buffer size used by stream.c. What happens is that the whole playlist file is read by stream.c in and put in the stream buffer by stream_fill_buffer() and stream->eof is set to true. However, playtreeparser.c only fills it's own buffer and then upon seeing stream->eof being true, treats the playlist as ended even tho the stream buffer still contains data.
stream_fill_buffer does not set eof, only stream_read_internal does. stream_read_internal should only be called when the stream buffer is empty, and will only set eof if it cannot read any further data, i.e. the current position really is the end of the file. So something is wrong with your explanation. One potential issue is that seeking back from EOF will not unset EOF, that needs to be done separately, so maybe that is what causes the issue you see?
On Thu, 30 Jul 2015 10:28:56 +0200 Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
On Thu, Jul 30, 2015 at 12:52:09AM +0200, Joel Klinghed wrote:
Hi,
the bug this patches fixes appears when you have playlist files of a size larger than the buffer size used by playtreeparser.c but smaller than the buffer size used by stream.c. What happens is that the whole playlist file is read by stream.c in and put in the stream buffer by stream_fill_buffer() and stream->eof is set to true. However, playtreeparser.c only fills it's own buffer and then upon seeing stream->eof being true, treats the playlist as ended even tho the stream buffer still contains data.
stream_fill_buffer does not set eof, only stream_read_internal does. stream_read_internal should only be called when the stream buffer is empty, and will only set eof if it cannot read any further data, i.e. the current position really is the end of the file. So something is wrong with your explanation. One potential issue is that seeking back from EOF will not unset EOF, that needs to be done separately, so maybe that is what causes the issue you see?
My description was a bit lacking in details; playlist file is 2022 bytes. playtreeparser.c reads the first line in play_tree_parser_get_line(), calling stream_read(BUF_STEP=1024). 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. Back to play_tree_parser_get_line() that got the 1024 bytes it asked from stream_read(). The stream now has 998 bytes left in its buffer and eof set to true. Next call to play_tree_parser_get_line() will not call stream_read() as stream->eof is true. I should really have mentioned stream_fill_buffer() calling stream_read_internal() twice because the file is smaller than STREAM_BUFFER_MIN as the cause. Regards, Joel Klinghed
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);
On Fri, 31 Jul 2015 22:17:30 +0200 Reimar Döffinger <Reimar.Doeffinger@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
On Fri, Jul 31, 2015 at 11:39:23PM +0200, Joel Klinghed wrote:
I can confirm that your fix also fixes the problem I was having. A nice
Committed.
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 ;)
I agree but I have the same issue and lack time and test coverage to try to properly clean it up, at least for now.
participants (2)
-
Joel Klinghed -
Reimar Döffinger