← 输入框|github.com|刷新
Skip to content

Add HTMLImageElement and ImageData to copyExternalImageToTexture - #3430

Merged
kainino0x merged 1 commit into
gpuweb:mainfrom
kainino0x:imageelement
Jul 26, 2023
Merged

kainino0x merged 1 commit into
gpuweb:mainfrom
kainino0x:imageelement

Conversation

@kainino0x

Copy link
Copy Markdown
Contributor

HTMLImageElement requested by @kdashg , ImageData added to complete symmetry with TexImageSource WebGL (except for VideoFrame).

@kainino0x kainino0x added this to the V1.0 milestone Sep 9, 2022
@github-actions

github-actions Bot commented Sep 9, 2022 •

Copy link
Copy Markdown
Contributor

Previews, as seen when this build job started (8fdb28a):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

@kainino0x

Copy link
Copy Markdown
Contributor Author

Some notes:

  • I think adding HTMLVideoElement support (Add HTMLVideoElement to copyExternalImageToTexture #3410) was an easy sell - HTMLVideoElement is the only representation of a video, I think (except VideoFrame), so adding that strictly removed a middleperson (ImageBitmap). HTMLImageElement is not so straightforward: the often-recommended path for getting an image into an ImageBitmap is actually through createImageBitmap(Blob), not createImageBitmap(HTMLImageElement). HTMLImageElement.decode() is not semantically the same as createImageBitmap(), even though both are async:

    • HTMLImageElement in general, and decode() in particular, imply the the image should be prepared for the DOM: "resolves when the image is decoded and it is safe to append the image to the DOM".
    • ImageBitmap is used as a copy source (or bitmaprenderer frame) and therefore createImageBitmap() implies it should be prepared for copy. In browsers, this may or may not be the same thing.

    For this reason, the Fetch-Blob-ImageBitmap-WebGPU path is arguably more direct than an HTMLImageElement(-decode)-WebGPU path. Of course, a Fetch-Blob-WebGPU path would be even more direct....

  • ImageData is a relatively simple overload, though not trivial: It looks like it can be easily implemented on top of writeTexture, but it actually can't because ImageData is color-managed and writeTexture is not.

@kainino0x

Copy link
Copy Markdown
Contributor Author

I propose punting these decisions to post-v1.

@kainino0x
kainino0x requested a review from kdashg September 30, 2022 22:32
@kainino0x
kainino0x marked this pull request as draft September 30, 2022 22:32
@kdashg

kdashg commented Nov 16, 2022

Copy link
Copy Markdown
Contributor

This is probably tolerable for images, for which uploads should be cold code.

@kainino0x kainino0x modified the milestones: V1.0, post-V1 Nov 16, 2022
@kdashg

kdashg commented Nov 29, 2022

Copy link
Copy Markdown
Contributor
GPU Web meeting 2022-11-16 non-APAC-timed
  • KG: probably tolerable for image uploads to be post-V1. OK for them to be slow. Main concern - having to go through ImageBitmaps is harder to optimize for Firefox. But we don't care about this for images as much. So probably fine.
  • KN: there are things I'd like to do eventually, but not for V1.
  • KR: agree this isn't a performance-critical code path.

@kainino0x kainino0x modified the milestones: post-V1, Milestone 2 Jun 29, 2023
@kainino0x
kainino0x marked this pull request as ready for review June 29, 2023 00:04
@kainino0x kainino0x added the tacit resolution queue Editors have agreed and intend to land if no feedback is given label Jul 12, 2023
@kainino0x

Copy link
Copy Markdown
Contributor Author

meeting: Tentative agreement for now, but google wants to think a bit more about the ImageData overload

@kainino0x kainino0x modified the milestones: Milestone 2, Polish post-V1 Jul 12, 2023
@kainino0x

kainino0x commented Jul 12, 2023 •

Copy link
Copy Markdown
Contributor Author

Also tagging as likely polish-post-v1 (i.e. we can land this and then implementations will go ahead with it whenever they're ready)

@kainino0x kainino0x added the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Jul 13, 2023
@greggman

greggman commented Jul 21, 2023 •

Copy link
Copy Markdown
Member

I want to verify my understanding.

In WebGL I can set gl.pixelStorei(gl.UNPACK_COLORSPACE_CONVERSION_WEBGL, gl.NONE) and pass an HTMLImageElement to texImage2D and get the raw data from a .PNG image

In WebGPU, if we add support for HTMLImageElement, there is no such path. If I want to make sure to get the raw data from an image in WebGPU I have to first convert the image to an ImageBitmap

const ib = await createImageBitmap(someImageElement, { colorSpaceConversion: 'none' });

That is the only path that guarantees raw .PNG data doesn't have any color space applied in WebGPU. Correct?

@kainino0x

Copy link
Copy Markdown
Contributor Author

@greggman correct. The preferred case would be that your PNG is either untagged or tagged as sRGB, then the color conversion into sRGB will be a no-op.

Maybe there's a need for the NONE mode though?

@greggman

Copy link
Copy Markdown
Member

A PNG with srgb or none might have already been converted to display-p3 by the browser on a display-p3 display? I guess the spec could say it requires raw data, but you'd only be able to test on a device where you thought the browser might be converting color spaces for images it usually displays? I wonder how many WebGL apps out there are using .PNGs that would suddenly start providing different data if used in WebGPU if there was no flag

@kainino0x kainino0x removed the tacit resolution queue Editors have agreed and intend to land if no feedback is given label Jul 26, 2023
@kainino0x
kainino0x merged commit eead183 into gpuweb:main Jul 26, 2023
@kainino0x
kainino0x deleted the imageelement branch July 26, 2023 19:21
@ccameron-chromium

Copy link
Copy Markdown
Contributor

A PNG with srgb or none might have already been converted to display-p3 by the browser on a display-p3 display?

We should be clear that a browser that does that is in error. (And is also leaking tons of fingerprinting bits). The color conversion to apply must be "the color space specified in the image (if any, sRGB if none)" to "the color space specified to copyExternalImageToTexture").

Of note is that colorSpaceConversion: 'none' was removed from WebCodecs shortly after it shipped. A better name for colorSpaceConversion: 'none' is ignoreImageICCProfiles: 'yes-please'.

@greggman

Copy link
Copy Markdown
Member

A better name for colorSpaceConversion: 'none' is ignoreImageICCProfiles: 'yes-please'.

That sounds very specific. There are many other forms of color profile / color space metadata. It seems better to have a flag that says to ignore all of them, rather than a flag to ignore just one of them. The intent is "give me the raw uncompressed data from the file" most likely because the data is not colors.

@ccameron-chromium

Copy link
Copy Markdown
Contributor

Of note is that colorSpaceConversion: 'none' was removed from WebCodecs shortly after it shipped.

Sorry, this is incorrect. I was thinking of premultiply alpha.

@ccameron-chromium

Copy link
Copy Markdown
Contributor

A better name for colorSpaceConversion: 'none' is ignoreImageICCProfiles: 'yes-please'.

That sounds very specific. There are many other forms of color profile / color space metadata. It seems better to have a flag that says to ignore all of them, rather than a flag to ignore just one of them. The intent is "give me the raw uncompressed data from the file" most likely because the data is not colors.

Yes. There are ICC profiles and CICP chunks.

For ImageBitmap, the HTML spec says:

When the steps above require that the user agent crop bitmap data to the source rectangle with formatting, the user agent must run the following steps:
[...]
9. If image is an img element or a Blob object, let val be the value of the colorSpaceConversion member of options, and then run these substeps:

  • If val is "default", the color space conversion behavior is implementation-specific, and should be chosen according to the default color space that the implementation uses for drawing images onto the canvas.
  • If val is "none", output must be decoded without performing any color space conversions. This means that the image decoding algorithm must ignore color profile metadata embedded in the source data as well as the display device color profile.

This only applies to HTMLImageElement and Blob (so it doesn't apply to videos, canvases, etc). The part about this only applying when a crop is required is terrifying, and I'm not sure what to do with it.

So a more realistic name would be ignoreImageColorProfileMetadata:true.

For reference, I added a test page (comparing 2D, WebGL, and WebGPU) here.

Over in another thread, junov@ suggested that this was added to compensate for a bug where Chrome would automatically convert images to the display's color space (and this was a flag to suppress that behavior). Chrome never does that anymore (it's a fingerprinting vector, if nothing else), so maybe this just isn't needed anymore (in any of its various incarnations).

aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 10, 2023
This CL adds support for HTMLImageElement and ImageData to
the GPUQueue copyExternalImageToTexture() method.

CTS: gpuweb/cts#2886
Spec: gpuweb/gpuweb#3430
Bug: 1471372
Change-Id: I52d4bfef4196b76a77e713d09231a54058c766c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4729404
Reviewed-by: Shaobo Yan <shaobo.yan@intel.com>
Commit-Queue: Fr <beaufort.francois@gmail.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Justin Novosad <junov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1182067}
@kainino0x kainino0x modified the milestones: Polish post-V1, Milestone 1 Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet proposal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants