KhronosGroup / WebGL

The Official Khronos WebGL Repository
Other
2.64k stars 669 forks source link

Should do color space mapping when upload DOM elemet to sRGB formated texture? #2165

Open qkmiao opened 7 years ago

qkmiao commented 7 years ago

Current conformance test doesn't do color space mapping for such case. What do you think? @kenrussell @zhenyao

kenrussell commented 7 years ago

Different browsers currently behave differently when decoding resources like PNG images. For example, Chromium currently bakes in the display's color profile to the decoded image data. This causes the actual values extracted from the image to differ from machine to machine.

For this reason I think the UNPACK_COLORSPACE_CONVERSION_WEBGL flag should still be set to false for the majority of these tests. (I assume we're talking about all of the sRGB format tests underneath sdk/tests/conformance2/textures/.)

There may be a good reason to test the application of the embedded color profile in JPEG or PNG images when uploading to sRGB textures. We should carefully consider exactly what we intend to test, and make sure all of the browser implementers are on board.

CC @jdashg @grorg @RafaelCintron @zhenyao @kainino0x

kenrussell commented 7 years ago

Also @junov

qkmiao commented 7 years ago

I think the UNPACK_COLORSPACE_CONVERSION_WEBGL flag doesn't relate to the internalformat argument of texImage2D. It's for image decoding. And this flag will be ignored if the TexImageSource is an imagebitmatp per WebGL 1 spec: https://www.khronos.org/registry/webgl/specs/latest/1.0/#PIXEL_STORAGE_PARAMETERS.

What I want to know is when we upload a DOM element such as imagebitmap to a sRGB texture do we need to convert pixels from DOM element to sRGB format?

kenrussell commented 7 years ago

An ImageBitmap can wrap multiple sources. It can be created from an HTMLImageElement, an HTMLCanvasElement, HTMLVideoElement, ImageData, or Blob (or another ImageBitmap).

I don't think uploading the ImageBitmap should imply any pixel conversion. Otherwise the semantics are too difficult to explain. Instead any conversion should be done before the ImageBitmap is fully created, and the Promise that resulted from creating it is resolved.

CC'ing @xidachen for more comment.

A concrete example would help us understand the motivation for this question.

qkmiao commented 7 years ago

When I implemented GPU-GPU path for TexImage2D, I have to solve this issue. In my current implementation of CopyTextureCHROMIUM extension, copying source texture (in RGBA format) to dest texture (in SRGB8_ALPHA8 format) will do linear to sRGB conversion. If TexImage2D doesn't need to consider color space conversion, I will not enable GPU path for sRGB format.

xidachen commented 7 years ago

I agree with @kenrussell that the color space conversion stage should happen during the creation of ImageBitmap. When createImageBitmap promise is resolved, color space conversion should have been done.

I don't think I fully understand the question here. I remember when I wrote conformance tests for ImageBitmap, we can specify the dest format, and if the dest format is not RGBA, we have to go through a pixel conversion. Isn't that always true regardless whether ImageBitmap does the color space conversion or not?

qkmiao commented 7 years ago

"When createImageBitmap promise is resolved, color space conversion should have been done." I think at this time it's not in sRGB color space if dest format is sRGB.

My question is after createImageBitmap promise is resolved, should we do another color conversion to sRGB when extracting pixels from ImageBitmap if dest format is sRGB. In current test, if color is 128 in ImageBitmap, 128 is directly written to sRGB dest texture. After sampling and drawing with the sRGB dest texture, we expect 55 in RGBA color space. If we do sRGB conversion before writing color to dest texture, 188 (linearToSRGB(128)) should be written to sRGB dest texture. After sampling and drawing with the sRGB dest texture, we expect 128 in RGBA color space.

qkmiao commented 7 years ago

Not only ImageBitmap should consider this color space conversion but also cavas2d and webgl canvas.

xidachen commented 7 years ago

@zakerinasab Reza is working on color space conversion.

Regarding the texture upload. Currently when we extract color from an ImageBitmap, it is in RGBA color space. But Reza is making changes such that the extracted pixel color will be in sRGB color space. When that is done, I don't think we need the extra conversion (linearToSRGB(128)). Would that make sense?

zakerinasab commented 7 years ago

The current createImageBitmap allows to specify two color space conversion modes: none and default. The new "Canvas Color Space proposal" extends this to support other color spaces. Implementation-wise, in the current phase we are working on supporting these color space conversions in Chromium:

qkmiao commented 7 years ago

Even though other color spaces (i.e. srgb and linear-rgb) are supported, we still need to make decision on which color space should be selected when uploading imageBitmap to a texture according the format of dest texture. If you create ImageBitmap in srgb color space, when you upload it to a rgba or srgb dest texture, what should you do to the extracted color for dest texture in different format?

As I pointed, we also need to consider this problem when we upload a 2d canvas or a webgl canvas to a texture. Should we do color space conversion between extracted color and dest format? Hope I explain it clearly.

xidachen commented 7 years ago

Canvas color space work has just started, so let's not make it block your implementation of GPU-GPU texture upload task. At this moment, we don't have any color space conversion when we extract color from the canvas, or ImageBitmap, so I think the color space conversion is needed when the extracted color and the dest format is different.

@qkmiao @kenrussell Does that make sense?

qkmiao commented 7 years ago

I agree with @xidachen. I think it's reasonable to do color space conversion when the extracted color and the dest format are different. @kenrussell @zhenyao and other WebGL WG members, what do you think?

zhenyao commented 7 years ago

In ES3, both ReadPixels and Tex{Sub}Image code path are unaware of sRGB encoding. I think the intention is to leave the data conversion to developers. Following that philosophy, should we just do nothing in Tex{Sub}Image calls when sRGB formats are used?

qkmiao commented 7 years ago

@zhenyao I think it's not reasonable for ImageSource since there is no chance for Web developer to do data conversion when they specify texture from ImageSource such as video. After the new "Canvas Color Space proposal" is available, web developer can use it to do color space conversion for Canvas and ImageBitmap.

No matther what decision we make, it's better to write it explicitly in WebGL spec to eliminate ambiguity.

kdashg commented 7 years ago

I would love to do as little conversion as possible. However, the genie is out of the bottle for HTMLMediaElement uploads.

I am not interested in trying to make these conversions fast, but I agree we must be clear about what these do. (and ideally, don't do)

For further discussion, I think it would help to have concrete examples of commands, and decide how they should behave. There is enough terminology being thrown around that it can be hard to follow arguments in the abstract.

heycam commented 2 years ago

Recently I've been looking at support for 2D canvas with a Display P3 color space backing store in WebKit. As part of this work, I caused some WebGL tests to fail, because of assumptions about what to do with the color space of video ImageSources.

I agree with others that it's unreasonable to expect that the author is always in the position to handle ImageSource data in its original color space. There's nothing in the Web platform that allows the author to inspect a video or image and get its color space.

I think that by default, ImageSource data should be converted to the color space of the canvas. The Canvas Color Space feature was merged into the HTML spec earlier this year, so that would mean either sRGB or Display P3. This would be consistent with how CanvasImageSources are treated in 2D canvas.

(I don't think it's unreasonable to provide the author with a way to avoid the conversion, if they want to opt out of it. But if the author wants to avoid conversion, they might prefer to get access to the YUV data in a video ImageSource rather than the converted Rec.601/709/whatever RGB data. I'll leave it to others to think about this.)