add first prototype of texElementImage2D#3752
add first prototype of texElementImage2D#3752cabanier wants to merge 2 commits intoKhronosGroup:mainfrom
Conversation
kenrussell
left a comment
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.
| <p>The width and height of the texture are derived from the CSS width and height of the element at the time of rendering. Add links to whatwg.</p> | ||
| <p>TBD: define state of rendering + security. These will be more links to whatwg</p><p></p> | ||
| <p>If no WebGLBuffer is bound to the <code>PIXEL_PACK_BUFFER</code> target, generates an | ||
| <code>INVALID_OPERATION</code> error.</p> |
There was a problem hiding this comment.
This validation related to PIXEL_PACK_BUFFER looks reversed; if a buffer's bound to that target then the textures' contents come from it. I think that instead this should say "If a WebGLBuffer is bound...", since the goal is for the texture's contents to come from the rendered element.
|
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.
@cabanier -- yes, the IDL in the chromium repo represents our most current thinking. We just followed the paradigm of texImage2D, which allows a scaling factor to be specified via optional width/height parameters. If those parameters are omitted, then the size of the texture will be set so that blitting the texture into the canvas will cause the image to appear with the same visual proportions it would have were it placed outside the canvas.
Note that the WebGPU version of the API doesn't allow implicit scaling via width/height, which aligns with WebGPU's handling of images via copyImageToTexture.
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.
5 participants
cc @kenrussell