Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#10377 - PCM buffer remainder is played under too many conditions

Attached to Project: Rockbox
Opened by Jeffrey Goode (Blue_Dude) - Wednesday, 24 June 2009, 05:38 GMT+1
Last edited by Thomas Martitz (kugel.) - Monday, 29 June 2009, 21:21 GMT+1
Task Type Bugs
Category Music playback
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Version 3.3
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

The pcmbuf_play_remainder() routine in pcmbuf.c was intended to ensure that the last samples were played out of the buffer at the end of the playlist. This was done from a call in playback.c, but the placement of that call causes pcmbuf_play_remainder() to be called for all stops. This caused a subtle bug in another patch that could crash the player under certain conditions.

This patch changes the placement of the call so that it's only invoked at the end of the playlist as intended.
   playback.patch (0.7 KiB) 
 apps/playback.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

This task depends upon

Closed by  Thomas Martitz (kugel.)
Monday, 29 June 2009, 21:21 GMT+1
Reason for closing:  Accepted
Additional comments about closing:  Commit the last patch in r21569. Thanks.
Comment by Jeffrey Goode (Blue_Dude) - Wednesday, 24 June 2009, 17:25 GMT+1
Here's a better patch that does the same thing.
   playback.patch (0.5 KiB) 
 apps/playback.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comment by Thomas Martitz (kugel.) - Thursday, 25 June 2009, 12:02 GMT+1
Can you explain the actual bug and change please? what's the difference between "playing" and "!paused" ?
Comment by Jeffrey Goode (Blue_Dude) - Thursday, 25 June 2009, 14:55 GMT+1
Yeah, this one was a little weird. The problem was in the changes I made to the pcmbuf_play_remainder() routine in another patch. I added a pcmbuf_request_buffer line to make space at the end of the PCM buffer for the contents of the attack buffer. But during a manual stop, this line would go into an infinite loop waiting on buffer space that wasn't going to appear, and occasionally crash. So in order to discriminate between a manual stop and playing out to the end I changed the condition from "playing" to "!paused". A manual stop at this stage is actually paused but still playing, but it doesn't need the samples because it's not ever going to play them. But if playing to the very end, it's not paused and it needs them, so it gets them.

This change as a standalone patch has no ill effects and actually prevents an unneeded call. If it's not used for now, nothing bad will happen. Consider it a minor cleanup.
Comment by Jeffrey Goode (Blue_Dude) - Friday, 26 June 2009, 20:41 GMT+1
Same patch with code commentary.
   playback.patch (0.7 KiB) 
 apps/playback.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comment by Jeffrey Goode (Blue_Dude) - Friday, 26 June 2009, 20:46 GMT+1
A short hedge against something strange happening. This routine should never be called if (!playing) and (!paused), but just in case...
   playback.patch (0.7 KiB) 
 apps/playback.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comment by Jeffrey Goode (Blue_Dude) - Friday, 26 June 2009, 20:56 GMT+1
Cleaner code.
   playback.patch (0.7 KiB) 
 apps/playback.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Loading...