Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify UNPACK_PREMULTIPLY_ALPHA for floating-point and integer pixel formats #3667

Open
lexaknyazev opened this issue Jul 24, 2024 · 6 comments

Comments

@lexaknyazev
Copy link
Member

lexaknyazev commented Jul 24, 2024

The UNPACK_PREMULTIPLY_ALPHA_WEBGL parameter is defined in WebGL 1.0 spec that on its own supports only unsigned normalized pixel formats.

The WebGL 2.0 and the related WebGL extension specs should define how this parameter works with:

  • RGBA and LUMINANCE_ALPHA formats with FLOAT and HALF_FLOAT types. Specifically, how floating-point overflow/underflow is handled.
  • RGBA_INTEGER format. Specifically, how signed/unsigned integer overflow is handled.
  • RGBA format with BYTE and SHORT (from EXT_texture_norm16) types.
@kdashg
Copy link
Contributor

kdashg commented Aug 8, 2024

I believe we should specifiy that:

  • For float formats: ieee math happens.
  • Int formats: clamping happens, and 0/0 is 0.
  • Normalized (incl signed) formats: clamping happens, 0/0 is 0.

@lexaknyazev
Copy link
Member Author

ieee math happens

Inf and NaN included?

Int formats: clamping happens

  • sint8: (-128, 0, 0, -1) => (+127, 0, 0, -1)?
  • sint8: (-128, 0, 0, +2) => (-128, 0, 0, +2)?
  • uint8: (127, 0, 0, 127) => (127, 0, 0, 127)?

Normalized (incl signed) formats: clamping happens

Since the values are normalized, multiplication results cannot be outside of [0, 1] or [-1, +1] ranges. The only tricky edge case is that -128 and -32768 must be treated as -127 and -32767 for the purposes of normalization.

@kenrussell
Copy link
Member

Notes from the WebGL concall:

Inf and NaN included?

Thinking: we do whatever the CPU hardware does.

Int formats: clamping happens

Let's say that the integer math's done with 32 bits of precision and then clamp back to the 8-bit range. Or, say that clamping isn't better than masking? Not sure what's better.

Meta-question, should we be premultiplying integer values at all? Consider ignoring the state for these non-normalized integer textures.

More discussion to come.

@kdashg
Copy link
Contributor

kdashg commented Sep 5, 2024

I wish we had never applied this setting to ArrayBufferView data. It really only makes sense for images, where you're trying to ensure consistency.
This should be viewed like unpackColorSpace, where it sets the destination alpha premultiplication state when we call e.g. texImage(img). Different sources have different alpha premult states: Canvases are generally alpha-premult:true, whereas PNG is stored alpha-premult:false. Setting UNPACK_PREMULTIPLY_ALPHA:true means that for canvases, we do no alpha-premult-conversion, whereas for PNG, we multiply color by alpha before the color conversions step. Likewise for UNPACK_PREMULTIPLY_ALPHA:false, we have to "unpremultiply" by dividing color by alpha in order to make the data alpha-premult:false, whereas for PNG, we do not change the data.
It's important to view these as the destination setters, rather than operations. (This also applies to the y-flip setting)

I believe Firefox warns if you use these things (such as flip-y) on ArrayBufferView data.

@kdashg
Copy link
Contributor

kdashg commented Sep 6, 2024

In WG, we decided that we would try to spec UNPACK_PREMULTIPLY_ALPHA as ignored for non-TexImageSources (thus excluding ArrayBufferViews).
Next step is writing a test to go with the spec change, and seeing if that breaks any implementations.

@kenrussell
Copy link
Member

Additional information from WebGL WG meeting of 2024-09-19:

One remaining problem is uploading canvases to RGBA8UI textures with premultiplied alpha: https://registry.khronos.org/webgl/specs/latest/2.0/#3.7.6 . That's the only problematic format in the table. The WG resolved to test this, and if browsers behave differently, carve it out from the rest of the spec update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants