GNOME Bugzilla – Bug 777687
Fix hidpi detection on vertical monitor layouts
Last modified: 2017-04-12 09:16:40 UTC
Mutter assigns a wrong output scale (=1) when a hidpi screen is rotated to portrait. It turns out that the code doing the scale calculations operate on pixel and mm sizes, with the small catch that pixel sizes have the applied transformation (i.e. h>w), which botches the DPI calculations, and bail out because one axis falls below the hidpi threshold. I'm attaching a patch that fixes this by transforming back pixel sizes to the untransformed layout, so comparisons happen between the right axes.
Created attachment 344104 [details] [review] backends: Calculate output scale correctly on vertical transforms The code calculating the output scale involves calculations around pixel and mm sizes, however we do compare post-transformation pixel sizes to untransformed mm sizes, which breaks the DPI calculations. Fix this by transforming back pixel sizes back to untransformed. While we're at it, actually compare the output height to HIDPI_MIN_HEIGHT instead of its width, it seems right according to the #define name and comment.
I have a commit on my wip branch that accidentally fixes this, as part of a larger refactorization: https://github.com/jadahl/mutter/commit/d538a0218eb4d20fa4d4833ae41857fbb5bfc886 The patch looks good, but could we just put it on gnome-3-22, but just go with some version of the one linked above for master? It'd save me some rebase head ache.
Sure, makes sense.
Pushed to gnome-3-22, master will be handled in bug #777732 Attachment 344104 [details] pushed as f8fd02d - backends: Calculate output scale correctly on vertical transforms
Created attachment 348163 [details] [review] meta-monitor-config: Pass logical monitor scale via config The default (calculated) scale is derived from the output, but ultimately set via the monitor scale. This will enable config files to override the scale. Yet to be done is handling when a scale is not supported by a backend (i.e. the X11 backend).
Created attachment 348164 [details] [review] monitor-manager: Remove 'scale' from MetaOutput Replace the 'scale' of an output with a vfunc on the MetaMonitorManager class that takes a monitor and a monitor mode which calculates the scale. On X11 this always returns 1, on KMS, the old formula is used. On the dummy and test backends, the already configured values are returned.
Review of attachment 348164 [details] [review]: Hmm, I fear this didn't rebase as easily. It seems to work fine to fix the issue of this bug (i.e. rotating doesn't affect scale anymore), but I do get crashes as soon as I plug an external monitor, it seems it's trying to create the MetaLogicalOutput (and guess its scale) when the MetaMonitor still has a NULL MetaMonitorMode. Since MetaLogicalOuput scale seems static after creation, I guess it's not ok to just guess 1 in this case, so there's perhaps any more piece missing here?
Created attachment 348657 [details] [review] monitor-manager: Update the monitor mode state before the logical state This means we can use up to date monitor mode data when generating the logical state. ----- (In reply to Carlos Garnacho from comment #7) > Review of attachment 348164 [details] [review] [review]: > > Hmm, I fear this didn't rebase as easily. It seems to work fine to fix the > issue of this bug (i.e. rotating doesn't affect scale anymore), but I do get > crashes as soon as I plug an external monitor, it seems it's trying to > create the MetaLogicalOutput (and guess its scale) when the MetaMonitor > still has a NULL MetaMonitorMode. > > Since MetaLogicalOuput scale seems static after creation, I guess it's not > ok to just guess 1 in this case, so there's perhaps any more piece missing > here? Yes, seems so. I put this patch before the others, and I can plug in a monitor withoun anything crashing. The only thing that worries me with these patches is that some user sets a mode on a monitor that is unknown to mutter, meaning it couldn't derive the current mode. I don't think it should happen, but maybe we should consider being more graceful in this regard, at least in paths that can be taken when running on X11.
As discussed in IRC, I did a forward port of the patch to master.
*** Bug 775349 has been marked as a duplicate of this bug. ***