Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Bugs
  • Category User Interface
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Release 3.9
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by Glued - 2011-08-20
Last edited by Michael Sevakis - 2011-08-23

FS#12238 - WPS delay on pause introduced by r30097

r30097 is a commit of MikeS  FS#12150 .

Mike, unfortunately the mixer fade out on pause has a side effect on WPS: the pause mode kicks in after a very noticeable delay.
It is an issue for themes that handle pause events, f.e. switching viewports on it. Now switching feels slow like walking in water.

I’ve honestly tried fixing it in apps/gui/wps.c > pause_action / unpause_action, but my code either looks dirty or doesn’t work. Since it’s your patch, you probably could fix it properly. At least I hope you can.

Closed by  Michael Sevakis
2011-08-23 01:39
Reason for closing:  Accepted
Additional comments about closing:  

The worst possible solution was chosen for r30340. ;-)

Michael Sevakis commented on 2011-08-20 23:21

I was sort of just waiting to see if there were issues just leaving it to block since the fade was shortened so much in an earlier commit. I have it mostly worked out already and will post a patch ASAP. Volume fading will be done in a tick task to ensure accuracy and audio will be notified when it completes.

Michael Sevakis commented on 2011-08-21 08:16

Try this one out. Seems to work well for me. I hope it addresses the issue well.

Glued commented on 2011-08-21 11:13

The delay is much shorter, yet is still present.

(Dodges the slipper.)

Do you think maybe instead of ticks > timeout api the ui playback status should be just directly connected to buttons actions?
An elevator button lights up when you press it, after all, not on arrival. Plus fading is pretty internal by nature. And from a user pov, it’s more important to see that the button wasn’t missed / diff viewport / stuff than that the fade is completed.

I was actually trying to map actions <> ui directly, thinking to impress you with a shorter solution, but the code is so damn new to me, that I scared my cat cursing.

Glued commented on 2011-08-21 17:59

Mike you’re not getting dircache panics? I had 3, but my build if unclean, so there’s a chance it’s unrelated.

Michael Sevakis commented on 2011-08-21 18:52

The engine changes state immediately after receiving the message to pause/resume, lets the UI thread proceed, then the fade happens in the background, unreleated to the playback state. There really isn’t anything more I could do on its end to improve it since it doesn’t block the UI for longer than it takes to process that. I gave the WPS a look and what’s left might be delay in the screen updates. The old fade() function in wps.c would call skin_update() immediately, but fade() is not used now.

ETA: No dircache panics BTW. I try other targets.

Glued commented on 2011-08-21 19:23

I see that. I was just trying to find a less complex and a slightly faster solution to look cool, and failed :).
As to the delay amount, it’s a huge improvement, barely noticeable now. I’m pretty amazed you’ve came up with the ticks idea that fast.

I checked your code char by char, and there’s really no way it could be done faster.

P.S. Please ignore my panics note, absolutely nothing to do with your code, sorry.

Michael Sevakis commented on 2011-08-21 20:48

:-)

Core playback functionality needs to get the heck out of screen code, which actually shouldn’t be too hard. Anyway, indeed the fading is indeed deep inside this because it uses the mixer and that complexity should remain well hidden.

The consequence of this is that the audio_stop becomes a request for something to happen later and unless I enforce that stop should take effect immediately, the playback engine is still running until the fade is complete (not true with audio_hard_stop, which must have the engine idle). As a side effect, the stop can even be reversed by requesting another action before the fade completes, but that’s what it is, a side effect of delaying the transistion. It is possible to make a stop request “no return”.

Things to think about…

P.S.R. Will do, which makes things easier on me! :-)

Michael Sevakis commented on 2011-08-22 08:18

This one makes “stop” mean “stop” as it should. Includes WPS update calls that should be there. I’m going with this with any needed tweaks for commit. Give it a go.

Glued commented on 2011-08-22 10:20

Your patch is terrible. It now solves the issue completely, while I couldn’t do neither that, nor improve your 1st.
I am against committing as this is just not fair.

Michael Sevakis commented on 2011-08-22 18:55

I like your style. Sure there’s nothing I can do to make it worse?

Michael Sevakis commented on 2011-08-23 00:03

I think this is the worst one so far…a far, far worse thing that I did than I have ever done. It is far too basic in comparison to the others.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing