FS#12238 : WPS delay on pause introduced by r30097

FS#12238 - WPS delay on pause introduced by r30097

Attached to Project: Rockbox
Opened by Glued (glued) - Saturday, 20 August 2011, 17:59 GMT
Last edited by Michael Sevakis (MikeS) - Tuesday, 23 August 2011, 01:39 GMT

Task Type	Bugs
Category	User Interface
Status	Closed
Assigned To	No-one
Operating System	All players
Severity	Low
Priority	Normal
Reported Version	Release 3.9
Due in Version	Undecided
Due Date	Undecided
Percent Complete
Votes	0
Private	No

Details

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.

This task depends upon

Closed by Michael Sevakis (MikeS)
Tuesday, 23 August 2011, 01:39 GMT
Reason for closing: Accepted
Additional comments about closing: The worst possible solution was chosen for r30340. ;-)

Comments (11)
Related Tasks (0/0)

Comment by Michael Sevakis (MikeS) - Saturday, 20 August 2011, 23:21 GMT

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.

Comment by Michael Sevakis (MikeS) - Sunday, 21 August 2011, 08:16 GMT

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

async-playback-fade-01.patch (11.1 KiB)

Comment by Glued (glued) - Sunday, 21 August 2011, 11:13 GMT

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.

Comment by Glued (glued) - Sunday, 21 August 2011, 17:59 GMT

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

Comment by Michael Sevakis (MikeS) - Sunday, 21 August 2011, 18:52 GMT

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.

Comment by Glued (glued) - Sunday, 21 August 2011, 19:23 GMT

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.

Comment by Michael Sevakis (MikeS) - Sunday, 21 August 2011, 20:48 GMT

:-)

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! :-)

Comment by Michael Sevakis (MikeS) - Monday, 22 August 2011, 08:18 GMT

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.

async-playback-fade-02.patch (11.3 KiB)

Comment by Glued (glued) - Monday, 22 August 2011, 10:20 GMT

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.

Comment by Michael Sevakis (MikeS) - Monday, 22 August 2011, 18:55 GMT

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

Comment by Michael Sevakis (MikeS) - Tuesday, 23 August 2011, 00:03 GMT

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.

async-playback-fade-03.patch (10.3 KiB)

	Tasks related to this task (0)

Duplicate tasks of this task (0)

Rockbox

FS#12238 - WPS delay on pause introduced by r30097

Details

Loading...