Skip to content
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

Bring back requestViewportScaling? #1091

Open
klausw opened this issue Jun 26, 2020 · 18 comments
Open

Bring back requestViewportScaling? #1091

klausw opened this issue Jun 26, 2020 · 18 comments
Milestone

Comments

@klausw
Copy link
Contributor

klausw commented Jun 26, 2020

Initial versions of the WebXR spec had included a requestViewportScaling API that allows applications to use a subset of the overall framebuffer for rendering.

Issue #617 had requested deferring it to simplify the initial WebXR API, but it sounded as if people were generally not opposed to the API as such and would be open to bringing it back at a later time.

In the meantime, we've gotten feedback that performance scaling can be tricky especially for smartphone AR applications, where render costs can rise dramatically when people move close to complex AR objects. For example, uses aggressive autoscaling in non-XR canvas mode, and the developers would be very interested in having similar functionality available in WebXR AR mode.

Can we revisit this to see if it would make sense to reintroduce this API?

For reference, the removal was in #631 .

@klausw
Copy link
Contributor Author

klausw commented Jun 26, 2020

/agenda

@probot-label probot-label bot added the agenda Request discussion in the next telecon/FTF label Jun 26, 2020
@thetuvix
Copy link
Contributor

As a note, HoloLens 1 originally had only a latent request model here (setting HolographicCamera.ViewportScaleFactor), where you'd ask for a particular scale and be given in future frames some similar scale you must use, perhaps rounded to the nearest 4 pixels or such. Since then, our native API moved to support arbitrary viewport overriding while you're already in a frame (HolographicCameraPose.OverrideViewport) and OpenXR now requires that runtimes support arbitrary an imageRect during layer submission.

Allowing apps to render to an arbitrary subset of the framebuffer and simply tell the UA what portion they're using that frame seems like a reasonable design to me if no other vendors will hit gotchas there.

@Manishearth
Copy link
Contributor

Somewhat relevant: In my implementation of FPO views in Servo I downscale them by a constant for performance. It would be interesting if this could be exposed and overrideable through such an API, though I don't think the current proposal is for a per-view thing.

@Yonet Yonet removed the agenda Request discussion in the next telecon/FTF label Jul 1, 2020
@Manishearth
Copy link
Contributor

@klausw @toji can you jot down what the conclusion on this was from the call?

@toji toji added this to the Future milestone Jul 10, 2020
@toji
Copy link
Member

toji commented Jul 10, 2020

Marking as future because while this did see some interest on the call and so we should continue looking into it, it's something that can be brought in additively later with no complications. I'll try and capture some more of the call discussion in another comment.

@klausw
Copy link
Contributor Author

klausw commented Jul 10, 2020

In summary, the rough consensus seemed to be that people seemed generally OK with a mechanism for apps to request use of under-sized viewports. There didn't seem to be specific concerns that implementations would be unable to accommodate this, but it seems safer to provide a mechanism for UAs to decline the request or to adjust the resulting value if needed. Providing GPU metrics to applications would make this more useful, but it seems worth experimenting with viewport scaling even in the absence of that.

Meeting minutes: https://www.w3.org/2020/06/30-immersive-web-minutes.html

Background: the current framebufferScaleFactor is set when creating the XRWebGLLayer, and cannot be changed for an existing layer. It's possible to change the scale at runtime by creating a new XRWebGLLayer and setting it as a new base layer via XRSession's updateRenderState, but this generally involves reallocations in the graphics pipeline. Applications should expect dropped frames when changing this, and it's not suitable as a per-frame dynamic scaling mechanism.
TODO: add a note to the spec to state this explicitly?

The framebuffer is carved up into views, for example one view per eye for a typical VR display. Currently, this viewport allocation is fully controlled by the UA.

In the previous WebVR API, applications had full control over viewport allocation and could request using only subsets of the framebuffer. Some implementations (Hololens?) had concerns that full application control could be inefficient or problematic. UA control could enforce constraints such as aligning to a preferred pixel grid.

According to Microsoft, dynamic viewport scaling hadn't seen much uptake by developers, including in corresponding native APIs, where developers often chose a fixed size and kept that for the duration of the experience. One concern was that it was difficult for applications to do smart scaling if they don't have any metrics about GPU utilization or time budgets.

Currently, developers targeting phone AR, for example , have expressed strong interest in dynamic scaling. In phone AR, even a rudimentary approach that accepts dropped frames seemed useful, since dropping frames isn't nearly as impactful as for a VR experience.

According to @thetuvix, OpenXR supports full viewport control, so implementations based on that should easily be able to do arbitrary viewports. Developers were inappropriately using framebuffer scaling, and viewport scaling would be much more efficient. It would be nice if there were a "keep me at framerate" flag in engines, but that seems rare, but it would be useful to have a way to prototype that.

The viewport scale mechanism should take effect on the same frame where it was requested. If a UA can't do this directly, they could use workarounds such as adjusting reprojection to compensate, but this must not cause visible dropped frames (especially if it would cause black flashes or similar) to avoid user discomfort if apps change the scale frequently.

The UA could provide a recommended viewport scale for each frame, for example based on a heuristic using UA-internal metrics. Applications could opt in to dynamic scaling by using that as-is, or apply clamping or other adjustments as needed, i.e. a lower bound to ensure small text remains readable. UAs could also potentially do automatic viewport scaling by default with an opt-out mechanism, but this risks surprising applications.

@klausw
Copy link
Contributor Author

klausw commented Jul 10, 2020

One more thing - I think there was also interest in applying a custom viewport scale to third-person views. For this use case, the requested and/or recommended viewport size should be a per-view property, not per-frame, so that the views can be scaled independently.

@klausw
Copy link
Contributor Author

klausw commented Jul 17, 2020

Just to make the proposal a bit more concrete, how would something like this sound?

  • add a new XRWebGLLayer.getScaledViewport(XRView, scale) method, with 0 < scale <= 1.0. (Viewports can only be scaled down, not up.) UAs are free to clamp or modify this value, including ignoring the scaling entirely.

  • add a readonly recommendedViewportScale property to XRView where UAs can optionally provide a suggested value.

Alternatively, assuming people are OK with overloaded methods in the API, this could be done by adding a new optional scale parameter to the existing getViewport method, treating an undefined scale as 1.0 for compatibility with current behavior.

In code, based on example 5 from https://immersive-web.github.io/webxr/#xrviewport-interface :

  for (xrView of viewer.views) {
    let xrViewport = xrWebGLLayer.getViewport(xrView);
    gl.viewport(xrViewport.x, xrViewport.y, xrViewport.width, xrViewport.height);

Scaled viewports:

let xrViewport = xrWebGLLayer.getScaledViewport(xrView, xrView.recommendedViewportScale || 1.0);

// Variant: overload getViewport with an extra parameter
let xrViewport = xrWebGLLayer.getViewport(xrView, xrView.recommendedViewportScale || 1.0);

// Variant: an undefined value is treated as 1.0
let xrViewport = xrWebGLLayer.getViewport(xrView, xrView.recommendedViewportScale);

Of course, applications could use their own logic to calculate a scale factor from scratch, or base it on clamping/modifying the UA-provided recommended scale factor.

Allowing the UA's flexibility to ignore or modify the requested scale factor is helpful for several reasons. It allows the UA to align the viewports to pixel boundaries where this improves efficiency, and avoids the need for complicated rectangle area allocation tracking. For example, if an application for some reason calls getScaledViewport multiple times for one view in the same frame with different scales, or with inconsistent per-eye values, the UA would be free to use its preferred value and ignore values inconsistent with its current viewport allocations.

@elalish
Copy link

elalish commented Jul 21, 2020

This sounds great to me; I like the overload version a little better, but it's not a strong opinion. Out of curiosity, in what circumstances would recommendedViewportScale != 1.0?

In any case, this scaling is critical for smooth frame rates, especially since I've observed frame rates dropping by more than 4x just by walking up to an AR model. The shader only runs on the blocks of pixels the rendering covers, which is usually not all that much, but when you get close, it can quickly become a disaster. In <model-viewer> we can keep the frame rate up using viewport scaling on our 3D view simply by targeting a frame rate slightly lower than the max. It is working well in practice, and once this API is available in WebXR, we'll do the same there.

@thetuvix
Copy link
Contributor

I'm generally comfortable with keeping the UA in the loop to allow runtimes to give back a slightly tweaked viewport (or even ignore the request and just return the original viewport) if needed.

One gotcha with the particular design proposed above is that it gives side effects to the call to getViewport. Today, you can call getViewport(view) however often you like within the code you run for a given frame and it just gives you that view's viewport - the reason it's a function is purely to index the available viewports by the view parameter. With this change, each getScaledViewport call would get you a scaled viewport, but what then chooses the official viewport that the UA will read from at the end of the frame? If it's simply the latest call to getViewport in a given frame, that seems both subtle and fragile if multiple components expect that they can just fetch the viewport as needed without coordination.

It may make more sense to have a requestViewportScale function here to make the active nature of the request more clear. You would then call getViewport(view) as often as needed afterwards to make use of the updated viewport you're rendering into. So long as some early part of the render loop is the only code that calls the request function, different render components of the app can later freely call getViewport as needed without coordinating with one another.

That approach still has the subtlety that the app must call getViewport can change out from under a render component that might not expect it if requestViewportScale is called halfway through the frame. However, that might be acceptable.

Perhaps more importantly, if most platforms just blindly accept any viewport you request, some apps may not actually go through the extra bother of calling getViewport to find out the UA's effective viewport and just run with the exact viewport they requested. If that only breaks on a small number of devices, many apps may mess that up, and so those devices may feel pressured to just allow arbitrary viewport requests. If we take another look and are comfortable just requiring all UAs on all platforms to allow any arbitrary viewport that fits in the framebuffer, we could just make this setViewport(view).

@klausw
Copy link
Contributor Author

klausw commented Jul 24, 2020

@elalish wrote:

This sounds great to me; I like the overload version a little better, but it's not a strong opinion. Out of curiosity, in what circumstances would recommendedViewportScale != 1.0?

The recommended viewport scale is intended as a mechanism for the UA to suggest a scale factor based on internal metrics or heuristics, with the goal that an application using that scale factor should get an acceptable compromise between resolution and framerate. It would stay at 1.0 if the application's rendering is keeping up with the target framerate at full viewport size, and would only drop lower if the rendering time exceeds the frame time budget.

Ideally, the UA's recommended viewport scale would remain at 1.0 if the frame rate is low due to reasons other than GPU rendering time, i.e. if the application spends an excessive amount of CPU time in JS processing, where reducing the viewport scale would reduce the visual quality without any performance benefits.

@klausw
Copy link
Contributor Author

klausw commented Jul 25, 2020

@thetuvix wrote:

With this change, each getScaledViewport call would get you a scaled viewport, but what then chooses the official viewport that the UA will read from at the end of the frame? If it's simply the latest call to getViewport in a given frame, that seems both subtle and fragile if multiple components expect that they can just fetch the viewport as needed without coordination.
[...]
Perhaps more importantly, if most platforms just blindly accept any viewport you request, some apps may not actually go through the extra bother of calling getViewport to find out the UA's effective viewport and just run with the exact viewport they requested.

I'm not sure I understand the failure mode you're worried about here. As far as I can tell, there is no way for applications to "run with the exact viewport they requested", the WebXR API doesn't have any way to request a specific viewport. The existing getViewport(view) method and the proposed new scale-factor variant return a viewport rectangle that's selected by the UA. The application must use that as-is in a glViewport call to get correct rendering.

If there are separate parts of the application that call getViewport(view, scale) independently without coordinating with each other, the UA can return consistent viewports to avoid consistency problems.

I had mentioned this in previous comment #1091 (comment)

For example, if an application for some reason calls getScaledViewport multiple times for one view in the same frame with different scales, or with inconsistent per-eye values, the UA would be free to use its preferred value and ignore values inconsistent with its current viewport allocations.

Would it address your concern to make this a spec requirement instead of a UA choice?

As an example, let's say the application does something like this:

// Main rendering, requesting 50% scaling
let xrViewport = xrWebGLLayer.getViewport(xrView, 0.5);
gl.viewport(xrViewport.x, xrViewport.y, xrViewport.width, xrViewport.height);
// ... draw main content

// Auxiliary rendering, i.e. an added effect, unaware of viewport scaling
let auxViewport = xrWebGLLayer.getViewport(xrView);
gl.viewport(auxViewport.x, auxViewport.y, auxViewport.width, auxViewport.height);
// ... draw aux content

In this case, the UA would return the same viewport for both calls. That would be the half-sized viewport if the UA supports viewport scaling, the full-sized viewport if it doesn't support scaling, or potentially even a different size such as 0.75 scale if the UA enforces a lower limit.

On a spec/implementation level, the list of viewports would be treated as modifiable at the start of each frame, with each view's viewport being decided and locked in for that frame on the first call to that view's getViewport. Additional calls to getViewport for that same view within the same frame return the same viewport, no matter if the following calls use a different scale factor or don't supply a scale at all.

This still leaves a few UA implementation choices:

  • Allocate all viewports once statically, and keep them the same for the entire session. That's basically the current behavior, and UAs could continue to do it this way if they don't want to support viewport scaling.
  • Treat the viewport list as modifiable at the start of the frame. At the first getViewport call for any view, allocate all the views based on the requested scale.
  • Treat the viewport list as modifiable at the start of the frame. At the first getViewport call for a view, allocate that specific view only. Other views requested later can use different scale factors which get locked in at the time they are requested.

If the application doesn't call getViewport for any view, the UA should continue using the previous frame's viewport list.

The assumption here is that applications must do a gl.viewport call using the returned viewport. They'd get wrong results if they simply assume what the viewport size should be, but arguably that this would already be wrong without viewport scaling. For example, an application must not assume that a stereo headset uses the left half of the viewport for the left eye.

As a side note, the getViewport(view, scale) variant has the helpful property that applications will just work on implementations that don't support viewport scaling. The UA will then simply ignore the extra parameter and return the default full-size viewport, and as long as the application uses the returned viewport they'll get correct rendering.

@klausw
Copy link
Contributor Author

klausw commented Aug 8, 2020

@thetuvix , does comment #1091 (comment) address your concerns?

To move this forward, can we revisit this in one of the next meetings? We had a few API variants under discussion, I'd propose the following to make it more concrete:

  • modify the existing XRWebGLLayer.getViewport(XRView) method to optionally take an additional numeric scale argument. A missing argument or undefined scale value is treated as using the default scale.
  • Add an optional read-only XRView.recommendedViewportScale numeric attribute where the UA can provide a suggested value for the current frame's viewport scale based on performance heuristics. UAs can leave this undefined if they don't have such a heuristic. (Application must not assume that this is a defined numeric value.)

I think this should be forwards and backwards compatible. If an application uses xrWebGLLayer.getViewport(xrView, xrView.recommendedViewportScale) on a UA that doesn't support viewport scaling, the UA ignores the extra argument, and looking up the recommended viewport scale harmlessly returns undefined. Conversely, an application that's unaware of using viewport scaling can continue to call xrWebGLLayer.getViewport(xrView) to get default-sized viewports.

As far as the specification is concerned, I think the needed changes would be something like this (handwavingly):

  • reinterpret the currently described list of viewports for the XRWebGLLayer as the list of full-sized viewports corresponding to scale=1.0. These stay unchanged for the lifetime of a session.
  • add a list of scaled viewports, where each entry consists of:
    • a scaled viewport which is a sub-rectangle of the full-sized viewport for that view, initially full-sized
    • a modifiable boolean, initially false
  • before the update the viewports step at the start of an animation frame, if the UA supports viewport scaling, mark each viewport in the list of scaled viewports as modifiable
  • in the getViewport(view, scale) method:
    • get the viewport from the list of scaled viewports
    • if the viewport is modifiable:
      • calculate a new viewport scale based on the supplied scale (or default 1.0 if the requested scale is undefined), and applying any applicable UA constraints such as a minimum scale. Maximum scale is 1.0.
      • update the viewport's x/y/width/height attributes based on the new scale, using a subrectangle of the corresponding full-sized viewport chosen by the UA
      • set modifiable to false for this viewport
    • return the scaled viewport. (Its current scale may be different from the requested scale if it wasn't modifiable.)

The goal of this is that a viewport's scale is only modifiable once within a given animation frame, and is then locked in for the rest of the animation frame. Also, a view's viewport will only change as a result of calling getViewport for that view, and will remain the same for future frames. As long as an application follows each getViewport call with a matching gl.viewport setting, it'll get correct rendering even if it is inconsistent about requested scale factors or if it only applies scaling to some views.

I added the constraint that the scaled viewports must each be fully contained within the corresponding original full-sized viewport. This ensures that each view can always be resized individually even if the other views aren't getting changed in this frame. Initially I thought it might be useful to let the UA change locations more freely, i.e. to keep two undersized eye views packed together in the top left corner of the framebuffer, but that would require moving other views to avoid overlap even if the application didn't call getViewport for them. UAs could still get contiguous viewports in some cases if desired, i.e. by arranging left/right eye views symmetrically around the middle of the framebuffer.

/agenda to discuss this proposal and potential alternatives

@probot-label probot-label bot added the agenda Request discussion in the next telecon/FTF label Aug 8, 2020
@thetuvix
Copy link
Contributor

Apologies for the delay! Yes, my primary concern here was too much ambiguity across UAs when two getViewport calls in a frame differ in scale - being explicit in the spec as you propose here would address my concerns!

I was skeptical in my comment when imagining a "last scale wins" approach, because a later component could cause spooky action at a distance to the viewport expected for the rendering already underway in an earlier component. However, "first scale wins" solves that nicely. Once any app code observes a viewport, it remains valid for the rest of that frame.

One more detailed spec questions for us to answer is what precisely not specifying scale means. I could see arguments for any of these - we should be specific, though:

  • Specified to be the same as 1.0
  • Specified to be the same as recommendedViewportScale
  • Up to the UA

Also pinging @rcabanier to reason about how the layers module can support viewport scaling in XRWebGLBinding.getSubImage/getViewSubImage, specifically how we communicate recommendedViewportScale for individual layers/views.

@klausw
Copy link
Contributor Author

klausw commented Aug 11, 2020

@thetuvix wrote:

One more detailed spec questions for us to answer is what precisely not specifying scale means. I could see arguments for any of these - we should be specific, though:

  • Specified to be the same as 1.0
  • Specified to be the same as recommendedViewportScale
  • Up to the UA

I think an undefined scale needs to be treated as 1.0 if the goal is to make this an opt-in mechanism. The recommendedViewportScale is intended to be adjusted by the UA frequently, potentially every frame, so rendering will only work correctly if the applications calls each view's getViewport every frame and applies the result.

We could consider using an opt-in mechanism similar to the secondary-views feature, or potentially allow the UA to automatically scale viewports if secondary-views is enabled.

@klausw
Copy link
Contributor Author

klausw commented Aug 11, 2020

Notes from today's call:

@thetuvix: Instead of adding an extra argument to getViewport, consider a separate XRView.requestViewportScale method. This would make it clearer that this is a request, not a guarantee to get an exact result, and it would also help for applications that want to set up the viewport scale at the start of a frame but don't have a need for the viewport yet.

I think this sounds reasonable, and seems cleaner than the overloaded method. It should be compatible with the proposed spec changes and semantics.

We need to clarify the specific meaning of the scale factor, is it an edge or area multiplier? Consensus was that the scale factor should multiply the width and height individually, consistent with the existing framebufferScaleFactor, but this needs to be clarified in the specification, including clarifying framebufferScaleFactor which currently refers to a somewhat ambiguous resolution scale. The alternative would be to interpret the scale factor as applying to the area or pixel count, where the width and height are individually multiplied by sqrt(scale).

Related to this, the specification should clarify that if the UA implements viewport scaling, it should consistently interpret the requested scale in this way to avoid inconsistencies. The UA would be free to apply constraints such as a minimum scale, or apply rounding or modifications such as aligning to a preferred pixel grid, but the overall result should be close to what the application requested.

@Manishearth : a downside is that applications don't know the size of the viewport before having to request and lock in a scale factor.

I think that isn't a major issue, the intent is for viewport scaling to be dynamic, so applications could modify the scale for future frames. @toji : If the application knows ahead of time that it wants fewer pixels to render, it should use framebufferScaleFactor instead of (or in addition to) viewport scaling since that also saves memory.

Next steps: I'll work on a PR.

@Yonet Yonet removed the agenda Request discussion in the next telecon/FTF label Aug 17, 2020
klausw added a commit to klausw/webxr that referenced this issue Sep 22, 2020
@klausw
Copy link
Contributor Author

klausw commented Sep 25, 2020

/agenda PR #1132 is now merged, adding to agenda for visibility and in case anyone has additional feedback.

@probot-label probot-label bot added the agenda Request discussion in the next telecon/FTF label Sep 25, 2020
@klausw
Copy link
Contributor Author

klausw commented Oct 13, 2020

Closing since I think we don't have any open discussion points, thank you to everyone for their feedback. The API is currently being prototyped in Chrome Canary behind the "WebXR Incubations" flag for Android's GVR and ARCore devices, please see https://crbug.com/1133381 in case you want to follow that.

@Yonet Yonet removed the agenda Request discussion in the next telecon/FTF label Oct 13, 2020
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

No branches or pull requests

6 participants