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

Update explainer to account for multi-GPU handling #226

Merged
merged 3 commits into from May 12, 2017
Merged

Update explainer to account for multi-GPU handling #226

merged 3 commits into from May 12, 2017

Conversation

toji
Copy link
Member

@toji toji commented May 2, 2017

Closely follows Microsoft’s suggestion from #223, though I’ve moved the
ensureCompatibility call to the VRDevice because it feels a bit better
localized that way.

Closely follows Microsoft’s suggestion from #223, though I’ve moved the
ensureCompatibility call to the VRDevice because it feels a bit better
localized that way.
@NellWaliczek
Copy link
Member

Thanks! This generally looks great :)
A few small things:

  • Your framing about "VR-Centric" vs "VR-Enhanced" was really helpful and it might be useful to include something similar here to assist developers in determining which approache is right for them.
  • Can we call out something about the fact that calling ensureContextCompatibility may (will always?) have impact on more than just the glcontext supplied? I'd love to know how folks are approaching the implementation of the power preference attribute. It may make sense for us to align with that pattern...
  • Can we say that ensureContextCompatibility may reject for other reasons (such as already having been paired with an incompatible VRDevice already?)

@RafaelCintron
Copy link

RafaelCintron commented May 3, 2017

I'd love to know how folks are approaching the implementation of the power preference attribute. It may make sense for us to align with that pattern...

@grorg detailed how Webkit is approaching the powerPreference attribute in a post to the WebGL mailing list. The latest I've heard from @kenrussell is Chrome will be following a similar approach.

The language in the spec is written such that contextLost/restored can happen at any time to regulate power and memory consumption.

explainer.md Outdated
@@ -499,6 +512,8 @@ interface VRDevice : EventTarget {

Promise<boolean> supportsSession(VRSessionCreateParametersInit parameters);
Promise<VRSession> requestSession(VRSessionCreateParametersInit parameters);

Promise<void> ensureContextCompatibility(WebGLRenderingContext context);

Choose a reason for hiding this comment

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

We should use WebGLRenderingContextBase instead of WebGLRenderingContext so that the API can be used with WebGL 2.

Choose a reason for hiding this comment

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

Agree with this, or perhaps use a union type if you want to refer to a data type that's exposed to JavaScript. (WebGLRenderingContextBase isn't.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I swear I had already done that.

I don't think there's a problem with using WebGLRenderingContextBase here, even if it isn't Javascript exposed. The IDLs should still parse correctly. (But if not, then yes. A union would work just fine.)

@toji
Copy link
Member Author

toji commented May 7, 2017

Updated the pull request based on feedback. Felt silly adding a couple of lines around some unrelated topics that I know we'll be replacing soon, but it was necessary to maintain a decent flow in the document. 🤷‍♂️

PTAL!

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

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

Looking good!

explainer.md Outdated

**VR Enhanced:** The app can take advantage of VR, but it's used as a progressive enhancement rather than a core part of the experience. Most users will probably not interact with the app's VR features, and as such asking them to make VR-centric decisions early in the app lifetime would be confusing and innapropriate. An example would be a news site with an embedded 360 photo gallery or video. (We expect the large majority of early WebVR content to fall into this category.)

This style of application should call `VRDisplay.ensureContextCompatibility` with the WebGL context in question. This will set a compatibility bit on the context that allows it to be used. Contexts without the compatibility bit will fail when attempting to create a `VRLayer` with them. In the event that a context is not already compatible with the `VRDisplay` the [context will be lost and attempt to recreate itself](https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.13) using the compatible graphics adapter. It is the page's responsibility to handle WebGL context creation loss properly, recreating any necessary WebGL resources in response. If the context loss is not by the page handled the promise returned by `ensureContextCompatibility` will fail. The promise may also fail for a variety of other reasons, such as the context being actively used by a different, incompatible `VRDevice`.
Copy link
Member

Choose a reason for hiding this comment

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

"It is the page's responsibility to handle WebGL context creation loss properly," Did you mean to have the word "creation" in this sentence?

explainer.md Outdated

The content to present to the device is defined by a [`VRLayer`](https://w3c.github.io/webvr/#interface-vrlayer). In the initial version of the spec only one layer type, `VRCanvasLayer`, is defined and only one layer can be used at a time. This is set via the `VRSession.baseLayer` attribute. (`baseLayer` because future versions of the spec will likely enable multiple layers, at which point this would act like the `firstChild` attribute of a DOM element.)

In order for a WebGL canvas to be used with a `VRCanvasLayer`, it's context must be _compatible_ with the `VRDevice`. This can mean different things for different environments. For example, on a desktop computer this means that the context should be created against the graphics adapter that the `VRDevice` is physically plugged into. On most mobile devices though, that's not a concern and so the context will always be compatible. In either case, the WebVR application will need to take steps to ensure WebGL context compatibility before using it with a `VRLayer`.
Copy link
Member

Choose a reason for hiding this comment

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

it's context must be->its context must be

explainer.md Outdated

In order for a WebGL canvas to be used with a `VRCanvasLayer`, it's context must be _compatible_ with the `VRDevice`. This can mean different things for different environments. For example, on a desktop computer this means that the context should be created against the graphics adapter that the `VRDevice` is physically plugged into. On most mobile devices though, that's not a concern and so the context will always be compatible. In either case, the WebVR application will need to take steps to ensure WebGL context compatibility before using it with a `VRLayer`.

When it comes to ensuring canvas compatibility there's a two broad categories that apps will fall under.
Copy link
Member

Choose a reason for hiding this comment

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

there's a two -> there's two

explainer.md Outdated

The content to present to the device is defined by a [`VRLayer`](https://w3c.github.io/webvr/#interface-vrlayer). In the initial version of the spec only one layer type, `VRCanvasLayer`, is defined and only one layer can be used at a time. This is set via the `VRSession.baseLayer` attribute. (`baseLayer` because future versions of the spec will likely enable multiple layers, at which point this would act like the `firstChild` attribute of a DOM element.)

In order for a WebGL canvas to be used with a `VRCanvasLayer`, it's context must be _compatible_ with the `VRDevice`. This can mean different things for different environments. For example, on a desktop computer this means that the context should be created against the graphics adapter that the `VRDevice` is physically plugged into. On most mobile devices though, that's not a concern and so the context will always be compatible. In either case, the WebVR application will need to take steps to ensure WebGL context compatibility before using it with a `VRLayer`.
Copy link
Member

Choose a reason for hiding this comment

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

on a desktop computer this means that the context should be created -> on a desktop computer this may mean the context must be created

explainer.md Outdated

The content to present to the device is defined by a [`VRLayer`](https://w3c.github.io/webvr/#interface-vrlayer). In the initial version of the spec only one layer type, `VRCanvasLayer`, is defined and only one layer can be used at a time. This is set via the `VRSession.baseLayer` attribute. (`baseLayer` because future versions of the spec will likely enable multiple layers, at which point this would act like the `firstChild` attribute of a DOM element.)

In order for a WebGL canvas to be used with a `VRCanvasLayer`, it's context must be _compatible_ with the `VRDevice`. This can mean different things for different environments. For example, on a desktop computer this means that the context should be created against the graphics adapter that the `VRDevice` is physically plugged into. On most mobile devices though, that's not a concern and so the context will always be compatible. In either case, the WebVR application will need to take steps to ensure WebGL context compatibility before using it with a `VRLayer`.
Copy link
Member

Choose a reason for hiding this comment

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

the WebVR application will need to take -> the WebVR application must take

explainer.md Outdated

The content to present to the device is defined by a [`VRLayer`](https://w3c.github.io/webvr/#interface-vrlayer). In the initial version of the spec only one layer type, `VRCanvasLayer`, is defined and only one layer can be used at a time. This is set via the `VRSession.baseLayer` attribute. (`baseLayer` because future versions of the spec will likely enable multiple layers, at which point this would act like the `firstChild` attribute of a DOM element.)

In order for a WebGL canvas to be used with a `VRCanvasLayer`, it's context must be _compatible_ with the `VRDevice`. This can mean different things for different environments. For example, on a desktop computer this means that the context should be created against the graphics adapter that the `VRDevice` is physically plugged into. On most mobile devices though, that's not a concern and so the context will always be compatible. In either case, the WebVR application will need to take steps to ensure WebGL context compatibility before using it with a `VRLayer`.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps...
compatibility before using it with a VRLayer. ->compatibility before using it with a VRCanvasLayer.

let gl = glCanvas.getContext("webgl", { compatibleVrDevice: vrDevice });
```

Ensuring context compatibility with a `VRDisplay` through either method may have side effects on other graphics resources in the page, such as causing the entire user agent to switch from rendering using an integrated GPU to a discreet GPU.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to make this last sentence more visible? Since this could have a big impact, it's pretty important it not get lost in the explainer ^_^

Copy link

@RafaelCintron RafaelCintron left a comment

Choose a reason for hiding this comment

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

Thank you for making these changes @toji . I have mostly minor issues.

explainer.md Outdated
@@ -120,15 +120,13 @@ function BeginVRSession(isExclusive) {
}
```

Once the session is started some setup must be done to prepare for rendering. The content to present to the device is defined by a [`VRLayer`](https://w3c.github.io/webvr/#interface-vrlayer). In the initial version of the spec only one layer type, `VRCanvasLayer`, is defined and only one layer can be used at a time. This is set via the `VRSession.baseLayer` attribute. (`baseLayer` because future versions of the spec will likely enable multiple layers, at which point this would act like the `firstChild` attribute of a DOM element.)
Once the session is started some setup must be done to prepare for rendering. When using an exclusive session the canvas width and height must be set to the appropriate size for the `VRDevice` display. The correct size can be queried from `VRSession.getSourceProperties()`. The depth range the session will use should also be set to something appropriate for the application. This range will be used in the construction of the projection matricies provided by `VRSession.getDevicePose()` each frame.

Choose a reason for hiding this comment

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

Once the session is started some setup must be done to prepare for rendering

Should be: "Once the session has started, some setup must be done to prepare for rendering."

When using an exclusive session the canvas width and height must be set to the appropriate size for the VRDevice display.

Should be: "When using an exclusive session, the canvas width and height must be set to the appropriate size for the VRDevice display."

### Setting up a VRLayer

The content to present to the device is defined by a [`VRLayer`](https://w3c.github.io/webvr/#interface-vrlayer). In the initial version of the spec only one layer type, `VRCanvasLayer`, is defined and only one layer can be used at a time. This is set via the `VRSession.baseLayer` attribute. (`baseLayer` because future versions of the spec will likely enable multiple layers, at which point this would act like the `firstChild` attribute of a DOM element.)

Choose a reason for hiding this comment

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

In the initial version of the spec only one layer type, VRCanvasLayer, is defined and only one layer can be used at a time.

Should be: "In the initial version of the spec, only one layer type, VRCanvasLayer, is defined and only one layer can be used at a time. "

explainer.md Outdated

**VR Enhanced:** The app can take advantage of VR, but it's used as a progressive enhancement rather than a core part of the experience. Most users will probably not interact with the app's VR features, and as such asking them to make VR-centric decisions early in the app lifetime would be confusing and innapropriate. An example would be a news site with an embedded 360 photo gallery or video. (We expect the large majority of early WebVR content to fall into this category.)

This style of application should call `VRDisplay.ensureContextCompatibility` with the WebGL context in question. This will set a compatibility bit on the context that allows it to be used. Contexts without the compatibility bit will fail when attempting to create a `VRLayer` with them. In the event that a context is not already compatible with the `VRDisplay` the [context will be lost and attempt to recreate itself](https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.13) using the compatible graphics adapter. It is the page's responsibility to handle WebGL context creation loss properly, recreating any necessary WebGL resources in response. If the context loss is not by the page handled the promise returned by `ensureContextCompatibility` will fail. The promise may also fail for a variety of other reasons, such as the context being actively used by a different, incompatible `VRDevice`.

Choose a reason for hiding this comment

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

If the context loss is not by the page handled the promise returned by ensureContextCompatibility will fail.

Should be: "If context lost is not handled by the page, the promise returned by ensureContextCompatibility will be rejected."

Copy link
Member Author

Choose a reason for hiding this comment

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

Oof. 😫 Yoda my fingers think I am.

explainer.md Outdated
When it comes to ensuring canvas compatibility there's a two broad categories that apps will fall under.

**VR Enhanced:** The app can take advantage of VR, but it's used as a progressive enhancement rather than a core part of the experience. Most users will probably not interact with the app's VR features, and as such asking them to make VR-centric decisions early in the app lifetime would be confusing and innapropriate. An example would be a news site with an embedded 360 photo gallery or video. (We expect the large majority of early WebVR content to fall into this category.)

Choose a reason for hiding this comment

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

innapropriate -> inappropriate.

}
```

**VR Centric:** The app's primary use case is VR, and as such it doesn't mind initializing resources in a VR-centric fashion, which may include asking users to select a headset as soon as the app starts. An example would be a game which is dependent on VR presentation and input. These types of applications can to avoid the need to call `ensureContextCompatibility` and the possible context loss that it may trigger by passing the `VRDevice` that the context will be used with as a context creation argument.

Choose a reason for hiding this comment

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

These types of applications can to avoid the need to call ensureContextCompatibility and the possible context loss that it may trigger by passing the VRDevice that the context will be used with as a context creation argument.

Should be: "These types of applications can avoid calling ensureContextCompatibility, and the possible context loss that it may trigger, by passing the VRDevice that the context will be used with as a context creation argument.

let gl = glCanvas.getContext("webgl", { compatibleVrDevice: vrDevice });
```

Ensuring context compatibility with a `VRDisplay` through either method may have side effects on other graphics resources in the page, such as causing the entire user agent to switch from rendering using an integrated GPU to a discreet GPU.

Choose a reason for hiding this comment

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

One disadvantage of having the developer pass the VRDevice as a context creation attribute and/or calling ensureContextCompatibility is that multiple documents on the page can fight about the GPU if each one passes a different VRDevice to these two mechanisms. Perhaps we only allow one selection at a time per document. For iFrames, we can use the permissions API to mitigate this.

@toji
Copy link
Member Author

toji commented May 9, 2017

Thanks for calling out my atrocious grammar. 😅 Updated with the above suggestions.

@toji
Copy link
Member Author

toji commented May 12, 2017

Given the lack of negative feedback I'm going to merge this. As always, nothing is final yet so please file issues or follow up pull requests if there's some aspect of this change that needs clarification.

@toji toji merged commit 446d1a9 into master May 12, 2017
@toji toji deleted the multi-gpu branch May 12, 2017 18:31
toji pushed a commit that referenced this pull request May 16, 2017
Following the language from the PR for the 2.0 explainer (#226), this adds a paragraph that describes the expected sequence of events during requestPresent, in the event that a WebGL context is not compatible with the VRDisplay's graphics adapter. The webglcontextlost will be fired, and if Event.preventDefault() is called, the user agent will at that point switch to the VRDisplay's graphics adapter and continue the requestPresent process.
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.

None yet

4 participants