This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#9795 - some playback rework
Attached to Project:
Rockbox
Opened by Jonathan Gordon (jdgordon) - Tuesday, 13 January 2009, 13:55 GMT+2
Last edited by Jonathan Gordon (jdgordon) - Monday, 06 April 2009, 02:40 GMT+2
Opened by Jonathan Gordon (jdgordon) - Tuesday, 13 January 2009, 13:55 GMT+2
Last edited by Jonathan Gordon (jdgordon) - Monday, 06 April 2009, 02:40 GMT+2
|
DetailsThis patch aims to clean things up a bit between the wps and playback, (also in playback itself.)
This changes the wps to use the track changed event to redraw and reload id3 infos and also hopefully removes some mess in playback which was needed for the few second transition period between the codec finishing a track and pcm finishing the same track. this should only effect swcodec, but hwcodec needs testing also to make sure nothing breaks. let me know if you find any wierdness (which isnt in svn) 
playback_changes.1.patch
(16.3 KiB)
apps/gui/gwps-common.c | 79 +++++++++++++--------------- apps/playback.c | 134 +++++++++++++++++++++++++++---------------------- 2 files changed, 112 insertions(+), 101 deletions(-) |
This task depends upon
Closed by Jonathan Gordon (jdgordon)
Monday, 06 April 2009, 02:40 GMT+2
Reason for closing: Accepted
Additional comments about closing: Kugel: feel free to cleanup the wps changes from your patch in svn.. but I disagree with your placement of the event_Add lines.... and generally dont agree with the need to remove the event
Monday, 06 April 2009, 02:40 GMT+2
Reason for closing: Accepted
Additional comments about closing: Kugel: feel free to cleanup the wps changes from your patch in svn.. but I disagree with your placement of the event_Add lines.... and generally dont agree with the need to remove the event
CC apps/playback.c
/home/user/rockbox/apps/playback.c: In function 'set_filebuf_watermark':
/home/user/rockbox/apps/playback.c:861: error: 'curtrack_id3' undeclared (first use in this function)
/home/user/rockbox/apps/playback.c:861: error: (Each undeclared identifier is reported only once
/home/user/rockbox/apps/playback.c:861: error: for each function it appears in.)
make: *** [/home/user/rockbox/build/apps/playback.o] Error 1
The next-track info is still missing when the next track isn't yet buffered. It appears when the track gets buffered.
(But if i understand the comment in playback.c (line 645) correctly this is expected.)
I once got a "codec failure" in the middle of a track during rebuffering and it skipped to the next track. But i haven't found a way to reproduce it.
When you select a song from the playlist viewer then the WPS is shown with the menu backdrop and it doesn't update. When pressing PLAY or LEFT it returns to the "real" WPS. When pressing UP or DOWN in that situation the playlist viewer is shown again.
With a clean build selecting a song will continue showing the playlist viewer.
The same happens when resuming playback from within a plugin (for example solitaire): A broken WPS is displayed even though you are still in the Audio Playback menu of the plugin.
FS#9796for details.Apparently I'm a bit of a dill :p I got events and threads mixed up... The WPS is being drawn inside the playback thread which is a BIG no-no! so this needs fixing
Not tried this patch yet, but I get the random chars ("heiroglyphs") with your cuesheet patch only.
JayKay, no i mean did the patching work? it most likely failed there which is why it couldnt compile. try with a clean svn
So, would you like me to try just this patch, or this patch + the cuesheet one? I'll apply whatever you prefer tonight.
The next-track info is sometimes wrong and shows the info from a track later in the playlist.
Reproduction: start playback and when buffering finishes enter either the quickscreen or the WPS context menu. When returning to the WPS the next-track info is wrong (it seems to show the info of the next unbuffered track). If you then enter the browser or the mainmenu and return to the WPS then the next-track info is correct again.
make: *** No rule to make target `/home/user/rockbox/apps/codecs/lib/misc.h', needed by `/home/user/rockbox/build/apps/codecs/lib/mdct2.o'. Stop.
Have you tried make clean, then make?
edit: reuploaded without the CF clock patch
I think this is working well on swcodec, not enough hwcodec has been done so I'm not sure what to do...
There are a couiple of issues with this on my H300:
1) Skipping to an unbuffered track displays garbage for ID3 (easy to repro, just skip backwards and forwards quickly - e.g. between tracks 2 and 3).
2) Skipping through an album, the UI is much less responsive than SVN (e.g. try repeatedly skipping back from track 8 to track 2).
I should add, this is is with JUST this patch.
as for ui responsiveness.. does it actually take longer to change tracks or just to redraw the screen? I'm guessing svn redraws as soon as the skip button is pressed and again when the track loads whereas this patch only redraws when the track loads (can be fixed if this is the case)
edit: (patch version 7a) changed the patch so its much more responsive now (or at least looks like it is....) This also hopefully gets rid of all possible garbage.. but I dont really like it :p need to do a bit of cleaning up... let me know if garbage comes back please
not much changed in this version apart from cleanups. Previous version could crash the sim pretty easily but havnt been able to with this version (yet :D )
Part of the cleanup was changing how the ipod acccessory stuff gets notified about track change, bassically the same patch as the wps, but I dont have an ipod so cant test if its still working... also needs more hwcodec testing.
H300 uisim r19798.
I think i've found the problem with the playlist position not being updated... its because of the order of what happens when you press next track in the wps. The playlist position is only redrawn in a full redraw which we are trying to limit to once per track (when the track actually starts) which works for automatic changes, but manual skipping doesn't work (even with a forced redraw after pressing the button) because there could be a delay between the first forced redraw (where at this point playback is waiting for the disk to spinup/track to get buffered) and the playlist position hasnt been updated yet (it doesn't until the track gets passed to the codec)
hmm.. just put svn onto my h300 and it seems the playlist position isnt really updated much better there than in the patch so i dont think this is much to worry about
what needs testing:
HWCODEC!
someone with an ipod acessory which relies on the track change event needs to make sure this still works...
1. When playing FLAC files sometimes the next-track info is missing or shows the metadata of the current song. The info appears/becomes correct when pressing any of the buttons or when rebuffering happens.
For reproduction reset settings, and then play a FLAC album and slowly skip forwards until the problem occurs. (With album art it seems to happen more often.)
2. When playing "normal" sized files it can happen that both next- and current-track info show the metadata of the next track.
For reproduction reset settings, and then play an mp3 album and as soon as the next-track info updates** quickly skip to the next track.
This seems to be best reproducible with mp3 and mpc but also happens with ogg and aac.
**there is sometimes old information shown for a few seconds, so wait for the correct info to show up.
It would still be nice if this could be fixed too. :)
H300 with r19869 (patch version 8b).
PaulJam, 1 looks likes its fixed, I'm not sure how big an issue 2 is... thats not really something you would do often?
I havnt checked out that runtime gathering problem yet...
http://www.rockbox.org/irc/log-20090122#12:05:46 is the link the the hwcodec related problem which needs to be fixed by someone who has a hwcodec target :/
/* We're in a track transition. The next track for the WPS is the one
currently being decoded. */
thistrack_id3 should be returned instead of othertrack_id3 in an automatic track change. This makes "Next Track:" info on the WPS correct until end of track. It also agrees with:
*othertrack_id3; /* prev track during track-change-transition, or end of playlist,
* next track otherwise */
I'm not submitting a patch because I don't understand this code enough. In particular, what about other kinds of track changes?
bit of a note to self: it looks like PLAYBACK_EVENT_TRACK_FINISH is still being sent when the codec finishes with the track and NOT when its actually finished playing whereas PLAYBACK_EVENT_TRACK_CHANGE happens when PCM is actually finished with the last track (as long as there is another track to play)... so I'm thinking FINISH should be removed and CHANGE renamed and fixed so it happens at the playlist end also. The only time I can think of where there would be a difference is in the WPS where a full redraw happens on track change, but that can be handled easily i think.
That can maybe wait untill after this is finished for good... I think the change is large enough as it is.
ninja edit: hmm... maybe not... _FINISH sends the id3 info of the just finished track and _CHANGE sends the info of the starting track...
right, test please, I think its ready to go in
I patched on top of r20352 and tested on the Gigabeat S30.
The problem that sometimes the next tracks metadata is shown for the current track after skipping is still present in the v10 patch.
Gather runtime seems to work now.
Another thing is that skipping to a track that uses another codec often results in codec failures (when skipping from an AAC or MPC to MP3 it often skips the next 10-20 tracks).
Player: H300 with default settings except for shuffle:on.
Hope you can find out what causes this little problem.
Thanks!
Divide by 0
at 03D80D94
Hope this helps.
can you attach your rockbox.map file in the build dir so we know for sure where that error is?
also makes some more next track tags static
edit: i cant make it "codec failed" between flac/ogg/mp3 by fast forwarding to before the end of one track and waiting for it to change over, i've got it on shuffle now so hopefully ill notice any errors it comes up with
Anyone have any ideas why keith seems to be the only one getting codec fails with this patch?
spent a few min looking into the problem where doing lots of skipping causes wierdness in the WPS (current and next track both showing the same info, or the track which starts playing wont be the one the filename showed when you stopped clicking next).
pixelma said that she'd seen this with SVN which makes my tihnking more reasonable...
I have a feeling this is a thread-sync problem. This patch must change something which makes it show up more than in svn.
This is how i understand things:
1) skip is pressed in the WPS (main thread) -> audio_skip() eventually posts Q_AUDIO_SKIP to the audio thread (non blocking)
2) WPS does an update..
3) playback thread eventually sets up the vars (ci.next_track and wps_offset) so it looks like the skip is happening
4) codec thread sees ci.next_track is != 0 so checks if the next track is for this codec or not. It calls (if the codec is the same) codec_load_next_track() which sends Q_AUDIO_CHECK_NEW_TRACK back to playback thread (blocking)
5) audio_check_new_track() does some magic and eventually calls playlist_next() which should bring the 3 threads back together (ci.next_track and wps_offset anyway)
so, I'm guessing that recipe is the problem. And because this is in svn also I'm inclined to ignore the issue (file it as a bug anyway) and hope it gets solved later. I think that whole mechanism is too convoluted. We need to have a discussion on how track changing is supposed to be done (and more generally how playback, buffering, codec and playlist all interact together).
I'll write up something for the ml...
/* Update the elapsed-time indicator */
samplesdone=fc.samplenumber+fc.blocksize;
elapsedtime=(samplesdone*10)/(ci->id3->frequency/100);
The codec is accessing ci->id3 after the struct it points to has been zeroed.
added some code which will hopefully sotp that div0 but i wonder if that 2nd yield is even needed?
edit: talk in IRC looks like that extra yield should be safe... perrikwp are you getting that div0 with the .11(a) patch? I think nico's suggestion fixed that problem
attached is a logf dump of the track skip.. you can see the codec error, but I dont know how useful this is :(
him and I (mostly he) spent a fair while figuring out what the issue was.... fun starts about http://www.rockbox.org/irc/log-20090401#06:04:33
for those who cant handle the horrow of the logs...
[23:20:38] <JdGordon> so user presses next, wps calls audio_next and then does a full redraw (which includes audio_current_track)... but because playback has already started preparing for the next track the read idx has moved so audio_current_track thins we have moved on already and does the bufread causin both id3 structs to be the same
its about midnight here so im posting this so i dont forget, but also for anyone who wants to tackle this issue.... there is a very small amount of time where playback/ui is in a nasty state (I think much smaller than svn though) which needs to be safeguarded. the *simple* (and possibly not completly correct) fix is to return thertrack_id3 at playback.c:582, but It feels/looks wrong to me so a proper fix needs to be done/investigated.
I'm really glad we finally got that last bug which means this can be commited soon maybe... so if anyone wants to comment on the dev-ml thread re playback and how we want to fix it... be my guest :)
side not that dreamlayers found.... the copy_mp3entry() call around line 2040 is wrong... why is it being done (its in svn also), and why is it copying the struct which is cleared 50 lines up? should that be othertrack_id3 instead?
If the answer is 'yes' then this bug I reported, http://www.rockbox.org/tracker/task/10085, should interest you.
To summarize, the remote LCD wps isn't fully updated on track change.
I tried with v12 patch, and nothing changed regarding this bug.
I'm confident this version is good to go, there is that big FIXME comment but that could just be thistrack_id3 instead of othertrack_id3, or completely useless code....
There's no point in wasting cpu time in unsed code paths while the the user is not in the wps, really. And also remember that the number of events is limited. Removing the wps ones properly frees up slots for potential other events.
1) the time spent in the 2 events when they are unneeded (in a usual session) would be tiny compared to the time spent enabling/disabling the events.
2) that code is actually 100% safe
3) the total number of events is arbitrarily set based on the maximum that would ever be required, same for max threads and other OS buffer sizes... if another slot is needed it will be added.
Adding and removing events is a matter of assigning a value to a variable. This is tiny. Running two functions called via a function pointer is not (compared to assigning a variable).
What is "usual session"? That doesn't mean anything without explanation.
Not removing dead code is bad practice too. You shouldn't even think about it.
100% is still a dangerous statement.
the usual case is starting playback and leaving it in the wps.
what dead code?
I'll give this a good testing today, but some superficial suggestions:
- Why rename prevtrackid3 to othertrackid3? I though the old name was clearer (plus it makes the patch smaller!)
- You have a C++ comment in there..."//#define LOGF_ENABLE"
- Looks like you should bump the plugin API version (scrapping audio_has_changed_track)
But if you leave the wps just to show the main menu (which saves cpu time and battery) or running some plugins, for several minutes or hours is a use-case too (my use-case for example). And then you remove once, and save a dozens of calls to the callback functions.
I agree with kugel - it's good to tidy up...
</headaboveparapet>
fixed and fixed (it must have been bumped during a sync and i missed it :p )
also, this should fix the issue in
FS#10085We need to regard track transition as a large time, not a small time - it's thousands of milliseconds and has caused plenty of trouble ;)
>> that othertrack is actually the next track, not the previous track
Hmm, this doesn't seem obvious - I'll take a look at the code later. If you pick a single viewpoint (i.e. "codec transition" vs. "WPS transition" ) and name your module-level variables based on this then this it should come out in the wash. I don't think that the "codec transition" viewpoint should never be used/needed outside of playback.c and the codecs....
Variables in playback are from playbacks POV, 99% of the time othertrack_id3 is pointing to the next track. during the 2s transition it will point to the current track while thistrack_id3 points to the one the codec is about to start... ok so yeah it is confusing, but I dont think prevtrack is any better..
Ah crap.. ok, just use 13 for the time being and manually bump the plugin api.
BTW: I'm thinking the event could also be sent to the id3 viewer to update on track change too.
I also fused partial_update and update_track to just update. update_track is pretty much not needed anymore, since refreshing id3 info is done through the event callback. So, we just want to force updating on timeout or if AB/cuesheet markers want to be added/removed (not on manual track skip anymore).
I moved the initial id3 assigning and event adding to restoration part of the loop, and set restore to true initially. This way the events and id3 will be properly re-added/-assigned on re-entering the wps.
fyi, events are not sent TO anything they are received BY something... and yeah the id3 screen could receive the track changed event but 1) not in this patch and b) I'm not sure we want that anyway (without at least asking if the user wants to see the new tracks info)
edit. few=fix
was your commit yesterday to fix the skip length issue with this patch?