KhronosGroup / WebGL

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

RGB10_A2 invalid for DOM uploads per spec, but required in conformance tests #2645

Open kdashg opened 6 years ago

kdashg commented 6 years ago

"When the data source is a DOM element [...] each channel's representation is an unsigned integer type of at least 8 bits. Converting such representation to signed integers or unsigned integers with more bits is not clearly defined. [...] Therefore, only converting to unsigned integer of at most 8 bits, half float, or float is allowed."

I'm triaging our test failures, and Firefox is failing these tests: "Error: WebGL warning: texImage2D: Format or type is invalid for DOM sources."

Any objections to removing these tests? (and replacing with negative tests?)

@kenrussell

kainino0x commented 6 years ago

I don't know exactly what the intent of that spec text is. But it seems to me that it's actually concerned with uploading to integer formats. I'm pretty sure RGB10_A2 is a UNORM format (i.e. it takes on values in the [0.0, 1.0] range, discretized), so it seems to be well defined that the range should be remapped. (I think it's just as well defined as upload to RGBA4 (also UNORM), which is already allowed.)

(By this logic, RGB10_A2UI should still be disallowed.)

zhenyao commented 6 years ago

RGB10_A2 is in the spec actually @jdashg

kdashg commented 6 years ago

It is not clear that it's talking solely about integers as opposed to normalized ints: "When the data source is a DOM element (HTMLImageElement, HTMLCanvasElement, or HTMLVideoElement), or is an ImageBitmap or ImageData object, commonly each channel's representation is an unsigned integer type of at least 8 bits. Converting such representation to signed integers or unsigned integers with more bits is not clearly defined. For example, when converting RGBA8 to RGBA16UI, it is unclear whether or not the intention is to scale up values to the full range of a 16-bit unsigned integer. Therefore, only converting to unsigned integer of at most 8 bits, half float, or float is allowed."

I can update the rationale so that it's clear.

I think offering conversions at all was a mistake, and that expanding the acceptable formats for WebGL 2 was a huge mistake.

RGB10_A2 was not acceptable previous to #2400. (to fix issue #2274) Only Chrome engineers were pinged on #2400, which is disappointing, and may be why I don't remember this. WebGL 2 had already been released in both Firefox and Chrome at this point.

kdashg commented 6 years ago

There's mention that we should talk about it on the conference call, so that very well may have happened. But I would like extra awareness when we're making changes to the spec, as opposed to just bug fixes.

kenrussell commented 6 years ago

Sorry for any lack of communication or miscommunication. Yes, the intent of that spec text was to disqualify the new ES 3.0 signed integer and unsigned integer formats. The support for RGB10_A2 was added in order to allow HDR image formats to be uploaded to WebGL. As this is a unorm format like RGBA8, conversion of values is unambiguous.

Let's not remove support for HDR images – it's important in order to keep WebGL's functionality up to date – but rather clarify the spec as needed.

lexaknyazev commented 5 years ago

allow HDR image formats to be uploaded to WebGL

Given that this issue talks about DOM uploads, does it mean that a PNG image with 16 bits depth per channel will be properly converted to 10 bits per color channel normalized RGB10_A2 datatype, i.e., the conversion will not be 16->8->10?

kenrussell commented 5 years ago

That's the intent, yes. Any conformance tests you can add which would verify this behavior across browsers will be gladly integrated.

lexaknyazev commented 5 years ago

Any conformance tests you can add which would verify this behavior across browsers will be gladly integrated.

@kenrussell Opened #2789 to track CTS updates.