Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Music playback
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Version 3.1
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by jdgordon - 2009-01-13
Last edited by jdgordon - 2009-04-06

FS#9795 - some playback rework

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

Closed by  jdgordon
2009-04-06 00:40
Reason for closing:  Accepted
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

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

small fix to hopefully fix the next track info not getting updated

i get a few errors:

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

With H300 r19757 (patch version 2):

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.

The last point (resuming playback from a plugin) seems to be somehow broken in a clean build too. See  FS#9796  for details.

JayKay: did the patch actually work? I’m guessing it didnt because line 861 has no mention of curtrack_id3 for me.. in fact.. 861 is an empty line.

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

new version fixes the naughty drawing in the wrong thread problem… Has anyone seen random chars instead of the proper track info? I’ve seen it a few times in the sim and once on my h300 but no idea how to reproduce :/

Hi Jd,

Not tried this patch yet, but I get the random chars (”heiroglyphs”) with your cuesheet patch only.

jdgordon: no. it stopped compiling after this error, so i couldnt try. ill test v3 in the afternoon

Steve, yeah, it makes sense to get it in that patch, the problem there is that playback doesnt make any attempt to make the mp3entry sturct valid for the lifetime of the track, this is why i did this patch.

JayKay, no i mean did the patching work? it most likely failed there which is why it couldnt compile. try with a clean svn

v3 will crash when the playlist finishes…. this version fixes that mistake… also this is the first one which hopefully will work on hwcodec if someone can test that

Hi Jd,

So, would you like me to try just this patch, or this patch + the cuesheet one? I’ll apply whatever you prefer tonight.

this one is the priority… ill sync cuesheet up to this once this is in svn, or closer… having trouble getting hwcodec testers interested… I got through 3 cds in the e200-sim and havnt seen any problems on my h300 so this might be ready

H300 with r19771, patch version 4:

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.

now playback.c compiles fine, but ill get an error while compiling apps/codecs/lib/codeclib.c

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.

Hi JayKay,

Have you tried make clean, then make?

patches, compiles and works without any complaints, assuming that “working” means that nothing noticable changed.

new version… this one reimplements some of the current behaviour by grabbing the id3 info of the first unbuffered track so its avilable to the wps… this might mean that the next track info avilaable event is useless….

edit: reuploaded without the CF clock patch

quick update… fix up some comments and move the wps event handlers into gwps.c where it possibly makes more sense…

I think this is working well on swcodec, not enough hwcodec has been done so I’m not sure what to do…

Hi Jonathan,

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.

ok, thanks

its harder to get garbage for me (guess its partly hdd vs CF speed) so if you could try out this patch and tell me if things get better there… it needs cleanup but if it does work a clean patch should be quick to fix

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

patch is starting to get bigger than I’d like… :/

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.

Version 8 of the patch breaks alternating sublines in the WPS. It always shows the first subline.
H300 uisim r19798.

No heiroglyphics with v8, and the ID3 info is displayed more promptly when skipping , but the actual playlist index (i.e. X of Y) is not updated as you skip through unbuffered tracks.

this one should fix the sublines…

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

sync and bump the plugin API version… sublines are working, cuesheet is hopefully working (done minimal testing in the sim), regular playback works, as does brutal track changing (although the current playlist position takes a tiny bit longer than svn to update.. not really much that can be done about that :( )

what needs testing:
HWCODEC!
someone with an ipod acessory which relies on the track change event needs to make sure this still works…

on e200 i didnt see anything which i cant see in svn.

There seem to be still some problems with the version 8b of the patch (h300 with r19818)

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.

I just noticed, that the first problem in the previous comment happens in SVN too (though a little bit different: the correct info only shows after leaving and reentering the WPS).
It would still be nice if this could be fixed too. :)

Another issue with this patch is, that the runtime data in the database (play count, recently played, etc.) doesn’t get updated even though the “Gather Runtime Data” option is enabled.

H300 with r19869 (patch version 8b).

resync.

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 :/

I think in audio_next_track(), after
/* 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?

I havnt looked at the code in ages, but yes I think you are correct… it does at least make the code agree with the comments… does it change anything with that switched?

minor update… fix gather runtime data and Boris’ bug..

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…

hwcodec bug fixed :D

right, test please, I think its ready to go in

Using this patch, every now and then a “Codec Failure” splash appears after skipping from one track to another track, but the track plays anyways. After a while, the “codec failure” splash disappears. I checked the codec files and they are from the same build, so the codec files are not the problem.

I patched on top of r20352 and tested on the Gigabeat S30.

is it easily repeatable? which codecs? you’re 100% sure you did a full rebuild?

Patch version 10 with r20354:
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.

As PaulJam also reported, the codec failure happens when switching to one type of codec to another. In my case, that track still plays though. I did a make veryclean after patching the code, but before building as I always do testing patches.

Hope you can find out what causes this little problem.

Thanks!

Oops made a little mistake, it actually does skip songs when the codec failure happens. I also got a Divide by 0 error but was not able to catch the address, because the Gigabeat S turned off before I could copy it down. This happened while skipping back and forward between tracks trying to reproduce the first bug.

Sorry to post so much. I finally caught the address for the error:

Divide by 0
at 03D80D94

Hope this helps.

no worries :)
can you attach your rockbox.map file in the build dir so we know for sure where that error is?

Here’s the map file.

   rockbox.map (221.7 KiB)

bah, that address is in the codec buffer… can you dinf the codec’s .map file (the one that actually crashed if you remember) and upload that one please?

I don’t remember what codec it crashed on, I only use MP3, FLAC, and OGG Vorbis. Would it help to at least upload those three corresponding map files?

not really… i’m not at work anymore so ill investigate tonight

woops, removed one too many lines last night.. this version makes the WPS draw when the next track starts again.
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

Ok, I have tested the new patch version 10a, no noticeable change in behavior. The codec failure is still there and the divide by zero error still pops up every now and then. It happens when restarting a FLAC file from the beginning. I have attached the corresponding codec map file to this message.

   flac.map (13.8 KiB)

do nico’s suggestion from http://www.rockbox.org/irc/log-20090322#18:58:03. The 2nd suggestion looks like the comment was outdated so fix that also.

Anyone have any ideas why keith seems to be the only one getting codec fails with this patch?

(disclaimer: this whole post might be wrong….)

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…

Regarding that divide by zero. It’s this in flac.c codec_main():

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

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

had a bit of playing with the sim today and had no audio problems (that I could tell… had it running in the background)… but playing around I did see some codec failures when swapping codecs FROM wma… anyone have any ideas?
attached is a logf dump of the track skip.. you can see the codec error, but I dont know how useful this is :(

all glory to dreamlayers for figuring out what the problem 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?

update with a first-go fix for the codec-fail problem… This works but I’m not convinced this is the best method (but im too tired to work out a good one)

Is the remote LCD wps in the scope of this patch ?

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.

yes, it is out of scope but we were hoping the WPS changes in this patch would fix it anyway

ignore that last patch… this one works the same as svn, fast track skipping shows the correct filename and amazingly the track you stop at is the one which starts playing :)

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

You should remove the events when you leave the wps, shouldn’t you?

usually yes… but the code is 100% safe while the wps is not displayed so it might be a waste of effort to make sure to remove and reenable the event every time

“the code is 100% safe” is a dangerous statement (especially when you say it :D ). Anyway, that’s why I eased wps leaving and re-entering (for the coder). You effectively need to enable add the event in 1 place, and remove them in 1 place too.

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.

this is going on a tangent but I’ll say this…
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.

You have looked at the event code, haven’t you?

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.

adding and removing an event means at the worst case doing 3 loops of size MAX_SYS_EVENTS, which yes is small, but if that happens every time the wps is exited/reopened itsw wasteful.
the usual case is starting playback and leaving it in the wps.
what dead code?

Hi Jonathan,

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)

If the usual case is never leaving the wps, then removing won’t ever be done. This use-case isn’t even touched by removing events.

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.

<headaboveparapet>
I agree with kugel - it’s good to tidy up… </headaboveparapet>

there is now a small time when the track is in transition that othertrack is actually the next track, not the previous track, I tihnk this was part of the origional problem in that thistrack actually is the last track (from playback’s POV iirc) and it got confusing. I tihnk the name change makes it more understandable… fixed and fixed (it must have been bumped during a sync and i missed it :p )

also, this should fix the issue in  FS#10085 

ah nuts to you both

there is now a small time when the track is in transition

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

13a doesn’t appear to update the remote WPS ID3 info on the H300 sim, FWIW.

sorry, (getting late here)… that small time used to be larger (I think). The actual trasition time takes about 2s between the codec finishing the track and PCM finishing the same track. during these 2s the UI could do wierd things and mess things up. What I’ve tried to do is make the gap (from the UI’s pov) much smaller… there is excat moments when the UI can cause nastiness and its tiny in comparison to svn (could be talking nonesense atm though… past midnight).

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.

FYI - 13 doesn’t either - might be a sim thing, will retest with SVN later.

SVN also fails to update the sim’s remote LCD, so don’t worry about that too much. I don’t have a real LCD remote though.

The wps_state.do_full_update = false; line should be after both gui_wps_update has been called for both screens (i.e. out of the FOR_NB_SCREENS(i) loop). In 13/13a, full_update is still (similar to svn) set false after the first screen has called it.

Also, add_event should happen in the restore part, else the events won’t get re-added after the wps has been left once.

This fixes my aforementioned problems, as well as missing id3 info if track changed happened while not in the wps if it didn’t return (e.g. in pitchscreen, quickscreen, id3 viewer).
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.

havnt looked at your changes but I assume they are fine.
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)

I have used the patch today in my “every day usage”, and I had some problems with skip length. Not absolutely reproduceable so far though.

your placement of the add_event() lines is wrong….

Interesting. Telling me how it is exactly wrong, or providing a fix is more interesting though.

edit. few=fix

it is just plain wrong…. which is why I origioannyl just registered them once and left them. the WPS entry/exit code is a mess and needs to be cleaned up more before this is required.

was your commit yesterday to fix the skip length issue with this patch?

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing