[fix] - fix the autoclose timeout calculation - using SystemClockMillis #778

Merged
merged 1 commit into from Mar 31, 2012

Conversation

Projects
None yet
4 participants
@Memphiz
Owner

Memphiz commented Mar 17, 2012

I got problems with the yesno dialog which is shown after resolution switching in iOS. When using the old timeout implementation based on GetFrameTime, that dialog doesn't even show up because it timed out before.

I added 2 printouts for verification of the old, wonky behaviour:

GUIDialog.cpp:L99: i logged the m_showStartTime here

GUIDialog.cpp:L229: i logged the m_showStartTime + m_showDuration < TimeUtils::GetFrameTime() here.

This is the printout:

http://pastebin.com/tmwW98YP

As you can see the timeout is 10 secs for that yesno dialog. And XBMC eats that 10 secs up in no time. Time traveling?

No sure if the switch to SystemClockMillis is the right fix here. You can't test with this special dialog because resolution/monitor switching is not in mainline yet.

@jimfcarroll

This comment has been minimized.

Show comment Hide comment
@jimfcarroll

jimfcarroll Mar 17, 2012

Member

I don't think that's quite right. On windows SystemTimeMillis can wrap. If you're trying to check that a certain duration has gone by use the EndTime class in threads/SystemClock.h - the comment has an explanation.

Member

jimfcarroll commented Mar 17, 2012

I don't think that's quite right. On windows SystemTimeMillis can wrap. If you're trying to check that a certain duration has gone by use the EndTime class in threads/SystemClock.h - the comment has an explanation.

@davilla

This comment has been minimized.

Show comment Hide comment
@davilla

davilla Mar 17, 2012

Contributor

if GetFrameTime is misbehaving on ios, that need to get fixed as it is used elsewhere.

Contributor

davilla commented Mar 17, 2012

if GetFrameTime is misbehaving on ios, that need to get fixed as it is used elsewhere.

@jmarshallnz

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Mar 17, 2012

Member

If it takes ~10 seconds to switch resolutions (your log doesn't go back far enough to verify this) then this would be expected. GetFrameTime() returns the projected next frame time which is based off the last call to UpdateFrameTime(). If this was 10 seconds ago (i.e. the main thread has not rendered for 10 seconds due to it farting around switching resolutions) then this is to be expected.

An alternate fix might be in CGUIDialog::DoProcess() check whether we've rendered or not yet and set the show time there:

if (!HasRendered())
m_showStartTime = currentTime;

Member

jmarshallnz commented Mar 17, 2012

If it takes ~10 seconds to switch resolutions (your log doesn't go back far enough to verify this) then this would be expected. GetFrameTime() returns the projected next frame time which is based off the last call to UpdateFrameTime(). If this was 10 seconds ago (i.e. the main thread has not rendered for 10 seconds due to it farting around switching resolutions) then this is to be expected.

An alternate fix might be in CGUIDialog::DoProcess() check whether we've rendered or not yet and set the show time there:

if (!HasRendered())
m_showStartTime = currentTime;

@Memphiz

This comment has been minimized.

Show comment Hide comment
@Memphiz

Memphiz Mar 18, 2012

Owner

indeed switching display on ios can last 10secs on my slow display. I don't get a callback when its really finished switching so i have 2secs fadeout + 6 secs worst case switching time + 2 secs fade in ... all in all 10 secs ... will test the other approach then...

Owner

Memphiz commented Mar 18, 2012

indeed switching display on ios can last 10secs on my slow display. I don't get a callback when its really finished switching so i have 2secs fadeout + 6 secs worst case switching time + 2 secs fade in ... all in all 10 secs ... will test the other approach then...

@Memphiz

This comment has been minimized.

Show comment Hide comment
@Memphiz

Memphiz Mar 18, 2012

Owner

Updated the PR. I don't think basing the AutoClose timing in the GUIDialog on GetFrameTime is the right thing to do then, when it depends on when we rendered last. So i've done the EndTime approach here. (JM your suggestion didn't work either).

Pretty straight forward i think :)

Owner

Memphiz commented Mar 18, 2012

Updated the PR. I don't think basing the AutoClose timing in the GUIDialog on GetFrameTime is the right thing to do then, when it depends on when we rendered last. So i've done the EndTime approach here. (JM your suggestion didn't work either).

Pretty straight forward i think :)

@jmarshallnz

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Mar 18, 2012

Member

Why didn't it work?

Member

jmarshallnz commented Mar 18, 2012

Why didn't it work?

@Memphiz

This comment has been minimized.

Show comment Hide comment
@Memphiz

Memphiz Mar 18, 2012

Owner

I don't know. I just gave it a quick shot and it didn't do it. I didn't investigate any further because i already felt that basing the timeout on GetFrameTime here is not the right thing to do. Are you of another opinion here? Or just curious why it didn't work?

Owner

Memphiz commented Mar 18, 2012

I don't know. I just gave it a quick shot and it didn't do it. I didn't investigate any further because i already felt that basing the timeout on GetFrameTime here is not the right thing to do. Are you of another opinion here? Or just curious why it didn't work?

@jmarshallnz

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Mar 18, 2012

Member

Both - once rendered, the frame timer should essentially be the same.

Member

jmarshallnz commented Mar 18, 2012

Both - once rendered, the frame timer should essentially be the same.

@Memphiz

This comment has been minimized.

Show comment Hide comment
@Memphiz

Memphiz Mar 18, 2012

Owner

sigh ;)

Owner

Memphiz commented Mar 18, 2012

sigh ;)

@Memphiz

This comment has been minimized.

Show comment Hide comment
@Memphiz

Memphiz Mar 18, 2012

Owner

I find the code much cleaner by using that nice EndTime class jimfcarroll did. (last try)

Owner

Memphiz commented Mar 18, 2012

I find the code much cleaner by using that nice EndTime class jimfcarroll did. (last try)

@Memphiz

This comment has been minimized.

Show comment Hide comment
@Memphiz

Memphiz Mar 19, 2012

Owner

So i gave it another shot. It doesn't work with setting showstarttime in doprocess. Since i just don't get how this is supposed to fix the issue. Could you just add a 10 secs sleep in WinSystemOSX.mm:629 and reproduce my problem and help me out?

Owner

Memphiz commented Mar 19, 2012

So i gave it another shot. It doesn't work with setting showstarttime in doprocess. Since i just don't get how this is supposed to fix the issue. Could you just add a 10 secs sleep in WinSystemOSX.mm:629 and reproduce my problem and help me out?

@jmarshallnz

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Mar 19, 2012

Member

I'll see if I have time to take a look tonight.

Member

jmarshallnz commented Mar 19, 2012

I'll see if I have time to take a look tonight.

@ghost ghost assigned jmarshallnz Mar 23, 2012

@Memphiz

This comment has been minimized.

Show comment Hide comment
@Memphiz

Memphiz Mar 31, 2012

Owner

Thx jm your fix worked only once - so i added a "m_showStartTime = 0;" into DoModal_Internal (in your name as i've squashed the commits) - because the dialog is reused as it seems. Works now. What do you think?

Owner

Memphiz commented Mar 31, 2012

Thx jm your fix worked only once - so i added a "m_showStartTime = 0;" into DoModal_Internal (in your name as i've squashed the commits) - because the dialog is reused as it seems. Works now. What do you think?

@jmarshallnz

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz Mar 31, 2012

Member

Hmm, yes, it will need resetting - I only tested it once :p. I'd do it in GUI_MSG_WINDOW_INIT I think (where the previous code was to set it to the frametime).

Member

jmarshallnz commented Mar 31, 2012

Hmm, yes, it will need resetting - I only tested it once :p. I'd do it in GUI_MSG_WINDOW_INIT I think (where the previous code was to set it to the frametime).

@Memphiz

This comment has been minimized.

Show comment Hide comment
@Memphiz

Memphiz Mar 31, 2012

Owner

done ...

Owner

Memphiz commented Mar 31, 2012

done ...

jmarshallnz added a commit that referenced this pull request Mar 31, 2012

Merge pull request #778 from Memphiz/dialogtimeout
[fix] - fix the autoclose timeout calculation - using SystemClockMillis

@jmarshallnz jmarshallnz merged commit cbd97ea into xbmc:master Mar 31, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment