Skip to content

Add support for targetFrameRate and supportedFrameRates #1201

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

Merged
merged 10 commits into from
Jul 20, 2021
Merged

Add support for targetFrameRate and supportedFrameRates #1201

merged 10 commits into from
Jul 20, 2021

Conversation

cabanier
Copy link
Member

@cabanier cabanier commented May 31, 2021

Closes #1193


Preview | Diff

@cabanier cabanier requested review from toji and Manishearth May 31, 2021 20:00
@cabanier cabanier requested a review from thetuvix June 15, 2021 23:34
@cabanier
Copy link
Member Author

@thetuvix I updated my proposal.

index.bs Outdated
Comment on lines 889 to 891
The <dfn attribute for="XRSession">targetFrameRate</dfn> attribute reflects the internal [=XRSession/framerate=]. If the {{XRSession}} has no [=XRSession/framerate=], return `null`. The <dfn attribute for="XRSession">ontargetframeratechange</dfn> attribute is an [=Event handler IDL attribute=] for the {{targetframeratechange}} event type. If the {{XRSession}}'s frame rate is updated for any reason, it MUST [=apply the target frame rate=] with the new frame rate.

The <dfn attribute for="XRSession">supportedFrameRates</dfn> attribute returns a list of supported frame rates. This attribute is optional and MUST NOT be present for {{XRSessionMode/inline}} sessions. If the user agent can dynamically change the frame rate, this should be reflected as a frame rate of <code>0</code>. If the {{XRSession}} has an internal [=XRSession/framerate=], it also MUST support the {{XRSession/supportedFrameRates}} attribute.
Copy link
Contributor

Choose a reason for hiding this comment

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

If the {{XRSession}}'s frame rate is updated for any reason, it MUST [=apply the target frame rate=] with the new frame rate.

I don't fully follow when this applies.

For example, on HoloLens, the app cannot affect the frame rate in any way - in normal usage, the app's render rate will always be 60Hz. If the app misses frames, previous frames can be reprojected, but the app continues to be expected to keep up. Specifically, I would expect supportedFrameRates to only contain 60, because the app cannot request any other frame timings.

However, if the user starts a Mixed Reality Capture video recording, the system will drop to 30Hz all-up, and the app will start to receive 30Hz frame timing. Is this sentence saying that because the effective frame rate got updated "for any reason", that an ontargetframeratechange event must be fired and targetFrameRate must now return 30? This seems wrong, since 30 is not an element within supportedFrameRates.

If the user agent can dynamically change the frame rate, this should be reflected as a frame rate of 0.

This clause sets the wrong mental model for developers. This makes it sound like the target frame timing is fixed if the app chooses a non-0 value, and dynamic if the app chooses 0. In reality, things are always somewhat dynamic regardless - the UA can make use of the targetFrameRate hint provided by the page, but any given frame can have a duration that either matches or doesn't match that target for various platform-specific reasons.

It seems more flexible and consistent to treat targetFrameRate as a hint to the UA, with the page able to observe for any given frame what the effective frame rate/duration is, which can change whether the page has set a non-0 target or not.

Perhaps an answer here is to change the API to requestTargetFrameRate, which returns a promise that completes when the UA has taken action on the request to its best ability. No guarantee is made that the frame rate won't change further for other reasons after the request (e.g. halved due to system overlay, background app, this app not hitting framerate, thermal condition, etc.). However, if the app is waiting until the frame rate logically switches to 72Hz before starting video playback, the requestTargetFrameRate promise completion is the time to start. We could then choose to additionally surface on XRFrame a given frame's duration, without promising that it will be an element of supportedFrameRates, although perhaps it is cleaner first to see if devs are satisfied with just the promise.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the {{XRSession}}'s frame rate is updated for any reason, it MUST [=apply the target frame rate=] with the new frame rate.

I don't fully follow when this applies.

For example, on HoloLens, the app cannot affect the frame rate in any way - in normal usage, the app's render rate will always be 60Hz. If the app misses frames, previous frames can be reprojected, but the app continues to be expected to keep up. Specifically, I would expect supportedFrameRates to only contain 60, because the app cannot request any other frame timings.

supportedFrameRates is the list of framerates that the author can set. They are passed to the system and it will decide how (or if) to apply it. In the case of Hololens, this list could just 60 or it can decide not to support this function which means that the app can't control frame rate.

However, if the user starts a Mixed Reality Capture video recording, the system will drop to 30Hz all-up, and the app will start to receive 30Hz frame timing. Is this sentence saying that because the effective frame rate got updated "for any reason", that an ontargetframeratechange event must be fired and targetFrameRate must now return 30? This seems wrong, since 30 is not an element within supportedFrameRates.

Since the rate the author sets is just a hint for the compositor, it is free to change it.
Quest also has cases where it drops frame rates. For instance, if the user presses the Oculus button on the controller to bring up the menu, the rate is cut by half.

If the user agent can dynamically change the frame rate, this should be reflected as a frame rate of 0.

This clause sets the wrong mental model for developers. This makes it sound like the target frame timing is fixed if the app chooses a non-0 value, and dynamic if the app chooses 0. In reality, things are always somewhat dynamic regardless - the UA can make use of the targetFrameRate hint provided by the page, but any given frame can have a duration that either matches or doesn't match that target for various platform-specific reasons.

I'm happy to remove this special case.

It seems more flexible and consistent to treat targetFrameRate as a hint to the UA, with the page able to observe for any given frame what the effective frame rate/duration is, which can change whether the page has set a non-0 target or not.

Yes, this is what I intended to do. Maybe I need it to make it more explicit and also add a note?

Perhaps an answer here is to change the API to requestTargetFrameRate, which returns a promise that completes when the UA has taken action on the request to its best ability. No guarantee is made that the frame rate won't change further for other reasons after the request (e.g. halved due to system overlay, background app, this app not hitting framerate, thermal condition, etc.). However, if the app is waiting until the frame rate logically switches to 72Hz before starting video playback, the requestTargetFrameRate promise completion is the time to start. We could then choose to additionally surface on XRFrame a given frame's duration, without promising that it will be an element of supportedFrameRates, although perhaps it is cleaner first to see if devs are satisfied with just the promise.

I prefer to have an event instead of a promise to give the UA the flexibility to change the rate at any point, even without setting a target frame rate.
For instance, on Hololens this event would fire if Mixed Reality Capture starts.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the user agent can dynamically change the frame rate, this should be reflected as a frame rate of 0.

This clause sets the wrong mental model for developers. This makes it sound like the target frame timing is fixed if the app chooses a non-0 value, and dynamic if the app chooses 0. In reality, things are always somewhat dynamic regardless - the UA can make use of the targetFrameRate hint provided by the page, but any given frame can have a duration that either matches or doesn't match that target for various platform-specific reasons.

I'm happy to remove this special case.

Yea - I would eliminate ever enumerating 0 from supportedFrameRates. We still need to define the default null state for targetFrameRate (and allow apps to set it again when video playback ends), though that can just mean that the app is expressing no preference to the UA.

I prefer to have an event instead of a promise to give the UA the flexibility to change the rate at any point, even without setting a target frame rate.
For instance, on Hololens this event would fire if Mixed Reality Capture starts.

Got it - the name ontargetframeratechange suggested to me that the event could only return values from supportedFrameRates, and perhaps only triggered when the app had requested a change to the target frame rate.

Perhaps just call this onframeratechange to make clear that it's about the effective frame rate, not the target?

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it - the name ontargetframeratechange suggested to me that the event could only return values from supportedFrameRates, and perhaps only triggered when the app had requested a change to the target frame rate.

Perhaps just call this onframeratechange to make clear that it's about the effective frame rate, not the target?

In the case of Oculus, the system still draws at the full frame rate. It's just the browser that is asked to slow down. Given that, I'm leaning toward ontargetframeratechange but I would be fine either way.

@cabanier
Copy link
Member Author

@thetuvix I updated the spec text a bit per your comments.

@Maksims
Copy link

Maksims commented Jun 22, 2021

Not clear from the spec (perhaps only for me), but can UA change targetFrameRate? Or if it is set to a specific value, by the developer it will stay to that value, regardless if UA is capable of keeping the target frame rate or not.

If UA can internally change targetFrameRate. Then it might lead to confusion for the developer, who then will try to dispatch updateTargetFrameRate again and again, to try to override internal decision by UA.

Also, what happens in this scenario:

  1. App sets targetFrameRate to non-default 72 using updateTargetFrameRate.
  2. User clicks "video record".
  3. UA decides to drop to 30.
  4. UA dispatches ontargetframeratechange event and updates targetFrameRate?
  5. User finishes to record video.
  6. What happens now, does UA brings targetFrameRate back to 72 as App set before, dispatching ontargetframeratechange?

If the described scenario is good, then for a developer to know what targetFrameRate has been chosen, the developer needs to store on an application-level variable with chosen value (that was provided to updateTargetFrameRate). Not a big deal.

@cabanier
Copy link
Member Author

Not clear from the spec (perhaps only for me), but can UA change targetFrameRate? Or if it is set to a specific value, by the developer it will stay to that value, regardless if UA is capable of keeping the target frame rate or not.

It's a read only attribute so the author can't use it to change the rate. They have to use updateTargetFrameRate.

The value of targetFrameRate is the rate at which the author should redraw the scene. It can be influenced by updateTargetFrameRate but the UA is free to ignore that or update it for any other reason.

If UA can internally change targetFrameRate. Then it might lead to confusion for the developer, who then will try to dispatch updateTargetFrameRate again and again, to try to override internal decision by UA.

Yes, this is a feature for advanced users and I can see that if there is a chance of confusion if they don't understand how it works. Do you have a suggestion how we can avoid that?

Also, what happens in this scenario:

  1. App sets targetFrameRate to non-default 72 using updateTargetFrameRate.
  2. User clicks "video record".
  3. UA decides to drop to 30.
  4. UA dispatches ontargetframeratechange event and updates targetFrameRate?
  5. User finishes to record video.
  6. What happens now, does UA brings targetFrameRate back to 72 as App set before, dispatching ontargetframeratechange?

If the described scenario is good, then for a developer to know what targetFrameRate has been chosen, the developer needs to store on an application-level variable with chosen value (that was provided to updateTargetFrameRate). Not a big deal.

Yes, I would expect that it would happen like this. Should I add some wording to state that the XR System should try to match the frame rate that the author set?

@toji
Copy link
Member

toji commented Jun 22, 2021

The value of targetFrameRate is the rate at which the author should redraw the scene. It can be influenced by updateTargetFrameRate but the UA is free to ignore that or update it for any other reason.

Hm... this is confusing to me, because updateTargetFrameRate() is called by the developer to set the frame rate that the UA should aspire to reach when external factors and app performance allows, right? So to have the targetFrameRate be anything other than the value that the user set (or some initial default) seems wrong. If it's trying to report the actual current frame rate then a different name feels appropriate, though I don't know that I'd choose to report it on the session.

Should I add some wording to state that the XR System should try to match the frame rate that the author set?

That would be good. It seems like if possible the system should always be trying to hit the framerate that the developer specified, because presumably they did so because their content works best at that rate. Obviously sometimes that's not practical, but it should always be the goal.

@thetuvix
Copy link
Contributor

thetuvix commented Jun 22, 2021

The value of targetFrameRate is the rate at which the author should redraw the scene. It can be influenced by updateTargetFrameRate but the UA is free to ignore that or update it for any other reason.

Hm... this is confusing to me, because updateTargetFrameRate() is called by the developer to set the frame rate that the UA should aspire to reach when external factors and app performance allows, right? So to have the targetFrameRate be anything other than the value that the user set (or some initial default) seems wrong. If it's trying to report the actual current frame rate then a different name feels appropriate, though I don't know that I'd choose to report it on the session.

Agreed - perhaps just call the event onframeratechange and the attribute frameRate (or duration) to make clear that it's about the effective frame rate, not the target? Given that this changes dynamically and doesn't just read back out the user's request, does it make more sense on XRFrame?

@cabanier:
Relating to @Maksims' concern, can you elaborate on the feedback from the app team that was considering this API proposal before you added the event and the readout of the effective frame rate? What code is it that they intend to write?

My concern is that their code would look like this:

  1. App sets targetFrameRate to 72.
  2. App waits for ontargetframeratechanged event.
  3. UA sets effective frame rate of 36, because device is currently in some half-frame-rate state.
  4. App receives onframeratechanged event, checks to see if new frame rate is 72, notices it's not, and keeps waiting.
  5. Video playback never starts.

Another risk is if the app codes its onframeratechanged logic to just always start playback or such, because it was tested on a device that only changes frame rates upon the app's request - that app code would break on devices that change framerates for other reasons.

The advantage I see of making the update a promise-returning request function is that it would encourage apps that are waiting to trigger some activity to crisply use promise completion rather than polling for the exact value they want, which may never come. We could still have an event or polling API to let apps know what's going on if the frame rate changes for reasons other than a request, though I might lean towards the polling version, as the only real app logic we know of that would logically toggle something based on the frame rate change should really be tied to the effective time of app's own request, not any other system-driven changes.

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

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

Added some notes from the conversation on the call today.

Additionally, given the discussion about it being difficult to give developers actionable performance feedback, I wonder if it would be sensible to split this proposal into two parts:

  1. The supportedFrameRates attribute and the updateTargetFrameRate() method.
  2. Communicating the current effective frame rate and an event when it changes.

Mostly because it seems like the former is pretty well agreed upon at this point, while the latter still needs a bit of discussion and iteration.

index.bs Outdated
[SameObject] readonly attribute XRRenderState renderState;
[SameObject] readonly attribute XRInputSourceArray inputSources;

// Methods
undefined updateRenderState(optional XRRenderStateInit state = {});
undefined updateTargetFrameRate(float rate);
Copy link
Member

Choose a reason for hiding this comment

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

From call: Should be updated to return a Promise<void> that resolves when the requested update has take effect (even if the effective frame rate does not match at the time.) We probably want the promise to reject if a value that's not present in supportedFrameRates is passed.

It would also be good to determine a way to allow developers to revert to a state of "no preference". Passing 0 may be effective here, though we explicitly don't want supportedFrameRates to enumerate 0.

index.bs Outdated
@@ -614,11 +614,14 @@ enum XRVisibilityState {
[SecureContext, Exposed=Window] interface XRSession : EventTarget {
// Attributes
readonly attribute XRVisibilityState visibilityState;
readonly attribute float? targetFrameRate;
Copy link
Member

@toji toji Jun 22, 2021

Choose a reason for hiding this comment

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

From call: This value actually represents the effective frame rate of the UA/system, and should be distinct from the developer-provided target frame rate. The UA should never change the target framerate that the developer provides (unless it's been set to a "no preference" state, I suppose) thought it does not have to honor it.

I believe Alex mentioned something about exposing this on the XRFrame instead, which is something I would support as well.

The current target frame rate does not need to be reported by the session, since the developer set it and can track it on their own if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #1201 (comment)
The UA can change or ignore frame rate changes so that author will need to know.
They can keep track of what rate they set and compare it to the one on XRSession but I'm unsure what they would do with that information.

index.bs Outdated
@@ -636,6 +639,7 @@ enum XRVisibilityState {
attribute EventHandler onsqueezestart;
attribute EventHandler onsqueezeend;
attribute EventHandler onvisibilitychange;
attribute EventHandler ontargetframeratechange;
Copy link
Member

Choose a reason for hiding this comment

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

From call: Whatever naming we choose for the targetFrameRate attribute above should also be reflected here, as this event indicates changes to the effective frame rate and not the developer-provided target frame rate. (Though the effective framerate may change in response to a change to the target framerate.)

@cabanier
Copy link
Member Author

cabanier commented Jun 25, 2021

The value of targetFrameRate is the rate at which the author should redraw the scene. It can be influenced by updateTargetFrameRate but the UA is free to ignore that or update it for any other reason.

Hm... this is confusing to me, because updateTargetFrameRate() is called by the developer to set the frame rate that the UA should aspire to reach when external factors and app performance allows, right? So to have the targetFrameRate be anything other than the value that the user set (or some initial default) seems wrong. If it's trying to report the actual current frame rate then a different name feels appropriate, though I don't know that I'd choose to report it on the session.

No, this does not reflect the frame rate of the system; it's supposed to be the rate that the system wants the experience to run.
For instance, on Magic Leap the system wants the experience to run at 60 but the system compositor will attempt to run at 120. On Oculus during blur events, the system wants the experience to run at half the system framerate.

@cabanier
Copy link
Member Author

Hm... this is confusing to me, because updateTargetFrameRate() is called by the developer to set the frame rate that the UA should aspire to reach when external factors and app performance allows, right? So to have the targetFrameRate be anything other than the value that the user set (or some initial default) seems wrong. If it's trying to report the actual current frame rate then a different name feels appropriate, though I don't know that I'd choose to report it on the session.

Agreed - perhaps just call the event onframeratechange and the attribute frameRate (or duration) to make clear that it's about the effective frame rate, not the target?

Since it's the rate that the system wants the experience to run and not the system rate, we shouldn't call it so.
We could also totally ignore the distinction and just talk about the framerate as what your experience sees and ignore the sytem one. Maybe that would be more clear?
We would have to add text that the system frame rate can be different in that case.

Given that this changes dynamically and doesn't just read back out the user's request, does it make more sense on XRFrame?

There aren't many attributes on XRFrame. Also, if it's put on XRFrame you can't control or read the frame rate outside of a Raf.

@cabanier:
Relating to @Maksims' concern, can you elaborate on the feedback from the app team that was considering this API proposal before you added the event and the readout of the effective frame rate? What code is it that they intend to write?
...

We covered this in the meeting and I will add a promise in addition to the event.

@cabanier
Copy link
Member Author

Added some notes from the conversation on the call today.

Additionally, given the discussion about it being difficult to give developers actionable performance feedback, I wonder if it would be sensible to split this proposal into two parts:

  1. The supportedFrameRates attribute and the updateTargetFrameRate() method.
  2. Communicating the current effective frame rate and an event when it changes.

Mostly because it seems like the former is pretty well agreed upon at this point, while the latter still needs a bit of discussion and iteration.

I agree. I'll update the PR with the promise

@Maksims
Copy link

Maksims commented Jun 28, 2021

Do we really need an event/promise here? What are real cases where the developer needs to know that UA decided to render something at a different frame rate? The actual frame rate is well sufficient for it.

I believe UA's ability to change targetFrameRate attribute - is definitely confusing to the developer, and can be considered as a "magic behaviour" which is hard to communicate from an API.

Event when change happens due to UA, might be also not necessary.
If this scenario is valid:

  1. App sets targetFrameRate to non-default 72.
  2. User clicks "video record".
  3. UA decides to drop frame rate to 30. (not changing targetFrameRate)
  4. User finishes to record video.
  5. UA tries to respect targetFrameRate, attempting to render at 72.
  6. Application tracks actual frame rate (not target), and adapts accordingly.

Then what value does event/promise brings? Or even changing targetFrameRate by UA.
targetFrameRate is a hint to UA, and cannot be a direct instruction due to complex scenarios as mentioned before.

If UA changes the frame rate, this has nothing to do with the developer wanting of frame rate to hit the target.
Such changes in frame rate are the same as frame rate changes due to performance reasons, and applications should use the actual frame rate for logic changes (speeds, delta times, progressive graphics, etc).

perhaps just call the event onframeratechange and the attribute frameRate (or duration) to make clear that it's about the effective frame rate, not the target? Given that this changes dynamically and doesn't just read back out the user's request, does it make more sense on XRFrame?

This might lead to confusion, as from the naming, a developer could expect it to fire every frame, as almost no frame framerate is the same. So when it changes due to performance reasons from 60 to 59, will that throw an event, that is a "frame rate change".

The advantage I see of making the update a promise-returning request function is that it would encourage apps that are waiting to trigger some activity to crisply use promise completion rather than polling for the exact value they want, which may never come. We could still have an event or polling API to let apps know what's going on if the frame rate changes for reasons other than a request, though I might lean towards the polling version, as the only real app logic we know of that would logically toggle something based on the frame rate change should really be tied to the effective time of app's own request, not any other system-driven changes.

The promise might not do a favour here, as due to UA logic: the set target frame rate might never be respected.
The developer definitely should not make an assumption that by setting target frame rate, it will ever get there or even be respected by UA. So targetFrameRate - is only a hint for UA to try to respect that, but not obliged.

@cabanier
Copy link
Member Author

Do we really need an event/promise here? What are real cases where the developer needs to know that UA decided to render something at a different frame rate? The actual frame rate is well sufficient for it.

The feature is mainly meant for performance tuning.
If the developers is made aware of a chance in framerate (up or down) and knows the load on the system, they can adjust their rendering pipeline or game logic.
Without an event, the developer would have to poll to detect changes.
Without a promise, the developer would also have to poll and might wait indefinitely if the frame rate didn't change.

...
If this scenario is valid:

  1. App sets targetFrameRate to non-default 72.
  2. User clicks "video record".
  3. UA decides to drop frame rate to 30. (not changing targetFrameRate)
  4. User finishes to record video.
  5. UA tries to respect targetFrameRate, attempting to render at 72.
  6. Application tracks actual frame rate (not target), and adapts accordingly.

Then what value does event/promise brings? Or even changing targetFrameRate by UA.
targetFrameRate is a hint to UA, and cannot be a direct instruction due to complex scenarios as mentioned before.

Correct. It is a hint to help with performance/media playback

If UA changes the frame rate, this has nothing to do with the developer wanting of frame rate to hit the target.
Such changes in frame rate are the same as frame rate changes due to performance reasons, and applications should use the actual frame rate for logic changes (speeds, delta times, progressive graphics, etc).

Indeed. Except in this case, the logic to determine the frame rate changes are made by the developer.

perhaps just call the event onframeratechange and the attribute frameRate (or duration) to make clear that it's about the effective frame rate, not the target? Given that this changes dynamically and doesn't just read back out the user's request, does it make more sense on XRFrame?

This might lead to confusion, as from the naming, a developer could expect it to fire every frame, as almost no frame framerate is the same. So when it changes due to performance reasons from 60 to 59, will that throw an event, that is a "frame rate change".

I'm unsure if it would be confusing but I agree it makes more sense on XRSession.

The advantage I see of making the update a promise-returning request function is that it would encourage apps that are waiting to trigger some activity to crisply use promise completion rather than polling for the exact value they want, which may never come. We could still have an event or polling API to let apps know what's going on if the frame rate changes for reasons other than a request, though I might lean towards the polling version, as the only real app logic we know of that would logically toggle something based on the frame rate change should really be tied to the effective time of app's own request, not any other system-driven changes.

The promise might not do a favour here, as due to UA logic: the set target frame rate might never be respected.
The developer definitely should not make an assumption that by setting target frame rate, it will ever get there or even be respected by UA. So targetFrameRate - is only a hint for UA to try to respect that, but not obliged.

Yes, I also didn't think the promise was absolutely necessary but other people felt strongly that is was.
Given that it's a reasonable request and avoids potential hangs if the user doesn't use the API correctly, I had no objection to adding it.

@Maksims
Copy link

Maksims commented Jun 28, 2021

If the developers is made aware of a chance in framerate (up or down) and knows the load on the system, they can adjust their rendering pipeline or game logic.

They are. By tracking actual frame rate (time passed since last frame).
I doubt targetFrameRate has anything to do with tracking actual performance. It is more of using capabilities of hardware, and their special behaviours.

I'm afraid some people got confused by specifics of this proposal. Mixing "a hint for a desired target frame rate" and "actual frame rate". What was also suggested by @toji.

What would be real world use case for tracking target frame rate changes?

@cabanier
Copy link
Member Author

If the developers is made aware of a chance in framerate (up or down) and knows the load on the system, they can adjust their rendering pipeline or game logic.

They are. By tracking actual frame rate (time passed since last frame).

Unfortunately, that data is very noisy so it's not a good indication of the actual framerate.

I doubt targetFrameRate has anything to do with tracking actual performance. It is more of using capabilities of hardware, and their special behaviours.

It's not about tracking performance. This feature is designed to relieve the load on the system for experience that can't make framerate or have headroom to have a higher framerate.

I'm afraid some people got confused by specifics of this proposal. Mixing "a hint for a desired target frame rate" and "actual frame rate". What was also suggested by @toji.

What would be real world use case for tracking target frame rate changes?

I noted this earlier.
Basically, there's the frame rate that the system is running (which is not observable to the author) and the one that the system wants the experience to run. They do not have to be the same so the targetFrameRate was introduced.
However, it seems that this is causing more confusion so maybe it's better to drop the target part of the name.

@Maksims
Copy link

Maksims commented Jun 29, 2021

Unfortunately, that data is very noisy so it's not a good indication of the actual framerate.

Developers smooth out the data through previous frames. And make decisions of that.

However, it seems that this is causing more confusion so maybe it's better to drop the target part of the name.

That definitely will make more confusion. As it will not be an actual frame rate.

I'm really keen to see an actual example of using events/promise, for updating frame rate, that is applicable to real-world applications.

@cabanier
Copy link
Member Author

Unfortunately, that data is very noisy so it's not a good indication of the actual framerate.

Developers smooth out the data through previous frames. And make decisions of that.

Those decisions would be incorrect. :-(
Measuring the delta between frames and the length of the Raf will not give you the correct results, even with smoothing.

However, it seems that this is causing more confusion so maybe it's better to drop the target part of the name.

That definitely will make more confusion. As it will not be an actual frame rate.

Does it matter it is not the actual framerate? All the author should care about is how quickly they should produce frames.

I'm really keen to see an actual example of using events/promise, for updating frame rate, that is applicable to real-world applications.

As opposed to polling the rate?

@thetuvix
Copy link
Contributor

thetuvix commented Jun 29, 2021

I noted this earlier.
Basically, there's the frame rate that the system is running (which is not observable to the author) and the one that the system wants the experience to run. They do not have to be the same so the targetFrameRate was introduced.
However, it seems that this is causing more confusion so maybe it's better to drop the target part of the name.

There are various frame rates we could refer to in various places in the API here: (I'll use the names that I've been hearing in my own head)

  1. The nominal frame rate: This is the rate at which the UA is asking the app to render frames if it maintains nominal performance. Apps that miss frames may not end up actually getting rAFs this many times per second, but that is what the system is aiming to achieve.
  2. The effective frame rate: This is a performance measurement of how many rAFs the app is actually managing to push through each second. This would fluctuate based on the app hitting or missing the UA's frame timing.
  3. The target frame rate: This is the app's hint to the UA on what nominal frame rate it would prefer to target, either because it knows it has heavy graphics, or because it is trying to match a multiple of a video frame rate.
  4. The display frame rate: This is the actual rate at which frames are drawn to the physical display, which can be reprojected at 2x/3x/4x from the app's nominal frame rate. This is a hardware implementation detail that probably has no reason to be exposed to the app.

I believe the confusion we've had in this discussion is whether 1 or 3 makes sense as "target frame rate". Happy to just call the function requestFrameRate() to avoid having to find a name for 3 that means the same thing to everyone.

For an additional polling/event API to read 1, OpenXR worked around this whole naming concern around "frame rate" by calling it XrFrameState::predictedDisplayPeriod. Right next to this is predictedDisplayTime, which tells you the forward-predicted midpoint of the interval during which this next frame will be displayed. predictedDisplayPeriod then predicts how long this next frame will stay visible to the user - dividing 1 second by this predictedDisplayPeriod yields you the nominal frame rate. Perhaps relating the readout half of the API to something other than "frame rate" can help unlock less ambiguous naming.

The promise might not do a favour here, as due to UA logic: the set target frame rate might never be respected.
The developer definitely should not make an assumption that by setting target frame rate, it will ever get there or even be respected by UA. So targetFrameRate - is only a hint for UA to try to respect that, but not obliged.

The fact that the UA may never achieve the app's requested target frame rate is exactly the reason for the promise - apps that use it are less likely to fall into various traps:

  • With polling, the app's only signal that the UA has finished attempting to adopt the target frame rate is to see if that exact frame rate shows up as the current rate. If for some reason, frame rate switches are suppressed at the moment, the app may accidentally wait forever.
  • With an event, the app's only signal that the UA has finished attempting to adopt the target frame rate is to see if the onframeratechanged event fires. However, because this event fires on any frame rate change, it is really about the frame rate value becoming different - it would be strange for it to fire if the frame rate is staying the same due to some system condition limiting the device to a max frame rate. Therefore, if the device is in that condition, the event may not fire and the app may get stuck.
  • With a promise, the app can be signaled specifically when that particular target frame rate change request completes. The change in target frame rate might be completed without actually changing the nominal frame rate, if some system condition is suppressing that particular frame rate. (e.g. a 30fps limitation during video recording or when in the system menu) However, the target frame rate change has logically completed, and the promise can thus be completed to allow app logic such as video playback to proceed. Some time later, the app's effective frame rate may then reach the new target frame rate, e.g. once that system condition resolves (system menu is closed, video recording ends).

There aren't many attributes on XRFrame. Also, if it's put on XRFrame you can't control or read the frame rate outside of a Raf.

I agree that XRSession is the right home for the requestTargetFrameRate promise-returning function, since that represents changes in logical app state (the user is playing a video), rather than per-frame render details, which app logic will want to access outside of a rAF. If most developers doing video frame rate matching use that promise and never poke at any frame rate readout API, we did our API design job correctly. When such a frame rate request API is exposed in OpenXR, I would indeed expect it to take an XrSession handle as its first parameter.

Developers for whom the requestTargetFrameRate promise is insufficient are likely doing fancier per-frame performance analysis to determine when to drop or raise their target frame rate. Those developers are already reasoning about the details of individual rAFs, and so it seems reasonable to me to expose that kind of extra nominal frame rate readout API on XRFrame. It's worth noting that in OpenXR where this exists today, it's exposed on the equivalent of XRFrame as XrFrameState::predictedDisplayPeriod.

@Manishearth Manishearth added this to the Pre-CR milestone Jun 29, 2021
@cabanier
Copy link
Member Author

Thanks for this great writeup!
Should I add the different framerate distinctions to the spec? I can almost copy/paste it I think :-)

Are you suggesting that we should drop the target part from the attributes and the request function?

index.bs Outdated
1. The [=XR Compositor=] MAY use |rate| to update the {{XRSystem}}'s frame rate.
1. Set the |session|'s internal [=XRSession/framerate=] to the {{XRSystem}}'s recommended frame rate. It is up to the {{XRSystem}} to determine this recommended frame rate.
1. [=Queue a task=] to perform the following steps:
1. Await until the {{XRSystem}}'s frame rate has taken effect.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should perhaps be more precise here that we are waiting until the XR compositor makes all changes it intends to make at this time in response to the request. This accommodates the case where the device is already at 30fps and stays there - no change was made, but the XR compositor is ready to respect that request when the temporary condition is lifted.

Suggested change
1. Await until the {{XRSystem}}'s frame rate has taken effect.
1. If the [=XRSession/framerate=] was changed, await until the {{XRSystem}}'s frame rate has taken effect.

@thetuvix
Copy link
Contributor

Are you suggesting that we should drop the target part from the attributes and the request function?

It might be worth trying that on for size, to see if it resolves some of the differing interpretations we all had where some folks (including me) assumed target meant the app's hint itself.

In the spec language, we could still define targetFrameRate or frameRateHint as the explicit internal spec variable for the app's specified hint on the XRSession - this could let us make clear the idea that the hint is a durable request to the XR Compositor until unset, regardless of whether the compositor happens to be able to fully respect the change at this very moment or not.

@Maksims
Copy link

Maksims commented Jun 30, 2021

It would be still good to see a real-world concept for a promise/event handling and application logic. What would be the use cases.

As discussion maybe went into promise/event path due to misunderstanding of the scope of this proposal (targetFrameRate, not actual frameRate).

@thetuvix
Copy link
Contributor

The primary use case I recall hearing on the call:

  1. User is in a VR app at the default frame rate for the device (60, 90 or 120Hz).
  2. User selects a video to play, which happens to be a 24Hz video.
  3. The app fetches supportedFrameRates and finds that the highest multiple of 24 in the list is 72.
  4. The app calls requestTargetFrameRate(72) to set the device into a mode optimized for 72Hz (and thus 24Hz) content to be displayed.
  5. The app awaits the completion of the promise returned by requestTargetFrameRate.
  6. The app starts playing the video, having ensured that video playback only started after any mode switches that might have temporarily blanked out the display.

For this scenario, the promise approach is cleaner than an event or polling approach for the reasons discussed above.

Note that if this was the only scenario, it would be cleaner for the function to be requestFrameRateMultiple(24) or such, to give the UA maximum flexibility in selecting what may not quite be multiples to optimize video playback. However, this design is going with a more general approach as there are other perf tuning scenarios that the devs @cabanier has worked with have in mind. It would be good to check the design we're landing on by writing out those other use cases too.

@Maksims
Copy link

Maksims commented Jun 30, 2021

  • The app awaits the completion of the promise returned by requestTargetFrameRate.
  • The app starts playing the video, having ensured that video playback only started after any mode switches that might have temporarily blanked out the display.

That would not be possible, due to uncertainty of a platform, which cannot guarantee frameret set.
Also, if UA will promise to respect targetFrameRate eventually it will be not a great UX with such uncertainty, that will lead to unecessary pause for video playing.
This is similar to buffering videos: it is best to start playing video at lower quality, and then improve.

In such scenario if promise/event happens always, it will happen next frame? As "UA acknowledged a hint".
But if it does not guarantee to respond, then developer needs to create parallel timer, and trigger video playback on timeout of awaiting a promise/event?

@thetuvix
Copy link
Contributor

thetuvix commented Jul 1, 2021

The promise would be signaled when the request is completed, either honoring the request or not:

  • If the UA can honor the frame rate request at this moment, change the device's nominal frame rate to the new target frame rate, and signal the promise once the change is complete. (within 1 second typically)
  • If the UA cannot make any change at the moment, record the new target frame rate to be respected once possible, and signal the promise right away.

The purpose of the promise is to be an attractive alternative to the codepaths apps would be tempted to write without it - unlike the promise, those alternate codepaths are wrong on some devices.

For example, without this promise, the app is likely to instead write code that polls to see when their specific target frame timing is actually achieved - on devices temporarily in some 30fps mode, they could be stuck waiting for minutes. The promise gives the UA a chance to signal that the requested frame rate has been logically achieved: the browser has taken note of the hint and may apply it later, but no change is coming within the next 1 second, and so you should just start your activity now.

The app benefit of waiting to start video playback until the frame rate change has completed is to avoid playing the first few seconds of the video while the display is blanked out actively changing frame rates. If this was simply a case of starting while lower-res like with streaming video, the user could at least tell what's going on in the video for those few seconds - however, if some VR device needs to blank to change frame rates, the user might miss part of the video entirely.

@cabanier
Copy link
Member Author

cabanier commented Jul 1, 2021

FYI I've been publishing the wip on https://cabanier.github.io/webxr/

@Maksims
Copy link

Maksims commented Jul 1, 2021

  • (within 1 second typically)

Does this need to be specified by the spec? How does the developer know how long it can take? If there is a guarantee of some time limit, or it is up to a developer to code their own logic with timeouts?

Is blanking - is a current behaviour by existing platforms when changing the screen refresh rates?

@cabanier
Copy link
Member Author

cabanier commented Jul 1, 2021

  • (within 1 second typically)

Does this need to be specified by the spec? How does the developer know how long it can take? If there is a guarantee of some time limit, or it is up to a developer to code their own logic with timeouts?

The spec is silent how long it takes for any promise to be fulfilled (ie requestReferenceSpace and requestSession).

Is blanking - is a current behaviour by existing platforms when changing the screen refresh rates?

What is "blanking"?

@Maksims
Copy link

Maksims commented Jul 1, 2021

What is "blanking"?

@thetuvix:

however, if some VR device needs to blank to change frame rates, the user might miss part of the video entirely.

I assume it is when the hardware is changing resolution/refresh rate, it can lead to the screen blinking to black and back.

@thetuvix
Copy link
Contributor

thetuvix commented Jul 2, 2021

The spec is silent how long it takes for any promise to be fulfilled (ie requestReferenceSpace and requestSession).

Indeed - and in the case of a permissions prompt, a function like requestSession may indeed take minutes for its promise to complete.

In this case, we likely want to make a stronger spec guarantee around requestFrameRate - I'm not sure if we should encode some exact threshold like 1 second - instead, we can just distinguish whether the UA is able to start switching the frame right immediately or not:

  • UA can switch to the new frame rate in the next frame: Promise should complete next frame.
  • UA can start switching frame rate immediately but will take 2 seconds to finish: Promise should wait the 2 seconds to complete.
  • UA can't start switching frame rate until later because of some ongoing blocking condition (e.g. system menu is displayed): Promise should complete immediately.

Basically, we can write the spec language for the promise qualitatively around whether the XR Compositor is able to start effecting the frame rate change now (await that completion) or not yet (complete now).

I assume it is when the hardware is changing resolution/refresh rate, it can lead to the screen blinking to black and back.

Yes - that's what I'd meant. Displays are not always able to change refresh rates seamlessly with no visual artifacts - I recall from @cabanier that the app developers he was working with had a concern that they wanted to hold off on video playback until the transition to the new frame rate was finished. He can share more info on the specifics of what they were exactly worried about there.

@cabanier
Copy link
Member Author

cabanier commented Jul 8, 2021

@thetuvix what do you think if the latest PR?
@toji is there an issue with merging this? Should we discuss this in a meeting?

Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

I personally think this is pretty good now, but we should wait for Brandon's approval as well.

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

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

This looks really good and I'm eager to approve it, there's just a couple more (hopefully small) tweaks I'd like to see.

index.bs Outdated

<div class="algorithm" data-algorithm="update-frame-rate">

The <dfn method for="XRSession">updateFrameRate(|rate|)</dfn> method passes the [=target frame rate=] |rate| to the {{XRSession}}.
Copy link
Member

Choose a reason for hiding this comment

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

I feel like there's a bug here because while this description states that the method is being passed the target framerate the algorithm doesn't actually set or store that anywhere, which makes it appear that this is setting the internal framerate directly with an opportunity for the compositor to veto, at which point any target frame rate expressed by the developer would effectively be lost.

Can we add an explicit target framerate slot to the session, like the existing internal framerate, that this algorithm sets to the value passed to updateFrameRate() as long as it's one of the supported framerates? That way it's made explicit that the target frame rate hint provided by developers doesn't simply vanish the next time the system updates it's internal framerate.

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like there's a bug here because while this description states that the method is being passed the target framerate the algorithm doesn't actually set or store that anywhere, which makes it appear that this is setting the internal framerate directly with an opportunity for the compositor to veto, at which point any target frame rate expressed by the developer would effectively be lost.

Does the line [=Apply the frame rate=] with |rate|, |promise| and |session| not use this rate?
It's indeed true that the value that the developer set is indeed lost. I believe we discussed that they could store this themselves if they wanted to

Can we add an explicit target framerate slot to the session, like the existing internal framerate, that this algorithm sets to the value passed to updateFrameRate() as long as it's one of the supported framerates? That way it's made explicit that the target frame rate hint provided by developers doesn't simply vanish the next time the system updates it's internal framerate.

Yes, that sounds reasonable.

Copy link
Member

Choose a reason for hiding this comment

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

Does the line [=Apply the frame rate=] with |rate|, |promise| and |session| not use this rate?

It only uses it when the developer initially calls updateFrameRate() and will be overwritten the next time the system has to adjust the rate for any reason.

It's indeed true that the value that the developer set is indeed lost. I believe we discussed that they could store this themselves if they wanted to.

I don't care about being able to report the value the user set back to them. They definitely can track that themselves if they want. I'm just pointing out that the spec doesn't appear to require the implementation to remember the user-set target, which also implies that it wouldn't factor into any future frame rate adjustments.

I can see developers reading this and assuming that their expressed target framerate gets lost after the next time that onframeratechange is called, which may encourage them to constantly reset the target frame rate in response to onframeratechange events, which is probably not a pattern we want to encourage.

index.bs Outdated
[SameObject] readonly attribute XRRenderState renderState;
[SameObject] readonly attribute XRInputSourceArray inputSources;

// Methods
undefined updateRenderState(optional XRRenderStateInit state = {});
Promise&lt;undefined&gt; updateFrameRate(float rate);
Copy link
Member

Choose a reason for hiding this comment

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

I know this was discussed earlier in the thread, but @Manishearth and I would prefer that this function be named updateTargetFrameRate() to disambiguate which frame rate it is setting and make it explicit that it may not be reflected in the frameRate attribute.

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

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

LGTM! Fixed one minor issue via a suggestion.

This reads much more clearly to me now, thank you for taking the time to update it.

@toji toji merged commit 75fdd84 into immersive-web:main Jul 20, 2021
@cabanier cabanier deleted the framerate branch July 21, 2021 03:55
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.

Allow session to specify refresh rate
5 participants