Rockbox

Tasklist

FS#11721 - Runtime statistics data not gathered when playback stops

Attached to Project: Rockbox
Opened by sideral (sideral) - Thursday, 04 November 2010, 16:59 GMT
Last edited by Jonathan Gordon (jdgordon) - Tuesday, 23 November 2010, 00:13 GMT
Task Type Bugs
Category Database
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Runtime data gathering in tagtree_buffer_event / tagtree_track_finish_event (tagtree.cc) relies on the PLAYBACK_EVENT_TRACK_BUFFER and PLAYBACK_EVENT_TRACK_FINISH events. The latter event is generated only on a song change, but not when playback stops (user stops playback, poweroff, or end of playlist). This leads to statistics not being updated and old statistics data being loaded again next time a previously stopped song is played.

To reproduce, turn on logf and observe the “be:” (buffer event) and “ube:” (unbuffer event) messages in the debug log. Stopping playback does not generate a “ube:” message.

The attached patch fixes this behavior by generating the PLAYBACK_EVENT_TRACK_FINISH event from the audio_stop_playback function as well.
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Tuesday, 23 November 2010, 00:13 GMT
Reason for closing:  Accepted
Additional comments about closing:  r28644.
Comment by Jonathan Gordon (jdgordon) - Tuesday, 16 November 2010, 13:00 GMT
I'm not entirely sure this is correct.
thistrack_id3 in the last few seconds of a track is actually the *next* track so if playback is manually stopped then I think the wrong track will get used by the callback. Also at end of playlist that could be garbage also I think?
Comment by sideral (sideral) - Wednesday, 17 November 2010, 00:24 GMT
An excellent catch! This indeed seems to be a bug, although in practice nothing bad will happen.

The problem is that when thistrack_id3 has already been swapped for the next track (or zeroed out at end of playlist), PLAYBACK_EVENT_TRACK_FINISH has already been generated. In this case, my patch generates the event again, for a wrong or zeroed out thistrack_id3.

The reason that typically nothing bad happens is that the consumer of the event, tagtree_track_finish_event, guards against being called with an unplayed or nonexisting track and will do nothing. However, I observed at least one case where stale data was written out for the new track before it had begun playback.

Thus, the fix is not just a matter of generating PLAYBACK_EVENT_TRACK_FINISH with the right id3 instance. Rather, we should not generate the event at all if it had already been generated for the track being stopped.

I have attached a new patch that implements a better fix.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 17 November 2010, 00:29 GMT
That still wont work with end of playlist though, I think? isnt id3 == NULL in that case?
Comment by sideral (sideral) - Wednesday, 17 November 2010, 09:19 GMT
Strike that last patch for now; I'm afraid it only works in the simulator. :-(

I need to find a robust method for determining that the PLAYBACK_EVENT_TRACK_FINISH event has already been generated when the track is stopped. Perhaps someone can recommend a good method?

(In response to your question, I was going to say that AFAICT thistrack_id3 is never NULL and thus everything would work out nicely, but it seems that id3 is never == thistrack_id3 on real devices.)
Comment by Jonathan Gordon (jdgordon) - Wednesday, 17 November 2010, 09:28 GMT
almost noone active knows playback.c so you're pretty much on your own :(
"but it seems that id3 is never == thistrack_id3 on real devices." <- really? that seems odd (or at least should be the same behavior as the sim)
Comment by sideral (sideral) - Wednesday, 17 November 2010, 21:48 GMT
Oops – I was thoroughly confused by the existence of two versions of audio_current_track(). My editor jumped to the version in mpeg.c first, but it's the wrong one… :-(

So, strike that last confused comment of mine instead – I actually do stand by my last patch. :-) (I'm attaching a new version [v3] of the patch anyway because the previous one [v2] had a typo in the comment.)

> That still wont work with end of playlist though, I think? isnt id3
> == NULL in that case?

As I said: thistrack_id3 is never NULL (it always points to either mp3entry_buf[0] or mp3entry_buf[1]) and thus everything works out nicely: If id3 == NULL, the comparison fails and no PLAYBACK_EVENT_TRACK_FINISH event will be generated. I've verified for both the simulator and for a real device that the final call to audio_stop_playback (both when triggered manually or automatically) does not generate a spurious second PLAYBACK_EVENT_TRACK_FINISH event. In this case (end of playlist), the correct data is saved through the PLAYBACK_EVENT_TRACK_FINISH event generated by audio_check_new_track.

Loading...