Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't adjust refreshrate back to RES_DESKTOP while movie is still playing... #1242

Merged
merged 2 commits into from
Aug 10, 2012

Conversation

Memphiz
Copy link
Member

@Memphiz Memphiz commented Aug 4, 2012

This is a feature request from the forum and i really want this.

When we play a movie with refreshrate adjustment and we switch to the menu while having the movie in the background - the desktop resoltution is set. In that case the refreshrate is also set back to 60hz normally. So whenever we change between fullscreen and menu overlay we get the annoying refreshrate switchtime (depends on the tv - but usually some secs black screen).

This PR only changes back to res_desktop when no movie is playing. So we get a smooth instant transition from videofullscreen to the overlay menu and only switch refreshrates when we start or stop a movie.

@jmarshallnz
Copy link
Contributor

Does it switch back when playback stops?

@FernetMenta
Copy link
Contributor

We have so many posts in the forum regarding refresh rate. How about giving the user more insight on what's happening behind the scenes? Example:

  • start video will bring up a selection dialog with available refresh rates (+ no change), bast matching preselected. One big advantage would be that on NVidia (Linux) users will see that there is only 23.971 instead of 23.976 as long as they don't have configured a proper modeline.
  • stopping a video will switch back to look-and-feel-resolution
  • refresh rate dialog can be opened by button on video OSD (or hotkey)
  • dialog can be configured to show up or not, default is yes

@mkortstiege
Copy link
Member

@FernetMenta, Not sure if i like it. If it's really needed we should not default to true.

@FernetMenta
Copy link
Contributor

At least bring up a big fat warning window if refresh rate does not match exactly :)

@Memphiz
Copy link
Member Author

Memphiz commented Aug 5, 2012

Well i would not like to introduce any new click to play feature (imagine there is a resume point. It first ask if you want to resume and then it asks for the refreshrate).

Asking for the refreshrate is not needed - XBMC already is able to find the correct refreshrate.

This commit is more about stopping unneeded refreshrate switches (which really can last about 5 secs and are totally annoying and destroying the experience).

This also will not become a PR with any gui options. I want the refreshrate adjustment to be smart. We don't want to introduce any new settings for this.

The only thing which was pointed out by jm. This fix behaves as follows:

  1. Start movie playback
    a. goto fullscreenvideo? -> refreshrate from the renderer (so the adjusted fps fitting refreshrate).
    b. no fullscreenvideo (menu in forground)? -> refreshrate from the renderer
  2. Stop movie playback
    a. stop from fullscreenvideo? -> switch back to res_desktop refreshrate/look&feel res (60hz for example)
    b. stop from non-fullscreenvideo (menu in forground)? -> refreshrate from the renderer (possible issue...)

1.b stops the annoying refreshrate switching once a video is playing and the tab key gets pressed for showing the menu

2.b leads to a regression if you want to call it that way. This means if you play a 24fps movie the refreshrate changes to 24hz. Then hit tab for getting the menu in forground. Then stop playback. This would leave us in the 24hz mode when no video is playing.

So if this is an issue. We should switch to res_desktop/look&feel res on playback stop.

On my tests i had no issues running the gui with 24hz ...

@FernetMenta if its about 23.blablah fps movies. We could fire up a toast dialog when the "wrong" modlines are detected. But this would be out of scope of this PR imho.

@jmarshallnz
Copy link
Contributor

IMO when playback stops it should revert to the UI res, yes.

One thing to consider is that we could also be switching from/to a resolution that differs by more than just the refresh rate in the future. There's a patch to not scale video for example.

@Memphiz
Copy link
Member Author

Memphiz commented Aug 5, 2012

Yeah i know of that PR too. Imho it wouldn't change anything to this PR. During video playback the UI would just stay in the renderer resolution aswell. Since the native resolution PR claims that users with super duper external upscalers are getting high quality - they won't really recognize that the menu resolution is low :p.

I guess the native resolution PR ends up as a menu option. If it hits mainline someday we could just disable this PR here once the native resoltution is switched on (so it would change resolution back to res_desktop/look&feel res once the menu is in foreground). Nothing to think of now imho because the outcome of the native res PR is still unsure as far as i have read.

@FernetMenta
Copy link
Contributor

Asking for the refreshrate is not needed - XBMC already is able to find the correct refreshrate.

Only if you have a proper setup which is very often not the case. (At least on Linux where you have edit xorg.conf)
Ok, this is off topic here.

So if change of refresh rate no longer solely belongs to GUIWindowFullScreen, shouldn't the call to CGraphicContext::SetFullScreenVideo removed from there and hooked into start/stop player? E.g. PVR allows to start videos not fullscreen. Currently you have the switch when entering full-screen.

@Memphiz
Copy link
Member Author

Memphiz commented Aug 5, 2012

@FernetMenta mhh i have tested PVR with this patch. I'm a bit unsure why. But it behaves same like before on first selection of a tv channel. So it won't switch during the preview playback but when i switch fullscreen. Maybe g_application.IsPlayingVideo() is not set yet when starting in preview mode? Not sure...

@FernetMenta
Copy link
Contributor

That's because SetFullScreenVideo is called by GUIWindowFullScreen and the minimized PVR view is a separate window.
I was trying to raise this question: Does switching refresh rate belong solely to GUIWindowFullScreen? This PR suggests no, hence I would remove the call from there and call it whenever player is started/stopped.

@Memphiz
Copy link
Member Author

Memphiz commented Aug 5, 2012

Mhh not sure. If we would move it to playback start/stop it would switch resolution on starting a live tv channel even in the preview. Thats again a thing which definitly will annoy users.

So in that case i would say. Refreshrate should be switched to movie fps whenever the movie goes fullscreen at the first time (this is not the same as playback start!). Then it should only change back when playback is stopped - not when we hit tab and go into the menu.

@Hitcher
Copy link
Contributor

Hitcher commented Aug 5, 2012

I've used the GUI at 24Hz and it's horrible (I can even see a difference at 50Hz) so I'd want it to be used at 60Hz whether a video is playing in the background or not.

@Memphiz
Copy link
Member Author

Memphiz commented Aug 5, 2012

HitcherUK are you using refreshrate adjustment? Don't you get the refreshrate switch when video plays and you go into the menu?

@Hitcher
Copy link
Contributor

Hitcher commented Aug 5, 2012

Yes but it's less than a second and it's much, much less annoying than a flickering GUI in my opinion.

@Memphiz
Copy link
Member Author

Memphiz commented Aug 5, 2012

Ok we make this an 3way option instead of a bool then. Adjustrefreshrate can be switched to:

  1. Never (maybe rename to Off? not sure?)
  2. Always (this is the former behavement)
  3. On Start/Stop (the new behaviour including switching back to res_desktop/look&feel res on playback stop) - maybe needs a better naming too?

@Memphiz
Copy link
Member Author

Memphiz commented Aug 6, 2012

Anyone if this is an acceptable thing? Any testers out there? ^^

@neilj1983
Copy link

Worked fine for me!

@ghost
Copy link

ghost commented Aug 8, 2012

fine in principle but 2b needs resolving. doing it just small preview windows is silly imo. this about annoyance while wanting to do a quick menu op or whatever, not about smooth-as-butter 128x96 movie playback ;)

@Memphiz
Copy link
Member Author

Memphiz commented Aug 8, 2012

2b is solved already with these commits (switching back to res desktop on playback stop)

…ng if we switch refreshrates on player start/stop only or always when leaving/entering WindowFullscreenVideo
@Memphiz
Copy link
Member Author

Memphiz commented Aug 9, 2012

Rebased and changed the cptspiff things. If this should go in in this merge window i have to pull the button today (not there tomorrow) or someone else has to do it tomorrow.

@Memphiz
Copy link
Member Author

Memphiz commented Aug 9, 2012

Mhhh might merge this from work tomorrow if nobody stands on his head in the next 12 hours.

Memphiz added a commit that referenced this pull request Aug 10, 2012
Don't adjust refreshrate back to RES_DESKTOP while movie is still playing...
@Memphiz Memphiz merged commit 9bac537 into xbmc:master Aug 10, 2012
@Voyager1
Copy link

I'm having issues with this. I start my desktop in 50Hz. Then when I start a movie at 24fps, it correctly switches to 24Hz, but when I stop the movie it does not flip back to 50Hz (as it used to).
To make it worse, when I then play a 25fps (European) Bluray rip, it tries to play it at 23.976fps ('cause sync playback to display is ticked) and that doesn't look good at all...

@Memphiz
Copy link
Member Author

Memphiz commented Aug 14, 2012

@Voyager-xbmc

Is this on windows? If so you can follow it here:

http://forum.xbmc.org/showthread.php?tid=135660&pid=1168524#pid1168524

I'm unable to reproduce the problem sadly. I need someone who provides a log.

@Voyager1
Copy link

yes, Windows and same issue - sorry I didn't see the forum thread.

@Memphiz
Copy link
Member Author

Memphiz commented Aug 14, 2012

fixed

@Hitcher
Copy link
Contributor

Hitcher commented Aug 29, 2012

The wording of these options has me confused.

'Adjust display refresh rate to match video - Always' to me means if the video is 24Hz then the display will be ALWAYS be 24Hz whether the video is fullscreen or not and this isn't the case. Shouldn't it be 'Fullscreen only'?

Whereas 'Adjust display refresh rate to match video - On start/stop' to me this means if the video is 24Hz then the display will change to 24Hz when the video starts and wont change back until the video is stopped whether fullscreen or not. Isn't this 'Always' then?

@Memphiz
Copy link
Member Author

Memphiz commented Aug 29, 2012

No its.

Always: Switch refreshrate always when traversing from menu to fullscreen and vice versa
Start/Stop: Switch refreshrate when starting the video and going to fullscreen and stopping the video and going back to the menu. DON'T adjust the refreshrate when we got to the menu while movie is playing.

@elupus
Copy link
Contributor

elupus commented Aug 29, 2012

Alternate texts.
On playback start/stop
On fullscreen enter/exit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants