Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Bugs
  • Category Database
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by sideral - 2010-11-04
Last edited by Jonathan Gordon - 2010-11-23

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

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.

Closed by  Jonathan Gordon
2010-11-23 00:13
Reason for closing:  Accepted
Additional comments about closing:  

r28644.

Jonathan Gordon commented on 2010-11-16 13:00

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?

sideral commented on 2010-11-17 00:24

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.

Jonathan Gordon commented on 2010-11-17 00:29

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

sideral commented on 2010-11-17 09:19

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.)

Jonathan Gordon commented on 2010-11-17 09:28

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)

sideral commented on 2010-11-17 21:48

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...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing