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

Spec glrc.drawingBufferStorage. #3222

Merged
merged 3 commits into from
Dec 20, 2023

Conversation

kdashg
Copy link
Contributor

@kdashg kdashg commented Mar 3, 2021

An alternative to #3220.

Copy link
Member

@kenrussell kenrussell left a comment

Choose a reason for hiding this comment

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

This is pleasingly small and gives applications the chance to feature detect. Personally I like this approach. Other opinions / feedback?

A few questions.

specs/latest/1.0/index.html Outdated Show resolved Hide resolved
specs/latest/1.0/index.html Show resolved Hide resolved
specs/latest/1.0/index.html Show resolved Hide resolved
@@ -1671,6 +1703,7 @@ <h3><a name="WEBGLRENDERINGCONTEXT">The WebGL context</a></h3>

const GLenum RGBA4 = 0x8056;
const GLenum RGB5_A1 = 0x8057;
const GLenum RGBA8 = 0x8058;
Copy link
Member

Choose a reason for hiding this comment

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

Some WebGL conformance tests like conformance/context/constants-and-properties.html and conformance2/context/constants-and-properties-2.html (and maybe others) will have to be updated for this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's on the list of test changes that we'll need.

Copy link
Member

Choose a reason for hiding this comment

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

@ccameron-chromium points out in https://chromium-review.googlesource.com/c/chromium/src/+/4887172 that it's necessary to add RGB8 to WebGL 1.0 too, because {alpha:false} contexts use this as drawingBufferFormat even though drawingBufferStorage can't be called for such contexts afterward.

@@ -1728,6 +1761,8 @@ <h3><a name="WEBGLRENDERINGCONTEXT">The WebGL context</a></h3>
sequence&lt;DOMString&gt;? getSupportedExtensions();
object? getExtension(DOMString name);

undefined backbufferStorage(GLenum sizedFormat, unsigned long width, unsigned long height);
Copy link
Member

Choose a reason for hiding this comment

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

Similar for this new method.

similar to <code>renderbufferStorage</code> as used on Renderbuffers.

Clearing behavior is equivalent to setting <code>HTMLCanvasElement.width</code> and <code>HTMLCanvasElement.height</code>, followed by changing the backbuffer format.
This method respects <code>WebGLContextAttributes.antialias</code>.
Copy link
Member

Choose a reason for hiding this comment

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

How can the user know (aside from calling gl.getError, which is discouraged) that the setting actually took effect? Should we add a new read-only property alongside drawingBufferWidth and drawingBufferHeight?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't tell today for textures or renderbuffers, so I'm not super worried about going above-and-beyond for backbuffers.

@kenrussell
Copy link
Member

@lexaknyazev @ccameron-chromium please review this version of this functionality and see how you like it compared to #3220 . (I don't have the ability to explicitly add you as reviewers.) Thanks.

@kainino0x
Copy link
Contributor

I haven't thought on this too deeply, but it seems like quite a nice approach. I initially thought the dynamicness would be scary - but the semantic I imagine is simply "throw out the drawing buffer you allocated for this frame (if any) and replace it with new blank one with this format". As long as implementations can get that texture to the compositor, that's great.

nit: The "backbuffer" naming makes the "back"ness central when it's not necessarily relevant (though with the semantic above, maybe it is, since you couldn't have that semantic on a frontbuffer...?) Could be "default framebuffer", "default framebuffer color attachment", "drawing buffer", ?

@kdashg
Copy link
Contributor Author

kdashg commented Mar 4, 2021

I'm definitely being cheeky and saying "backbuffer" when I mean "default framebuffer", but "default framebuffer" is relatively poorly understood, I believe. I can s/backbuffer/default framebuffer/ though, it's technically more correct.

@kdashg
Copy link
Contributor Author

kdashg commented Mar 4, 2021

Actually, since I would like to avoid defaultFramebufferStorage, I would prefer to keep backbufferStorage.
It certainly behaves as the backbuffer as per spec (for dom atomicity and such) and I think this is probably the least bad name I can think of. Does anyone else have ideas?

I like this as-is, because I feel like practically 100% of people will understand what's going on here, even without a good background in WebGL details.

@lexaknyazev
Copy link
Member

Looks great overall. Couple thoughts:

  • The WebGL backbuffer format complements the color space set during canvas creation. Are all possible combinations valid? For example, will future BT.2020 canvas work with RGBA8 backbuffers?
  • It is not immediately clear to me how respecifying backbuffer's width affects HTMLCanvasElement.width, CSS width, and WebGLRenderingContext.drawingBufferWidth.

@ccameron-chromium
Copy link
Contributor

  • The WebGL backbuffer format complements the color space set during canvas creation. Are all possible combinations valid? For example, will future BT.2020 canvas work with RGBA8 backbuffers?

I think all combinations should be valid. They are all equally feasible technically, but have varying degrees of usefulness. Most APIs (Metal, Vulkan, D3D) allow all combinations.

Would we want to include the color space as an argument to backbufferStorage, too? (I'm bringing the question up for completeness -- I have no opinion on that topic. For WebGPU, the plan is to add color space to GPUSwapChainDescriptor, which is vaguely analogous. Should we leave open the possibility of adding it as an optional argument?)

@kdashg
Copy link
Contributor Author

kdashg commented Mar 4, 2021

TBH moving all the way to a WebGLSwapChain sounds better. I'll see what that looks like.
It'd be fine if colorspace was fixed, but I agree that it's a little weird to have one fixed and not the other.

@kdashg
Copy link
Contributor Author

kdashg commented Mar 4, 2021

And +1 on keeping colorspace and format orthogonal, even if some combos are bad ideas. (rec2020 rgba8)

@kainino0x
Copy link
Contributor

TBH moving all the way to a WebGLSwapChain sounds better. I'll see what that looks like.

Moving to WebGLSwapChain would be extremely cool, but I worry it would make the road to implementation a lot longer. Seems ideal to do things incrementally: design something like backbufferStorage() now that can still be used in a future with a WebGLSwapChain and maybe later even headless WebGL contexts.

On the topic of WebGLSwapChain, I bet WebGPU's GPUCanvasContext could serve dual purpose. Note the GPUDevice isn't bound to the GPUCanvasContext until GPUCanvasContext.configureSwapChain.

interface GPUCanvasContext {
    GPUSwapChain configureSwapChain(GPUDevice device, GPUSwapChainDescriptor descriptor);

    WebGLSwapChain configureSwapChain(WebGLRenderingContext context, GPUSwapChainDescriptor descriptor);
    // or
    WebGLFramebuffer configureSwapChain(WebGLRenderingContext context, GPUSwapChainDescriptor descriptor);
};

or something.

@kenrussell
Copy link
Member

Thanks @kainino0x for your input.

Per discussion today with @jdashg and @ccameron-chromium - we're in agreement that adding a swap chain concept to WebGL at this late stage in its evolution is too much to add.

Here's the current plan:

  • Add drawingBufferFormat read-only attribute to WebGLRenderingContextBase
  • Add drawingBufferStorage(format, width, height) method to WebGLRenderingContextBase, which allows setting it

Still discussing how colorSpace will be handled. Since the drawing buffer's format is changeable, colorSpace really needs to be changeable at run time, too, rather than being specified during context creation time.

Respecify the size and format of the backbuffer,
similar to <code>renderbufferStorage</code> as used on Renderbuffers.

Clearing behavior is equivalent to setting <code>HTMLCanvasElement.width</code> and <code>HTMLCanvasElement.height</code>, followed by changing the backbuffer format.
Copy link
Member

@RafaelCintron RafaelCintron Mar 13, 2021

Choose a reason for hiding this comment

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

The spec talks about clearing behavior here.

I presume this means that after the developer calls backbufferStorage, subsequently querying HTMLCanvasElement.width, HTMLCanvasElement.height, drawingBufferWidth and drawingBufferHeight will return the values passed to `backbufferStorage'?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, modulo the same limitations the browser already enforces. Recall that drawingBufferWidth and drawingBufferHeight may be clamped due to platform limitations. The intent here is to make this new API behave exactly the same as resizing the HTMLCanvasElement or OffscreenCanvas, to avoid adding yet another behavior to the web platform.

Respecify the size and format of the backbuffer,
similar to <code>renderbufferStorage</code> as used on Renderbuffers.

Clearing behavior is equivalent to setting <code>HTMLCanvasElement.width</code> and <code>HTMLCanvasElement.height</code>, followed by changing the backbuffer format.
Copy link
Member

@RafaelCintron RafaelCintron Mar 13, 2021

Choose a reason for hiding this comment

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

If the user first calls backBufferStorage and subsequently sets the HTMLCanvasElement.width and HTMLCanvasElement.height attributes, will the default framebuffer be resized to the last format passed as the sizedFormat parameter? Or will the default framebuffer be resized to whatever the default format the user agent decided when it first allocated the default framebuffer? The former seem preferable but I don't have a strong opinion.

Copy link
Member

Choose a reason for hiding this comment

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

It'll use the last format passed in to backBufferStorage - which per the last WebGL WG meeting will be renamed to drawingBufferStorage in the next iteration of this proposal.

Copy link
Member

@kenrussell kenrussell left a comment

Choose a reason for hiding this comment

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

Thanks Jeff for putting together and revising this; it looks good to me!

I'm really excited about trying this out! How do you think we should move forward? Land this and put implementations behind flags, for example?

</p>
<p>
In WebGL 1.0, <code>RGBA16F</code> requires the extension <code>EXT_color_buffer_half_float<code>.
In WebGL 2.0, <code>RGBA16F</code> requires the extension <code>EXT_color_buffer_float<code>.
Copy link
Member

Choose a reason for hiding this comment

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

EXT_color_buffer_half_float's legal in WebGL 2.0 contexts now - to support iOS devices specifically. It would be better to say that WebGL 2.0 requires either EXT_color_buffer_float or EXT_color_buffer_half_float.

@kenrussell kenrussell changed the title Spec glrc.backbufferStorage. Spec glrc.drawingBufferStorage. Mar 25, 2021
<dd>
The current effective format of the drawing buffer.
Initially <code>RGBA8</code> or <code>RGB8</code> for <code>alpha:true</code> and <code>alpha:false</code> respectively.
The actual height of the drawing buffer. May be different from the
Copy link
Member

Choose a reason for hiding this comment

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

This text and below's accidentally duplicated.

@greggman
Copy link
Contributor

greggman commented Feb 8, 2022

how do does this work with antialias? Does it ignore it? Up to browser? If up to browser can the browser switch from antialias true to false?

@kdashg
Copy link
Contributor Author

kdashg commented Feb 9, 2022

Good question. Fortunately, antialias (like depth and stencil) is fixed after context creation, so the logic would be similar to e.g. canvas.width = x, where if resize failed, and no satisfactory backbuffer could be created, our failure case of last resort is context-loss.

@ccameron-chromium
Copy link
Contributor

Is there anything more that we need to do in order to get this merged?

@kenrussell
Copy link
Member

@kdashg told me on chat it was OK if I merge this after final cleanup, so doing that now.

@kenrussell kenrussell merged commit 1025e76 into KhronosGroup:main Dec 20, 2023
2 checks passed
kenrussell added a commit to kenrussell/WebGL that referenced this pull request Dec 20, 2023
kenrussell added a commit that referenced this pull request Dec 20, 2023
@Elchi3
Copy link

Elchi3 commented Jan 25, 2024

This PR seems to be rendered on https://registry.khronos.org/webgl/specs/latest/1.0/ but not on https://registry.khronos.org/webgl/specs/latest/2.0/. Can the WebGL 2 spec be updated, too?

@kainino0x
Copy link
Contributor

It is defined on WebGLRenderingContextBase, so WebGL 2.0 inherits it from 1.0:

WebGL2RenderingContext includes WebGLRenderingContextBase;
WebGL2RenderingContext includes WebGL2RenderingContextBase;

(Same as for many other entrypoints.)

@Elchi3
Copy link

Elchi3 commented Jan 26, 2024

ahhh of course! Thank you!! Sorry for the noise.

This pull request was closed.
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.

8 participants