Add `MonitorHandle::current_video_mode()` #3802

daxpedda · 2024-07-19T13:49:13Z

After some discussion we decided its more useful to encode current monitor information into VideoModeHandle then move more information to MonitorHandle. To this end this PR implements MonitorHandle::current_video_mode().

Additionally MonitorHandle::size() and refresh_rate_millihertz() were removed, because it logically fits into video modes and not the monitor itself.

To improve the output, this PR also changes VideoModeHandle::refresh_rate_millihertz() to return a Option<NonZeroU32> and size() and bit_depth() an Option.

Follow up to #3801.

src/platform_impl/apple/appkit/monitor.rs

src/platform_impl/android/mod.rs

kchibisov · 2024-07-20T18:55:05Z

The bit_depth is more of a property of the active video format, so if I'd change the way handle works I'd probably just keep a handle and an option to get the active VideoMode for stuff like that.

The only information which is not video mode related is basically monitor name/position, etc, depth is not it.

daxpedda · 2024-07-20T19:14:13Z

The bit_depth is more of a property of the active video format, so if I'd change the way handle works I'd probably just keep a handle and an option to get the active VideoMode for stuff like that.

The only information which is not video mode related is basically monitor name/position, etc, depth is not it.

That's a good point. That would solve the issue for Web as well.
Will look into it and adapt the PR.

daxpedda · 2024-07-20T20:15:20Z

The bit_depth is more of a property of the active video format, so if I'd change the way handle works I'd probably just keep a handle and an option to get the active VideoMode for stuff like that.

The only information which is not video mode related is basically monitor name/position, etc, depth is not it.

@kchibisov I started working on it right now and realized that this argument invalidates MonitorHandle currently having size and refresh rate.
Is there some story behind it or should we remove it in favor of current_video_mode() exposing the current size and refresh rate?

kchibisov · 2024-07-20T20:55:02Z

Is there some story behind it or should we remove it in favor of current_video_mode() exposing the current size and refresh rate?

I think it was always like that, which doesn't make much sense. Probably it was like that for convenience or something. Just route everything into current_video_mode.

daxpedda · 2024-07-20T22:41:44Z

Is there some story behind it or should we remove it in favor of current_video_mode() exposing the current size and refresh rate?

I think it was always like that, which doesn't make much sense. Probably it was like that for convenience or something. Just route everything into current_video_mode.

Done.

I also changed VideoModeHandle::refresh_rate_millihertz() to return an Option.
Figured this was better then just falling back to 0. or 60_000. everywhere.

src/changelog/unreleased.md

MarijnS95

Approving the Android side given that all the weirdness in there (due to a lack of API in the NDK - we could call into Java for this though) was already there and is only moving in this patch but not fundamentally changing.

src/platform_impl/android/mod.rs

daxpedda · 2024-07-24T23:45:39Z

Re-requested reviews because I changed VideoModeHandle::size() and bit_depth() to return an Option now.
Some backends obviously make use of that now instead of returning default values.

src/platform_impl/android/mod.rs

daxpedda · 2024-07-25T11:51:55Z

src/platform_impl/android/mod.rs

+            ffi::context(&mut env)
+                .display_manager(&mut env)
+                .display(&mut env, self.id)
+                .unwrap()


This could panic, but is a known issue on all backends.
See #3258.

Well, if you function returns Option you can easily avoid panic.

I will make a follow-up PR where I finally tackle #3258.

The thing is that monitor data, such as name, etc, is static more or less. The position could change, indeed, but generally unless we make all of that into events and lifetime I'm not sure we can do much.

Good point.
I'm happy to cache the name, but again, lets discuss it in a follow-up PR.

daxpedda · 2024-07-25T11:53:18Z

src/platform_impl/android/ffi.rs

@MarijnS95 let me know if you think anything in here might actually panic.

kchibisov

I'd probably separate android impl into separate commit and rebase 2 commits in the end, but up to you.

src/monitor.rs

daxpedda · 2024-07-25T12:37:32Z

I'd probably separate android impl into separate commit and rebase 2 commits in the end, but up to you.

I did change it up a bit to make it easier to review.
Thanks for the suggestion!

MarijnS95 · 2024-07-25T13:13:19Z

Without going through the details, it looks like the Android implementation is quite overdone, now with JNI. Even if we discussed this before, I'd like to revise my statement.

When I proposed looking at Android functions and eventually calling them through JNI, it was meant as a hint that there appear to be matching APIs, and could eventually be used if there is a pressing need for it.
Using JNI also beats the user preference for a "native" activity (even though that itself is driven by some Java code, the underlying implementations that you talk against are all native once they've left the Activity lifecycle).

After all, having multiple displays on Android is extremely unlikely, and even for the current display, what is the user going to use this information for? Anything involving dimensions, pixel formats, refresh rate or color spaces will always end up going through the Surface. I think Wayland and specifically its color management protocol are opting for the same thing: everything goes through the surface, and the app doesn't and shouldn't really need to know about the display layout.

EDIT: The only thing I cannot find available in the NDK is a list of available frame rates to help the user guide if it makes sense to request a switch:
https://developer.android.com/media/optimize/performance/frame-rate

Summary: even if we could (as you have already shown): are you sure we want to start having and managing this blob of JNI "noise" in the Android backend? For likely zero actual use-case?

daxpedda · 2024-07-25T13:23:09Z

Summary: even if we could (as you have already shown): are you sure we want to start having and managing this blob of JNI "noise" in the Android backend? For likely zero actual use-case?

AFAIC I was only doing this to prevent having an Option on this method and learning a bit of JNI on the way.
I'm happy to remove the whole thing but Android should then return no MonitorHandle at all to prevent this issue, exactly like it was for Web before #3801.

On the other hand I've already implemented it and we will need some JNI sooner or later anyway (I assume?). I don't know if a multi-monitor setup on Android would even work with the current API (not talking about the implementation) or if it actually needs a dedicated platform-specific API (which I would assume).

Let me know how you would like to proceed.

MarijnS95 · 2024-07-25T13:43:36Z

Besides the effort you've put in, my preference goes out to not returning this information to the user at all. In case someone requests it we can always aim to re-land your commit from this PR (let's track it somewhere just in case) or keep it around as a reference for users desiring to implement this themselves?

I am not thinking of much that needs JNI now, except maybe the PRs that are ramping up towards IME support (whether that belongs in our out winit, I don't know)?

Either way, I've always desired to create an "android-support" kind of crate (under the rust-mobile umbrella) that abstracts away the JNI code (and could use precompiled Java/Kotlin classes for the real nasty stuff that is just too horrible to write in Rust via JNI). Details aside, we could move your JNI code there already?

kchibisov · 2024-07-26T13:29:36Z

I'd note that if Android stuff is controversial, we can split it into separate PR, since the first 3 commits should be good to go.

daxpedda · 2024-07-26T13:33:49Z

Besides the effort you've put in, my preference goes out to not returning this information to the user at all. In case someone requests it we can always aim to re-land your commit from this PR (let's track it somewhere just in case) or keep it around as a reference for users desiring to implement this themselves?

I'm unsure why we would remove it in the first place if we would be willing to put it back in?
But maybe we just don't know if and how we want JNI in Winit yet?

EDIT: Considering we may want that as a reference at a future date daxpedda@f938fc5.

Either way, I've always desired to create an "android-support" kind of crate (under the rust-mobile umbrella) that abstracts away the JNI code (and could use precompiled Java/Kotlin classes for the real nasty stuff that is just too horrible to write in Rust via JNI). Details aside, we could move your JNI code there already?

Yes, lets discuss it on Matrix further!

I'd note that if Android stuff is controversial, we can split it into separate PR, since the first 3 commits should be good to go.

I'm gonna do the removal in a couple of hours or so.
But we should wait for somebody to test/properly review the MacOS changes.

daxpedda · 2024-07-26T14:00:32Z

@MarijnS95 PR is ready for your approval.

MarijnS95 · 2024-08-02T09:43:51Z

I'm unsure why we would remove it in the first place if we would be willing to put it back in?
But maybe we just don't know if and how we want JNI in Winit yet?

Yeah, this mostly. I don't want to commit to maintaining JNI inside winit now, especially given that there doesn't appear to be any usefulness in exposing Android displays and modes via winit. If someone comes up with a tangible use-case, we can re-evaluate, perhaps by having your code in a separate crate that's more generically usable by the Android Rust ecosystem (as proposed above, kind of like a sibling to the ndk crate but for Java-only interfaces), and call that from winit instead 👍

MarijnS95

Android looks good!

src/platform_impl/android/mod.rs

src/changelog/unreleased.md

`VideoModeHandle::refresh_rate_millihertz()` and `bit_depth()` now return a `Option<NonZero*>`. `MonitorHandle::position()` now returns an `Option`. On Orbital `MonitorHandle::name()` now returns `None` instead of a dummy name.

Because we don't want to force all methods on `VideoModeHandle` to return `Option`, we decided to remove the already incomplete support in Android for both types.

daxpedda added the S - enhancement Wouldn't this be the coolest? label Jul 19, 2024

daxpedda requested review from jackpot51, notgull, kchibisov, madsmtm and MarijnS95 as code owners July 19, 2024 13:49

daxpedda commented Jul 19, 2024

View reviewed changes

src/platform_impl/apple/appkit/monitor.rs Outdated Show resolved Hide resolved

daxpedda mentioned this pull request Jul 19, 2024

Web: Implement MonitorHandle #3801

Merged

MarijnS95 reviewed Jul 19, 2024

View reviewed changes

src/platform_impl/android/mod.rs Outdated Show resolved Hide resolved

daxpedda marked this pull request as draft July 20, 2024 19:14

daxpedda force-pushed the monitor-bit-depth branch from d34b3dc to 521e071 Compare July 20, 2024 22:01

daxpedda changed the title ~~Add MonitorHandle::bit_depth()~~ Add MonitorHandle::current_video_mode() Jul 20, 2024

daxpedda marked this pull request as ready for review July 20, 2024 22:41

daxpedda force-pushed the monitor-bit-depth branch from 9b26bdd to 0f42269 Compare July 20, 2024 22:49

daxpedda mentioned this pull request Jul 20, 2024

Rename refresh rate #3806

Closed

daxpedda force-pushed the monitor-bit-depth branch from 0f42269 to 949e9d4 Compare July 23, 2024 13:58

kchibisov approved these changes Jul 23, 2024

View reviewed changes

src/changelog/unreleased.md Outdated Show resolved Hide resolved

daxpedda force-pushed the monitor-bit-depth branch from 949e9d4 to ac685d3 Compare July 23, 2024 17:59

kchibisov approved these changes Jul 23, 2024

View reviewed changes

daxpedda force-pushed the monitor-bit-depth branch from ac685d3 to 49d38ff Compare July 23, 2024 18:47

MarijnS95 approved these changes Jul 24, 2024

View reviewed changes

src/platform_impl/android/mod.rs Outdated Show resolved Hide resolved

src/platform_impl/android/mod.rs Outdated Show resolved Hide resolved

src/platform_impl/android/mod.rs Outdated Show resolved Hide resolved

src/platform_impl/android/mod.rs Show resolved Hide resolved

daxpedda requested review from kchibisov and MarijnS95 July 24, 2024 23:44

MarijnS95 approved these changes Jul 25, 2024

View reviewed changes

src/platform_impl/android/mod.rs Show resolved Hide resolved

src/platform_impl/android/mod.rs Outdated Show resolved Hide resolved

daxpedda force-pushed the monitor-bit-depth branch from 6d4bbd2 to a045f81 Compare July 25, 2024 11:47

daxpedda requested a review from MarijnS95 July 25, 2024 11:48

daxpedda force-pushed the monitor-bit-depth branch from a045f81 to dd70949 Compare July 25, 2024 11:50

daxpedda commented Jul 25, 2024

View reviewed changes

src/platform_impl/android/ffi.rs Outdated

Copy link

Member Author

daxpedda Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarijnS95 let me know if you think anything in here might actually panic.

kchibisov approved these changes Jul 25, 2024

View reviewed changes

src/monitor.rs Outdated Show resolved Hide resolved

daxpedda force-pushed the monitor-bit-depth branch from dd70949 to 694fa95 Compare July 25, 2024 12:36

daxpedda force-pushed the monitor-bit-depth branch from 694fa95 to f938fc5 Compare July 25, 2024 13:23

daxpedda force-pushed the monitor-bit-depth branch 2 times, most recently from b41ae81 to c48740a Compare July 26, 2024 14:00

MarijnS95 approved these changes Aug 2, 2024

View reviewed changes

src/platform_impl/android/mod.rs Show resolved Hide resolved

src/changelog/unreleased.md Show resolved Hide resolved

daxpedda added 2 commits August 8, 2024 00:47

Add MonitorHandle::current_video_mode()

2e53533

Remove MonitorHandle::size/refresh_rate_millihertz()

0ffcfd8

daxpedda force-pushed the monitor-bit-depth branch from c48740a to ae7550c Compare August 7, 2024 23:18

daxpedda added 2 commits August 8, 2024 01:29

Android: remove MonitorHandle support

3bab4ef

Because we don't want to force all methods on `VideoModeHandle` to return `Option`, we decided to remove the already incomplete support in Android for both types.

daxpedda force-pushed the monitor-bit-depth branch from ae7550c to 3bab4ef Compare August 7, 2024 23:30

daxpedda merged commit 3bab4ef into rust-windowing:master Aug 7, 2024
54 checks passed

daxpedda temporarily deployed to github-pages August 7, 2024 23:36 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `MonitorHandle::current_video_mode()` #3802

Add `MonitorHandle::current_video_mode()` #3802

daxpedda commented Jul 19, 2024 •

edited

Loading

kchibisov commented Jul 20, 2024

daxpedda commented Jul 20, 2024

daxpedda commented Jul 20, 2024 •

edited

Loading

kchibisov commented Jul 20, 2024

daxpedda commented Jul 20, 2024 •

edited

Loading

MarijnS95 left a comment

daxpedda commented Jul 24, 2024

daxpedda Jul 25, 2024

kchibisov Jul 25, 2024

daxpedda Jul 25, 2024

kchibisov Jul 25, 2024

daxpedda Jul 25, 2024

daxpedda Jul 25, 2024

kchibisov left a comment

daxpedda commented Jul 25, 2024

MarijnS95 commented Jul 25, 2024 •

edited

Loading

daxpedda commented Jul 25, 2024

MarijnS95 commented Jul 25, 2024

kchibisov commented Jul 26, 2024

daxpedda commented Jul 26, 2024 •

edited

Loading

daxpedda commented Jul 26, 2024

MarijnS95 commented Aug 2, 2024

MarijnS95 left a comment

Add MonitorHandle::current_video_mode() #3802

Add MonitorHandle::current_video_mode() #3802

Conversation

daxpedda commented Jul 19, 2024 • edited Loading

kchibisov commented Jul 20, 2024

daxpedda commented Jul 20, 2024

daxpedda commented Jul 20, 2024 • edited Loading

kchibisov commented Jul 20, 2024

daxpedda commented Jul 20, 2024 • edited Loading

MarijnS95 left a comment

Choose a reason for hiding this comment

daxpedda commented Jul 24, 2024

daxpedda Jul 25, 2024

Choose a reason for hiding this comment

kchibisov Jul 25, 2024

Choose a reason for hiding this comment

daxpedda Jul 25, 2024

Choose a reason for hiding this comment

kchibisov Jul 25, 2024

Choose a reason for hiding this comment

daxpedda Jul 25, 2024

Choose a reason for hiding this comment

daxpedda Jul 25, 2024

Choose a reason for hiding this comment

kchibisov left a comment

Choose a reason for hiding this comment

daxpedda commented Jul 25, 2024

MarijnS95 commented Jul 25, 2024 • edited Loading

daxpedda commented Jul 25, 2024

MarijnS95 commented Jul 25, 2024

kchibisov commented Jul 26, 2024

daxpedda commented Jul 26, 2024 • edited Loading

daxpedda commented Jul 26, 2024

MarijnS95 commented Aug 2, 2024

MarijnS95 left a comment

Choose a reason for hiding this comment

Add `MonitorHandle::current_video_mode()` #3802

Add `MonitorHandle::current_video_mode()` #3802

daxpedda commented Jul 19, 2024 •

edited

Loading

daxpedda commented Jul 20, 2024 •

edited

Loading

daxpedda commented Jul 20, 2024 •

edited

Loading

MarijnS95 commented Jul 25, 2024 •

edited

Loading

daxpedda commented Jul 26, 2024 •

edited

Loading