Rockbox

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?

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.

   0001-audio_stop_playback-Send... (0.7 KiB)

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

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

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)

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.

   0001-audio_stop_playback-Send... (1.1 KiB)