Skip to content
This repository

X11: refresh window after xrandr event #848

Closed
wants to merge 20 commits into from

6 participants

Rainer Hochecker Joakim Plate Wolfgang Schupp davilla Cory Fields Piethein Strengholt
Rainer Hochecker
Collaborator

This fixes 2 issues.

1)
Window/scaling was not refreshed when xrandr (issued by user outside of XBMC) has changed mode.

2)
On ATI HD series 2000 menu was scrambled after playback stopped and refresh rate was set back to desktop resolution.

Joakim Plate
Collaborator
Rainer Hochecker
Collaborator

It calls g_graphicsContext.SetVideoResolution after the mode switch has completed. This calls ResizeWindow and refreshes the gl context. The issue was reported on ATI HD 2000, 4000, 5000 series. I don't have such a model but was told this has fixed the problem.

Wolfgang Schupp
Collaborator

I run a hd 4250 and can confirm that this resolves 2).
without the patch the gui is always completely scrambled and unreadable.

Joakim Plate
Collaborator

My only hesitance is the include of xrandr stuff in the header file. It will get pulled in pretty much everywhere.

Rainer Hochecker
Collaborator

I removed xrandr from the header. The strategy is that if a user decides to change mode while XBMC is running, this mode becomes our new desktop resolution. So the mode is stored there. Do you agree with this?

xbmc/windowing/X11/WinSystemX11.cpp
... ...
@@ -178,6 +180,50 @@ bool CWinSystemX11::ResizeWindow(int newWidth, int newHeight, int newLeft, int n
178 180
   return false;
179 181
 }
180 182
 
  183
+void CWinSystemX11::RefreshWindow()
  184
+{
  185
+  // save current mode if this is not an internal request
  186
+  // make this our new desktop resolution
  187
+  if (!m_internalModeSwitch)
  188
+  {
  189
+    CLog::Log(LOGNOTICE, "CWinSystemX11::RefreshWindow - external or initial xrandr event");
  190
+    g_xrandr.Query(true);
  191
+    XOutput out  = g_xrandr.GetCurrentOutput();
  192
+    XMode   mode = g_xrandr.GetCurrentMode(out.name);
  193
+    UpdateDesktopResolution(g_settings.m_ResInfo[RES_DESKTOP], 0, mode.w, mode.h, mode.hz);
  194
+    g_settings.m_ResInfo[RES_DESKTOP].strId     = mode.id;
  195
+    g_settings.m_ResInfo[RES_DESKTOP].strOutput = out.name;
7
Joakim Plate Collaborator
elupus added a note April 10, 2012

Hmm.. there is no guarantee current mode is desktop so you can't overwrite it with current mode.

Rainer Hochecker Collaborator

After a forced query: g_xrandr.Query(true) GetCurrentOutput returns the current resolution. Isn't this desktop resolution?

Joakim Plate Collaborator
elupus added a note April 10, 2012
Rainer Hochecker Collaborator

That's why I do Query(true) here. After this GetCurrentOutput returns the resolution the user has set to with xrandr. The assumption is that when a user uses xrandr he/she wants this new mode as the desktop resolution. XBMC would then switch to this new mode when going windowed.

Rainer Hochecker Collaborator

I think I misunderstood you first comment. The question is should an external xrandr command overwrite the desktop resolution, right? I think so, in particular when using pvr where a user wants to switch frequently between fullscreen and minimized view.

Joakim Plate Collaborator
elupus added a note April 11, 2012

Hmm.. I see your point.

But maybe we should be checking if we are fullscreen or not instead of the m_internalModeSwitch?

If we are windowed, it should always update desktop resolution. If we are fullscreen it should not, just switch to the new resolution. (you can change rez by xrandr via ssh for example to test it).

Rainer Hochecker Collaborator

Yes, this makes sense. I will change it. Thanks.

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

Looks much cleaner now.

Rainer Hochecker
Collaborator

@elupus
Do you still see any errors/problems?

xbmc/windowing/X11/WinSystemX11.cpp
((13 lines not shown))
198 253
     OnLostDevice();
199 254
     g_xrandr.SetMode(out, mode);
200 255
   }
201  
-  else
202  
-    g_xrandr.RestoreState();
203 256
 #endif
2
Joakim Plate Collaborator
elupus added a note April 18, 2012

This is quite different from previous code. When we go to non fullscreen we previously ignored the given resolution parameter and use what had stored. Now it respects the passed resolution. I'm not sure all calls to that function passes a valid resolution when fullscreen is false.

Maybe it should use the data from DESKTOP resolution always in that case?

Rainer Hochecker Collaborator

Yes, I was uncertain about this. Changed it to use RES_DESKTOP if not fullscreen.

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

@elupus
Could you have another look on this? I changed:

  • SetFullScreen uses the fullscreen flag again
  • fixed RefreshWindow to work correctly in windowd mode
Joakim Plate elupus commented on the diff May 17, 2012
xbmc/windowing/X11/WinSystemX11.cpp
((9 lines not shown))
  196
+
  197
+  // only overwrite desktop resolution, if we are not in fullscreen mode
  198
+  if (!g_graphicsContext.IsFullScreenVideo())
  199
+  {
  200
+    CLog::Log(LOGDEBUG, "CWinSystemX11::RefreshWindow - store desktop resolution, width: %d, height: %d, hz: %2.2f", mode.w, mode.h, mode.hz);
  201
+    UpdateDesktopResolution(g_settings.m_ResInfo[RES_DESKTOP], 0, mode.w, mode.h, mode.hz);
  202
+    g_settings.m_ResInfo[RES_DESKTOP].strId     = mode.id;
  203
+    g_settings.m_ResInfo[RES_DESKTOP].strOutput = out.name;
  204
+  }
  205
+
  206
+  RESOLUTION_INFO res;
  207
+  unsigned int i;
  208
+  bool found(false);
  209
+  for (i = RES_DESKTOP; i < g_settings.m_ResInfo.size(); ++i)
  210
+  {
  211
+    if (g_settings.m_ResInfo[i].strId == mode.id)
4
Joakim Plate Collaborator
elupus added a note May 17, 2012

Doesn't this need to checkout output too?

Rainer Hochecker Collaborator
FernetMenta added a note May 17, 2012

I think your are right. I will rework it.

Joakim Plate Collaborator
elupus added a note June 17, 2012

Still seem to be missing output check.

Rainer Hochecker Collaborator
FernetMenta added a note June 17, 2012

Yeah, I need to update the PR. In the meantime I have almost fixed multi-head setups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Joakim Plate
Collaborator
elupus commented May 17, 2012

@bobo1on1 @theuni @cptspiff i need some comments here.

We are already rather broken on multi display setups, i'd rather not break it furhter.

Rainer Hochecker
Collaborator

I am currently working on a closely related topic: SDL prevents rotating the screen left or right. so I have to ditch SDL for X11 video and window events. I think I'll have something for review after the weekend. I would attach it this PR.

Rainer Hochecker
Collaborator

I updated this pr, rotating screen with xrandr does work now. This required to get rid of SDL for video and events (SDL joystick is still there)

  • in this iteration sdl video can be disabled by configuring with --disable-sdlvideo EDIT: the switch is now --enable-sdlx11, which is disabled by default
  • multi-screen setup might not work: I don't have such a setup
  • testing on DARWIN and GLES needs to be done
  • 30d38b2 is only temporary for testing
davilla
Collaborator
davilla commented May 23, 2012

darwin/osx seems fine and untouched.

this seems a little specific, defining a HAVE_SDL_VIDEO_X11 just for X11 usage when with a little work, HAVE_SDL_VIDEO could be used for all. But I can cleanup/refactor to be more generic after inject. I'll do a patch that does this and you can decide if you want to merge that patch into this pull/req

Rainer Hochecker
Collaborator

You are welcome if you want to invest the time. My idea was getting rid of SDL for X11 in a second round, this would make this macro obsolete. I didn't dare in this round but I see no need for SDL (X11) anymore.

davilla
Collaborator
davilla commented May 23, 2012

ok, I've added some inline comments that need to get fixed.

davilla
Collaborator
davilla commented May 23, 2012

pulled again

I see

if test "$use_sdlvideo" = "yes"; then

in configure.in

also see http://pastebin.com/RU5jLf5h

Rainer Hochecker
Collaborator

@davilla
Not sure what kind of a brain seizure I had yesterday. All your comments turned out to be correct. Check the "squash me" commits.

davilla
Collaborator
davilla commented May 24, 2012

ok now :)

Joakim Plate
Collaborator
elupus commented June 17, 2012

Imho... Can't we just drop the SDL path completely? Less ifdeffery and it's something we have wanted for a long time.

Rainer Hochecker
Collaborator

"Can't we just drop the SDL path completely"

Yes will do, with regard on all the other changes I have not merged into this PR it makes no sense maintaining both versions.

Cory Fields
Owner
theuni commented June 17, 2012

agreed.

Rainer Hochecker
Collaborator

I have update the PR with fixes for multi-monitor setups. Considerations:

On X11 monitors can be configured to be on separate X-screens or to share the same virtual screen. I needed to introduce a new gui settings parameter: Monitor.

Xrandr allows to dynamically add modes, this happens e.g. when a monitor is connected. This raises the need for dynamic updates of resolutions.

TODO:
Calibrations are broken in this branch. The problem is that they are stored with resolutions which may change or even not be available when XBMC is started. Currently all resolutions are saved to guisettings even those which have no calibration changes. This is not ideal. I am looking for a solution which updates a resolution with a stored calibration at the time a relosolution becomes available. Ideas? Should calibrations have their own section in settings and saved instead of resolutions?

davilla
Collaborator

"On X11 monitors can be configured to be on separate X-screens or to share the same virtual screen. I needed to introduce a new gui settings parameter: Monitor."

icky unless absolutely required. users will constantly misconfigure it then complain when it does exactly what they setup but they failed to understand the ramifications.

What's the 'most common setup' for users and how are we doing it now ? a) separate X-screens or b) share the same virtual screen. I suggest coding for the most common and moving the other to as.xml.

Rainer Hochecker
Collaborator

I think we can't favor a) over b) or vice versa. NVidia currently does not support xrandr 1.3 whereas ATI and Intel do. Secondly I would like to make it most comfortable to the user. In most cases they don't even know what a screen is but they can identify a monitor. Selecting a monitor implicitly identifies the X screen or the output of a virtual screen.

Users can't misconfigure this setting. On startup I check whether the monitor is present. If it is not connected, the output goes to the first connected monitor.

Rainer Hochecker
Collaborator

@davilla
The parameter Monitor is only for X11
You could do me a favor and have a closer look into 0840d65

Piethein Strengholt
Collaborator

I've tested these patches on my latest xbmcbuntu build. No issues found.

Joakim Plate
Collaborator
elupus commented July 03, 2012

@bobo1on1 probably should have a look at the ref clock things

@FernetMenta i assume you intend to squash some things together in this series. See some squash comments and some fixes on previous commits. Apart from some things i'm liking the look of it.

Rainer Hochecker
Collaborator

i assume you intend to squash some things together in this series. See some squash comments and some fixes on previous commits. Apart from some things i'm liking the look of it.

Correct. I expect some more changes from the review, then I will finalize the commits.

Joakim Plate
Collaborator
elupus commented July 05, 2012

I've started refactoring this series a bit.. Nothing major, mainly squashing things and moving hunks between commits, then i'll try to resolve some of the comments i've made above. Will post a branch later. Just wanted to let you know so we don't do double work.

Joakim Plate
Collaborator
elupus commented July 05, 2012

So i've done some restructuring of this series. Split some commits modified some things added some things that should be squashed later: https://github.com/elupus/xbmc/compare/dropsdl

There is not much changed compared to your branch in the final diff: http://paste.ubuntu.com/1076449/ but i've squash some things together and moved hunks between commits to keep it a bit more logical.

I'll probably keep on working on the first parts of this. The SDL -> X11 change, to try to finalize to get in. But would be nice if you could swap over and work on this series as a base instead.

Rainer Hochecker
Collaborator

Great, thanks! I will reset my branch to yours later.
I just noticed there's dead code in it. HAS_SDL_VIDEO_X11 does not exist anymore which makes WinSystemX11::CheckDisplayEvents() obsolete. It's a left-over from a previous iteration.

Joakim Plate
Collaborator
elupus commented July 05, 2012
Rainer Hochecker
Collaborator

Right, I started with the SDL code and changed it a bit. The parameters of XGrabPointer are different to allow the mouse leaving fullscreen XBMC. LeaveNotify triggers an ungrab. I agree, this is still ugly and __NET_ACTIVE_WINDOW seems more elegant. Will you do?
Is the current code broken? I have been testing with Unity (mostly 2D) and did not notice.

Joakim Plate
Collaborator
elupus commented July 05, 2012
Rainer Hochecker
Collaborator

elupus, I have based this PR to your branch.

Joakim Plate
Collaborator
elupus commented July 07, 2012

I'll keep working on it some more i think. I have some more stuff i want to change, then I'll open a new pull request. This one has ended up rather off topic from it's original intent.

Rainer Hochecker
Collaborator

Ok. There are some issues I noticed but have not implemented yet in this PR:

1)
Running XBMC without a WM results in a X Error message in the log.
CWinSystemX11::XErrorHandler: BadAtom, type:0, serial:54, error_code:5, request_code:18 minor_code:0

2)
Keyrepeat is hard coded to 10ms. I think this value should be increased or made configurable.

Are you going to have a look at this? Or do you want me to send you PRs to your dropsdl branch?

Joakim Plate
Collaborator
elupus commented July 07, 2012
Joakim Plate
Collaborator
elupus commented July 17, 2012

So i've tried to rework this a bit: #1175 let's continue this on that pull request since this has got a bit off topic.

Note, i'll probably force push that branch again. Some of these commit should still have your name in them since you are the majority author and we are likely to find more bugs.

Joakim Plate elupus closed this July 17, 2012
Joakim Plate elupus referenced this pull request September 16, 2012
Closed

Dropsdl #1175

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.