add first prototype of texElementImage2D - #3752
Conversation
kenrussell
left a comment
There was a problem hiding this comment.
One initial comment. Likely this PR will remain unmerged for a while until there are prototype implementations of this new API. In the meantime, progress can be made on the API's semantics, addressing the TBD mentioned here, etc.
|
The answer to WICG/html-in-canvas#68 in this PR seems to be that we do keep |
|
@cabanier what's still needed to make this ready for review? whatwg/html#11588 is making steady progress and I want to make sure the WebGL side is in sync. Importantly, the HTML spec now has a concept of an "element image snapshot" which is what must be used in canvas APIs like this one. Can you update the spec text to use that concept? You can use a direct link to https://whatpr.org/html/11588/canvas.html#concept-canvas-element-image-snapshots until the HTML spec PR is merged. |
| </dt> | ||
| <dd> | ||
| <p>Renders the given element to the currently bound WebGLTexture.</p> | ||
| <p>The width and height of the texture are derived from the CSS borderbox of the element at the time of rendering. Add links to whatwg.</p> |
There was a problem hiding this comment.
This seems pretty limiting compared to the 2d equivalent. Could this take optional width & height args? This seems especially important for high-dpr.
It also seems to be desirable to use a portion of an element in a texture, eg for extreme zoomed cases. This is also something you can do with the 2d API.
There was a problem hiding this comment.
I think the IDL changes in this PR are a bit behind; the current chromium implementation has four overrides for texElementImage2D which allow for specification of both source rect (for painting a section of an element, or for extending the painted area to include ink overflow) and also destination size (i.e. texture size).
@foolip can you update the PR to match the chromium implementation?
There was a problem hiding this comment.
The width/height was there originally but we were told that those APIs were going to be removed in the future. Has that changed?
What is the difference between passing width/height and setting it on the element?
There was a problem hiding this comment.
Setting it on the element means scaling everything else up within it, which is a pain. Trying to deal with the sub-rect case is even more painful.
There was a problem hiding this comment.
@szager-chromium that's good to hear. Although, it's a little frustrating that for some things, the explainer is more up to date, and for other things the PRs are more up to date, and for other things, like this, neither has the details.
There was a problem hiding this comment.
Where's the up to date proposal for the WebGPU API addition?
There was a problem hiding this comment.
Now that I'm looking at the code (which ironically I wrote), I see there there is a variant that allows you specify destination width/height, and applies that as a scale factor to the element's paint record. There is also a variant that allows you to specify a source rect that behaves the same as for texElementImage2D. There is not however a variant that allows you specify both of those.
I can't remember what I was thinking there, but the WebGPU API shape has definitely not had a lot of scrutiny yet, and can still be easily changed.
There was a problem hiding this comment.
It does seem worth having an API that allows both. It'd seem odd if the WebGPU version was the least capable.
I guess this can get more review when it's properly proposed somewhere.
There was a problem hiding this comment.
The missing overload has been added to the WebGPU equivalent API.
kenrussell
left a comment
There was a problem hiding this comment.
Four overloads of this single entry point seem like a little much to me. Would it be possible to collapse this down to two by saying that passing 0 for GLsizei width/GLsizei height has default behavior of deriving the width/height from the CSS borderbox of the element at the time of rendering?
| </dd> | ||
|
|
||
| <dt class="idl-code"> | ||
| [throws] undefined texElementImage2D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, |
There was a problem hiding this comment.
This is a duplicate of the entry on line 2240 and should be removed.
| </dt> | ||
| <dd> | ||
| <p>Renders the given element to the currently bound WebGLTexture.</p> | ||
| <p>sx, sy, swidth and sheight define a rectangular slice of the element's borderbox. This slice determines the size of the generated texture.</p> |
There was a problem hiding this comment.
This overload - if it's necessary - should define how the width and height are computed, similarly to the overload at line 2215.
Functionally this would work, but it contrasts with |
|
The existing Have you considered adding a new type to this list, e.g., Just saying that rendering HTML to a 2D Canvas and using that Canvas as a WebGL texture source provides much more functional and convenient experience (modulo pixel redirection) and likely bypasses all issues listed below simply because they are already defined for uploads from Issue 1: Texture allocation vs texture uploadThe
Sub-region texture updates are possible with the WebGL 2.0 supports a more efficient and safer workflow with immutable-format textures like this: gl.texStorage2D(...) // forever defines all dimensions and the pixel format, zero-inits the texture
gl.texSubImage2D(...) // updates the specified texture region with provided dataThe benefit here is that there won't be any implicit redefinitions and/or reallocations that are a common source of bugs both in apps and in implementations. This workflow would require setting the texture's dimensions before uploading the data, though. Issue 2: Texture typesThe WebGL 2.0 also supports 2D texture arrays and sliced 3D textures via Issue 3: Texture formatsThe
|
|
@lexaknyazev -- Thank you for the very detailed analysis, this is very helpful. We have discussed the possibility of adding another type to the
I don't think this is an improvement over your suggestion of drawing the element into a 2D canvas and then calling Of the three issues you described, I think the first is the most important. There should be a way to re-upload the texture without re-allocating it. This is especially important for continuously-animating pages where the HTML content may update on every animation frame. I think this one should be addressed before the feature ships in Chrome. Issue 2 seems a bit esoteric to me; and as you point out it's avoidable by specifying an explicit destination size. Maybe there's something we can do to make that nicer, but I wouldn't consider this a blocker. The |
The first step with the issue 2 is to decide whether texture types other than
There are some subtleties to consider here. For example, the current proposal has this line:
Setting the Another question is about all no-alpha formats. They are usually emulated in lower layers and in worst cases may involve costly per-pixel processing, e.g., to copy three-component RGB values to natively-supported RGBA with alpha set to 1.0. If the element rasterizer has an option to produce all-opaque alpha, that option could be passed explicitly to make sure that the declared texture format matches the internal representation. |
Some versions of this API take a width and height for the texture so if the author passes in the same values, there wouldn't be a reallocation.
Do you know how this is handled inside Chromium? I suspect it has to optimized this internally to get good performance. |
This is frankly well outside my area of expertise; but in the chromium implementation we funnel into the same code path as |
I would be inclined to restrict this to only
Removing those arguments sounds good to me!
I'm pretty sure the web rendering system always produces RGBA (not sure if this is chromium-specific, but I doubt it). But any way you slice it, I can't think of any reason why we would want to allow users to pick a different internal format, even if it could be supported. So again, my KISS instinct is to just drop this parameter. |
This means removing all And while we're here, are 8-bit values supposed to be sRGB-encoded? If so, the internal format for them should be |
|
On the question of excessive memory reallocations: even if we are able to optimize for the case where the size does not change with successive invocations, that still leaves a potential pitfall when animating the size or scale of an element. In that case, you would presumably want to allocate a texture at the maximum size of the animation and then draw sub-areas of it during other animation phases. I think this could still be accomplished in a couple of ways with the API as written:
Neither of those seems too onerous to me, but it does require the developer to be aware of the issue and take measures to avoid it. Requiring a call to |
|
Is the precise size of the In that case it would be feasible to do a sub-image upload via a new It might be better to start simple regardless and only support |
|
I think I'd prefer to start as simple as possible and then return to the idea of |
|
@szager-chromium I tried to incorporate everyone's feedback in the new PR. Let me know if you'd like to see changes. |
97f6e4f to
5a802e7
Compare
|
@szager-chromium Any comments on the latest PR? |
Looking at the canvas spec with the default set to srgb, the default internal format colorspace should probably be sRGB as well. When placing html with RGB8 passed as internalFormat as the explainer suggests, I can see the difference in colors between regular HTML and HTML-in-canvas, so probably the default format should be a gamma corrected one? |
I think there should be no difference when it's set to RGB8. @szager-chromium, is this a bug in Chromium's implementation? |
|
@lexaknyazev please let me know if you agree with my latest changes. |
| <tr><td>RGBA16F</td><td>HALF_FLOAT</td></tr> | ||
| <tr><td>RGBA32F</td><td>FLOAT</td></tr> |
There was a problem hiding this comment.
The spec should be very clear about the transfer functions used in each case.
I'd expect that 8-bit values are always sRGB-encoded and the choice between RGBA8 and SRGB8_ALPHA8 is only to allow apps to bypass hardware sRGB decoding during texture sampling if they wish so for some reason.
However, floating-point values are usually not sRGB encoded. If we aren't sure about the latter, maybe floating-point options could be omitted for now.
This it outside my expertise, but I think SRGB is already-gamma-corrected and RGBA is not, and the GL implementation treats them accordingly. Since we're dealing with web content, SRGB seems like the natural choice. @lexaknyazev -- does this sound right to you? |
The difference between Consider the following sequence:
You can argue that such workflow is neither semantically nor mathematically accurate but it should roundtrip rasterized values correctly. A "proper" workflow would look like this:
Important addition: |
I tried to incorporate your suggestion in 1f133db |
| <tr><td>RGBA16F</td><td>HALF_FLOAT</td></tr> | ||
| <tr><td>RGBA32F</td><td>FLOAT</td></tr> | ||
| </table> | ||
| <p>The RGBA source values produced by rendering the element image snapshot are computed independently of <code>internalformat</code>. In particular, when <code>internalformat</code> is <code>RGBA8</code> or <code>SRGB8_ALPHA8</code>, the generated <code>UNSIGNED_BYTE</code> source values are identical; the choice between those formats only affects the texture's storage and subsequent sampling behavior.</p> |
There was a problem hiding this comment.
This looks good for the UNSIGNED_BYTE case but if the spec is going to include floating-point options, they'd need a separate clarification.
There was a problem hiding this comment.
Can browser render HTML to floats? @szager-chromium, is it specified how chromium renders HTML to an HDR 2D canvas?
There was a problem hiding this comment.
@ccameron-chromium, I think you know this area, can you answer the question about wide color gamuts and canvas?
1f133db to
51f5f85
Compare
Add the GLint level parameter back to the WebGL 2 texElementImage2D signature so callers can choose which mipmap level to redefine. Update both the spec IDL block and the extracted webgl2.idl, and document the level argument in the method description.
|
@lexaknyazev FYI I added the level parameter back in. |
|
@cabanier -- based on conversation with WebGL veterans, we'd like to change the API to behave more like
I've got a CL up to implement this behavior in chromium, and I'm going to add inline comments to this PR where it needs to be updated. |
| GLfloat sy; | ||
| GLfloat swidth; | ||
| GLfloat sheight; | ||
| GLsizei width; |
There was a problem hiding this comment.
Add GLint xoffset and GLint yoffset parameters after sheight.
There was a problem hiding this comment.
The current Element API is
texElementImage2D(target, level, internalformat, element, config)
The texSubImage2D API is
texSubImage2D(target, level, xoffset, yoffset, ...)
Given that the proposed xoffset and yoffset parameters refer to the GL API, why add them to the config object (that is Element-specific) instead of the command (that is GL-specific)? I'd expect them to be in the same group as the level and the WebGL WG decided that level should be among the command parameters, not the config object.
There was a problem hiding this comment.
Also since the new design no longer allocates a GL texture, the internalformat parameter needs to be moved to the config.
There was a problem hiding this comment.
In other words:
- parameters that drive Element rasterization should be in the
config; - parameters that directly interact with GL should be on their usual places.
There was a problem hiding this comment.
The motivation for moving parameters to WebGLCopyElementImageConfig was just to avoid the need for overloads of the method just to accommodate various permutations of geometry arguments. The existing WebGLCopyElementImageConfig.(width|height) parameters already specify a destination size, and any scaling they imply is performed by GL, not by web content rendering. So I don't think adding xoffset and yoffset here contradicts any existing design principle or separation of web-specific from GL-specific arguments. The purpose of WebGLCopyElementImageConfig is really to encapsulate all the geometry.
There was a problem hiding this comment.
This is not what I have observed.
GL (on its own) does not have an ability to scale texture data during upload. You could have seen the Chromium behavior related to texture uploads from DOM images (which is not fully documented).
OK fair enough; but this doesn't really change my point about the purpose of WebGLCopyElementImageConfig being to contain all the geometry parameters, source and destination alike. From the perspective of a web developer, width and height are destination parameters, and I don't think a distinction between "part of OpenGL" and "part of the WebGL layer" is meaningful for them.
There was a problem hiding this comment.
widthandheightare destination parameters
Not quite if the command follows the texSubImage design.
There are several widths and heights here that exist in separate domains:
- Dimensions of the texture. These are set in advance by the developer (by calling regular
texStorageortexImagecommands) and not related to the Element API in any way. - Dimensions and the offset of the Element's source rectangle. These affect Element rasterization and are not related to the texture.
- Target rasterization dimensions. These also only affect the rasterization and are not related to the texture (as it exists already).
- Texture's sub-region offset (
xoffsetandyoffset). These have nothing to do with the rasterization geometry and only exist within the texture dimensions set in 1.
There was a problem hiding this comment.
I have in mind two reasons to prefer adding xoffset and yoffset to the config and leaving internalFormat where it is:
-
It places all geometry parameters in config, which I think is good ergonomics.
-
It doesn't break existing calling conventions if we allow
xoffsetandyoffsetto default to zero when not specified.
In my mind, that's more compelling than keeping the GL-specific bits outside the config. The very fact that this function uses a dictionary of parameters already sets it apart from existing WebGL API's.
Maybe we should get this on the WebGL working group agenda and discuss it there, what do you think?
There was a problem hiding this comment.
Maybe we should get this on the WebGL working group agenda and discuss it there, what do you think?
+1. I'll hold off on making changes until it's been discussed.
There was a problem hiding this comment.
@lexaknyazev -- thinking more about this, I see your point about aligning with the function signature of texSubImage2D and keeping the GL-specific parameters separate. Until you pointed it out, I didn't actually know that the automatic scaling behavior of the width and height parameters was specific to the WebGL layer.
I'm OK with putting xoffset and yoffset in the main method signature rather than in the config. I also think that we can probably get rid of the internalFormat parameter. So the new IDL would be:
undefined texElementImage2D(GLenum target, GLint level, GLint xoffset, GLint yoffset,
(Element or ElementImage) element,
optional WebGLCopyElementImageConfig config = {}); // May throw DOMException
Does this look good to you?
| <p>If a WebGLBuffer is bound to the <code>PIXEL_UNPACK_BUFFER</code> target, generates an <code>INVALID_OPERATION</code> error.</p> | ||
| <p>All <code>UNPACK_</code> pixel store parameters are ignored by this function.</p> | ||
| <p>If <code>config.sx</code>, <code>config.sy</code>, <code>config.swidth</code>, and <code>config.sheight</code> are all supplied, they define the source rectangle of the element image snapshot to render, in CSS pixels. If none of them are supplied, the source rectangle has <code>sx</code> and <code>sy</code> of 0, <code>swidth</code> equal to the source's natural width, and <code>sheight</code> equal to the source's natural height. If only some of them are supplied, generates an <code>INVALID_VALUE</code> error.</p> | ||
| <p>If <code>config.width</code> and <code>config.height</code> are both supplied, they define the generated texture size, in texels. If neither member is supplied, the texture width and height default to the source rectangle size scaled by the inverse of the canvas-grid-to-CSS scaling factor used to produce the element image snapshot. If only one of them is supplied, generates an <code>INVALID_VALUE</code> error.</p> |
There was a problem hiding this comment.
Replace "they define the generated texture size" with "they specify the area of the texture to be modified; this will result in scaling of the source graphic if the specified size does not match the source element's intrinsic size."
This is a big change. How will the author know the dimension of the texture? |
| GLfloat sy; | ||
| GLfloat swidth; | ||
| GLfloat sheight; | ||
| GLsizei width; |
There was a problem hiding this comment.
Add GLint xoffset and GLint yoffset
Here is some overly-verbose boilerplate for drawing an element at its intrinsic/natural size; I expect developers can use a slimmed-down version of this (or maybe we can add a convenience API to provide it): The feedback I got from WebGL veterans is that the automatic allocation behavior of Consider, for example, a simple layout animation: Under the current proposal, this would re-allocate the texture buffer on every paint. The recommended best practice for developers will be to pre-allocate a texture big enough to hold the maximum size of the target under the effects of the animation. |
cc @kenrussell