Support listing available video modes for a monitor #896

ghost · 2019-06-03T10:38:13Z

Resolves #877.

Adds a new method get_video_modes to the MonitorId struct in the public API, and implements it on the Windows platform. I'm going to look into implementing this on macOS as well..

Osspial · 2019-06-03T16:52:35Z

Hey, thanks for the PR!

FYI before you get much further into implementing this on macOS - the vast majority of work is currently being done on the eventloop-2.0 branch, and we're expecting to merge that onto master Soon. I'd suggest reworking this PR against that branch. Fortunately, looking at these changes I wouldn't expect that to be too difficult to do.

ghost · 2019-06-03T17:04:47Z

Thanks for the heads up. I'll take a look.

src/lib.rs

goddessfreya · 2019-06-03T22:49:35Z

src/lib.rs

+
+#[derive(Debug, Clone, Eq, PartialEq, Hash)]
+pub struct VideoMode {
+    dimensions: (u32, u32),


Can this become PhysicalSize instead of (u32, u32)?

This needs to implement Hash which PhysicalSize does not (and can not, as it's essentially a tuple of f64). Hence, we're doing the conversion from (u32, u32) to PhysicalSize in the getter method.

PhysicalSize is stored as u32s and not f64s on eventloop-2.0, so this should be possible once rebased.

Looks like this would depend on the changes made in #895 being merged first.

src/lib.rs

goddessfreya

As @Osspial mentioned, please rebase this off the other branch.

ghost · 2019-06-04T12:03:47Z

I've reworked this against the eventloop-2.0 branch and added support for macOS. There's a bit of a quirk there, as we can't currently obtain the refresh rate of the monitor without some patches upstream against the core-video-sys crate for example.

I'm also not a fan of allocating an additional HashSet on macOS instead of just re-using the iterator we get from calling map on the Vec returned from CGDisplayMode::all_display_modes, but I couldn't come up with a clean way to do this.

How do we proceed with implementing this for the rest of the platforms? As for the Android and iOS platforms I think this comes down to just returning the screen size, and additionally obtaining the refresh rate and bit depth if applicable. I haven't looked into what Emscripten is doing.

src/monitor.rs

goddessfreya · 2019-06-04T23:13:09Z

I've reworked this against the eventloop-2.0 branch and added support for macOS. There's a bit of a quirk there, as we can't currently obtain the refresh rate of the monitor without some patches upstream against the core-video-sys crate for example.

Filled upstream already, yeah?

I'm also not a fan of allocating an additional HashSet on macOS instead of just re-using the iterator we get from calling map on the Vec returned from CGDisplayMode::all_display_modes, but I couldn't come up with a clean way to do this.

I guess you could use a vec, then sort it at the end (or always keep it sorted) and dedup it, however that sounds like a pain to maintain.

ghost · 2019-06-05T05:35:53Z

I've reworked this against the eventloop-2.0 branch and added support for macOS. There's a bit of a quirk there, as we can't currently obtain the refresh rate of the monitor without some patches upstream against the core-video-sys crate for example.

Filled upstream already, yeah?

I'll look into filing a pull request against core-video-sys for this today.

ghost · 2019-06-05T08:42:56Z

We're now reporting the display refresh rate correctly on macOS. I've filed LuoZijun/rust-core-video-sys#2 to expose the necessary functionality from the Core Video API.

ghost · 2019-06-06T05:39:00Z

LuoZijun/rust-core-video-sys#2 has been merged, so all that remains is implementing this for the remaining platforms. I suppose I could take a look at either iOS or Linux today, so that we can ship this as soon as possible.

ghost · 2019-06-06T13:06:48Z

I've finished the iOS implementation. I couldn't get the Emscripten or Android platforms to compile. It looks like their implementation in eventloop-2.0 hasn't been finished.

ghost · 2019-06-08T10:35:08Z

I've finished implementing this on Wayland, and found a cleaner way to return iterators from the video_modes method. I've refactored all platforms accordingly. I'm going to look into implementing this on X11 next.

ghost · 2019-06-08T12:30:47Z

X11 implementation is done. This is now implemented on all platforms except for Android and Emscripten that don't seem to be ported over to eventloop-2.0 yet. Should be ready for final review!

goddessfreya

Quick skim of changes.

src/platform_impl/windows/monitor.rs

src/platform_impl/ios/monitor.rs

goddessfreya · 2019-06-08T16:46:35Z

src/platform_impl/linux/x11/util/randr.rs

+            .filter(|x| output_modes.iter().any(|id| x.id == *id))
+            .map(|x| {
+                let refresh_rate = if x.dotClock > 0 && x.hTotal > 0 && x.vTotal > 0 {
+                    x.dotClock as u64 * 1000 / (x.hTotal as u64 * x.vTotal as u64)


Can you explain this math?

I don't know what's going on here, but this is the same line of code that SDL, GTK and GLFW use to calculate the display refresh rate. I couldn't find any documentation for this.

src/platform_impl/linux/x11/util/randr.rs

goddessfreya · 2019-06-08T16:46:48Z

src/platform_impl/linux/wayland/event_loop.rs

+            .into_iter()
+            .map(|x| VideoMode {
+                dimensions: (x.dimensions.0 as u32, x.dimensions.1 as u32),
+                refresh_rate: (x.refresh_rate as f32 / 1000.0 + 0.5) as u16,


Can you explain this math?

We're receiving the refresh rate from Wayland in millihertz, so we must divide it by a thousand to get it in hertz. We then effectively ~~ceil~~ round the float before casting it into an integer, but I suppose we could've used ~~f32::ceil~~ f32::round here also.

In all, I'm not sure it is the right approach to represent refresh rates as u16 in Hertz. There are displays whose refresh rate is not an integer in this unit, no?

The refresh rate is usually not an exact integer, but very close (e.g. 59.996 Hz). We could report the display refresh rate as a float, but Windows and iOS only report it as an integer, so it might be misleading to return a float on these platforms. I assume this is what you meant?

I don't get what would be misleading about using a float while some platform always return integer values, could you elaborate on that?

We already have precedent with the DPI factor values, which are always integer on Wayland for example.

To elaborate, I feel like that returning a float here implies that this is an exact refresh rate, when in fact it's truncated or rounded (on at least two platforms). Returning an integer type does not have this implication to me.

If we look at other windowing libraries, there's precedent of using integers to represent the refresh rate in SDL and GLFW. I don't feel particularly strongly about this either way, so I'm fine with changing this to return floats if we all agree on that.

I personally have the other expectation, imo an integer refresh rate suggests that it is exact...

Anyway, I don't know much about which uses an app would have for the refresh rate. If rounding it doe snot pose any functionality issue, then I'm fine with it.

Having slept on it, I think we should leave it as a rounded integer, so at least the behaviour is consistent across platforms. If the user requires an exact value, there's not much point in us returning an exact value on some platforms, and a rounded value on others. Furthermore, changing this to a floating point value would complicate the implementation (though slightly) for VideoMode as we would now have to implement Hash and Eq by hand. If there later appears an use-case where an exact value is required, we can revisit this. For now, perhaps we can document it in the function description that the returned value is an approximation?

src/platform_impl/macos/monitor.rs

aloucks · 2019-06-09T05:48:40Z

It makes it difficult to see what's going on when you force-push after comments have started. The commits will all get squashed in the end anyway.

ghost · 2019-06-09T06:01:50Z

Sorry about that. I didn't realize this repository squashes pull requests upon merge. While making changes to the Wayland platform, I noticed that I needed to restructure the other platforms as well, and I decided to squash it all into a single commit at that point. There shouldn't be any more large changes landing to this branch at this point.

Osspial

I've got some relatively nitpicky changes, but overall this looks good. Thanks for looking into this!

FEATURES.md

src/platform_impl/windows/monitor.rs

felixrabe · 2019-07-01T15:34:44Z

I just noticed on my old macOS 10.9 MBAir that the core-video-sys crate brings in metal as a dependency, which means I can't compile at all on that machine. (This is my hack-during-commute laptop.)

Is there some way to do this without depending on Metal?

ghost · 2019-07-01T15:47:47Z

We don't actually need Metal for this at all, but core-video-sys happens to pull that in for other reasons. I've opened LuoZijun/rust-core-video-sys#3 to make the Metal dependency optional. I will follow this up with a pull request to winit that then disables the "metal" feature for the core-video-sys dependency.

* Support listing available video modes for a monitor * Use derivative for Windows `MonitorHandle` * Update FEATURES.md * Fix multiline if statement * Add documentation for `VideoMode` type

goddessfreya reviewed Jun 3, 2019

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

goddessfreya reviewed Jun 3, 2019

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

goddessfreya suggested changes Jun 3, 2019

View reviewed changes

ghost changed the base branch from master to eventloop-2.0 June 4, 2019 07:24

ghost changed the title ~~Add MonitorId::get_video_modes to list available modes~~ Add MonitorId::video_modes to list available modes Jun 4, 2019

ghost changed the title ~~Add MonitorId::video_modes to list available modes~~ Support listing available video modes for a monitor Jun 4, 2019

goddessfreya reviewed Jun 4, 2019

View reviewed changes

src/monitor.rs Outdated Show resolved Hide resolved

ghost marked this pull request as ready for review June 8, 2019 12:28

goddessfreya suggested changes Jun 8, 2019

View reviewed changes

aleksijuvani added 2 commits June 11, 2019 08:54

Support listing available video modes for a monitor

ab73921

Use derivative for Windows MonitorHandle

cb3bb4b

Osspial suggested changes Jun 11, 2019

View reviewed changes

FEATURES.md Outdated Show resolved Hide resolved

src/platform_impl/windows/monitor.rs Outdated Show resolved Hide resolved

aleksijuvani added 2 commits June 11, 2019 21:59

Update FEATURES.md

2c14da5

Fix multiline if statement

de705e3

Osspial approved these changes Jun 11, 2019

View reviewed changes

goddessfreya approved these changes Jun 11, 2019

View reviewed changes

Add documentation for VideoMode type

208afa7

Osspial merged commit 47b5dfa into rust-windowing:eventloop-2.0 Jun 12, 2019

Osspial mentioned this pull request Jun 21, 2019

Support listing available video modes for a monitor #877

Closed

ghost mentioned this pull request Jul 2, 2019

Remove Metal dependency on macOS #1003

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support listing available video modes for a monitor #896

Support listing available video modes for a monitor #896

ghost commented Jun 3, 2019 •

edited by ghost

Loading

Osspial commented Jun 3, 2019

ghost commented Jun 3, 2019

goddessfreya Jun 3, 2019 •

edited

Loading

ghost Jun 4, 2019

Osspial Jun 4, 2019

ghost Jun 4, 2019

goddessfreya left a comment

ghost commented Jun 4, 2019

goddessfreya commented Jun 4, 2019

ghost commented Jun 5, 2019

ghost commented Jun 5, 2019

ghost commented Jun 6, 2019

ghost commented Jun 6, 2019

ghost commented Jun 8, 2019

ghost commented Jun 8, 2019

goddessfreya left a comment

goddessfreya Jun 8, 2019

ghost Jun 9, 2019

goddessfreya Jun 8, 2019

ghost Jun 9, 2019 •

edited by ghost

Loading

elinorbgr Jun 10, 2019

ghost Jun 10, 2019

elinorbgr Jun 11, 2019

ghost Jun 11, 2019

elinorbgr Jun 11, 2019

ghost Jun 12, 2019

aloucks commented Jun 9, 2019

ghost commented Jun 9, 2019

Osspial left a comment

felixrabe commented Jul 1, 2019 •

edited

Loading

ghost commented Jul 1, 2019

Support listing available video modes for a monitor #896

Support listing available video modes for a monitor #896

Conversation

ghost commented Jun 3, 2019 • edited by ghost Loading

Osspial commented Jun 3, 2019

ghost commented Jun 3, 2019

goddessfreya Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goddessfreya left a comment

Choose a reason for hiding this comment

ghost commented Jun 4, 2019

goddessfreya commented Jun 4, 2019

ghost commented Jun 5, 2019

ghost commented Jun 5, 2019

ghost commented Jun 6, 2019

ghost commented Jun 6, 2019

ghost commented Jun 8, 2019

ghost commented Jun 8, 2019

goddessfreya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jun 9, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloucks commented Jun 9, 2019

ghost commented Jun 9, 2019

Osspial left a comment

Choose a reason for hiding this comment

felixrabe commented Jul 1, 2019 • edited Loading

ghost commented Jul 1, 2019

ghost commented Jun 3, 2019 •

edited by ghost

Loading

goddessfreya Jun 3, 2019 •

edited

Loading

ghost Jun 9, 2019 •

edited by ghost

Loading

felixrabe commented Jul 1, 2019 •

edited

Loading