1205168 - WebGL 2 - Make loading textures from canvas elements work for all new texture formats.

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[u480271]

This mostly means converting from BGRA/RGBA UNSIGNED_BYTE to the correct format.

Comment 1

[u480271]

Attachment: Part 0: Remove copy-ctor from strong type.

Checking strong type is sorted on copy-ctor is overkill and annoying when debugging.

Comment 2

[u480271]

Attachment: Part 1: Sort all the WebGLTexelFormats.

In preparation for adding new formats and conversion code, sort the
existing ones into some semblance of order.

Comment 3

[u480271]

Attachment: Part 2: Fix ValidateTexImageType for WebGL 2 texture types.

Function was rejecting valid types that are exposed via extensions in
WebGL 1 but are code in WebGL 2.

Comment 4

[u480271]

Attachment: Part 3: Texel Conversion - RG8 format.

Comment 5

[u480271]

Attachment: Part 4: Texel Conversion - RG16F format.

Comment 6

[u480271]

Attachment: Part 5: Texel Conversion - RG32F format.

Comment 7

[u480271]

Attachment: Part 6: Texel Conversion - RGB11F11F10F format.

Comment 8

[u480271]

Attachment: Part 7: Extract pixel data and params from arguments into structure.

Plus code structure cleanup in TexImage2D_base and TexSubImage2D_base to
make the functions more similar.

Comment 9

[u480271]

Attachment: Part 8: Stop messing with formats.

Strip back DriverFormatsForEffectiveInternalFormat function to do the
least work given the intention that the formats will have been through
validation and conditioning before the final conversion to driver
requirements.

Comment 10

[u480271]

Attachment: Part 9: Add BGRA8 and BGRX8 so EffectiveFormat can replace WebGLTexelFormat.

Comment 11

[u480271]

Attachment: Part A: Fix TexImage2D and TexSubImage2D with texture conversion. r=gilbert

Rework handling of texture uploads, especially for cases that require
conversion of source data.

Changed to to track three distinct pieces of information:
1) The 'effective sized' internal format of the texture,
2) The 'desired' source texture format (derived from
   unpackFormat/unpackType),
3) The 'actual' source texture format (for the supplied pixel data)

Formats 2) and 3) will be different when loading data from DOM elements
or ImageData.

Replaces WebGLTexelFormat with EffectiveFormat and format tables which
simplifies this code greatly. Their exists much commonality between
TexImage2D_base and TexSubImage2D_base suggesting that those functions
can be refactored to ensure `one definition` of format selection,
validation and conversion.

Comment 12

[u480271]

Attachment: Part B: Remove unneeded functions.

Comment 13

[u480271]

Attachment: Part C: Fix loading texture data from video elements.

Comment 14

[u480271]

Attachment: Part D: Zero pad GLenum in error message.

Comment 15

[u480271]

Attachment: Part E: Fix bytesPerPixel for Alpha/Lum/LumAlpha float formats.

Comment 16

[u480271]

Attachment: Part F: Unpack from GL_HALF_FLOAT.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8661633 [details] [diff] [review]
Part 3: Texel Conversion - RG8 format.

Review of attachment 8661633 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLTexelConversions.h
@@ +772,5 @@
> +    float scaleFactor = src[3] / 255.0f;
> +    uint8_t srcR = static_cast<uint8_t>(src[0] * scaleFactor);
> +    uint8_t srcG = static_cast<uint8_t>(src[1] * scaleFactor);
> +    dst[0] = srcR;
> +    dst[1] = srcG;

Why not assign directly to dst? Why have srcR/G temporaries?

Comment 18

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8661636 [details] [diff] [review]
Part 6: Texel Conversion - RGB11F11F10F format.

Review of attachment 8661636 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLTexelConversions.h
@@ +151,5 @@
> +// These routines come from angle/common/mathutil.h
> +// They are copied here to remove the dependency on ANGLE headers
> +// included from mathutil.h
> +MOZ_ALWAYS_INLINE uint16_t
> +packToFloat11(float fp32)

I would honestly rather INVALID_OP for this format.

@@ +1063,5 @@
>  
>  template<> MOZ_ALWAYS_INLINE void
> +pack<WebGLTexelFormat::RGB11F11F10F, WebGLTexelPremultiplicationOp::None, float, uint32_t>(const float* __restrict src, uint32_t* __restrict dst)
> +{
> +    dst[0] = ((packToFloat11(src[0]) <<  0) |

Comment as to why this is packed backwards. IIRC it's because this is uploaded as UNSIGNED_INT_11F_11F_10F_REV?

Comment 19

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8661637 [details] [diff] [review]
Part 7: Extract pixel data and params from arguments into structure.

Review of attachment 8661637 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLTextureUpload.cpp
@@ +20,5 @@
>  #include "WebGLTexelConversions.h"
>  
>  namespace mozilla {
>  
> +struct PixelUnpackSource

Oh man, this is going to bitrot me hardcore.

@@ +571,4 @@
>          size_t   bitsPerTexel = GetBitsPerTexel(effectiveInternalFormat);
>          MOZ_ASSERT((bitsPerTexel % 8) == 0); // should not have compressed formats here.
>          size_t   dstTexelSize = bitsPerTexel / 8;
> +        size_t   srcStride = (source.stride) ? source.stride : checked_alignedRowSize.value();

These parens seem unnecessary.

@@ +857,4 @@
>              return;
>          }
>          if (!mContext->ConvertImage(width, height, srcStride, dstStride,
> +                                    (uint8_t*) source.pixels, convertedData,

const uint8_t*

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8661648 [details] [diff] [review]
Part A: Fix TexImage2D and TexSubImage2D with texture conversion. r=gilbert

Review of attachment 8661648 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLFormats.cpp
@@ +334,5 @@
> +    ////////////////////////////////////////////////////////////////////////////
> +    // GLES 3.0.4 p111-113
> +    // RGBA
> +    AddUnpackTuple(LOCAL_GL_RGBA, LOCAL_GL_BYTE                       , EffectiveFormat::RGBA8_SNORM );
> +    AddUnpackTuple(LOCAL_GL_RGBA, LOCAL_GL_UNSIGNED_INT_2_10_10_10_REV, EffectiveFormat::RGB10_A2    );

This section basically mirrors what's done in CreateForWebGL2. We should really not have two different parts need to be the same.

@@ +499,5 @@
>      // GLES 3.0.4, p205-206, "Required Renderbuffer Formats"
>      FOO(STENCIL_INDEX8);
>  
> +    // GLES 3.0.4, p128, table 3.12.
> +    AddSizedFormat(LOCAL_GL_ALPHA8,            EffectiveFormat::Alpha8);

ALPHA8 is not a valid sized type. Alpha8 is only an effective type, not a sized type. r-

::: dom/canvas/WebGLTextureUpload.cpp
@@ +69,5 @@
> +static size_t
> +CalculateStrideInBytes(int width, int bytesPerTexel, int alignment)
> +{
> +    // TODO: Why go through all the trouble of converting to
> +    // CheckedUint32 and then not checking the result is valid?

We implicitly get the assert from .value() that it's valid.

@@ +77,5 @@
> +    return stride.value();
> +}
> +
> +// Helper to convert EffectiveFormat into the equivalent GLenum.
> +// This is used to store the "effective" internal format in ImageInfo and can be removed

Ok.

@@ +672,5 @@
>          return;
>      }
>  
> +    // desiredSrcInfo is the format/type that has been requested to be unpacked
> +    const auto* desiredSrcInfo = webgl::GetInfoByUnpackTuple(unpackFormat, unpackType);

This early use of FormatInfo is risky, and we should really wait until 44 so we can hope to land all the FormatInfo patches together. It's good news that it seems to be working even without my patches.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8661650 [details] [diff] [review]
Part B: Remove unneeded functions.

Review of attachment 8661650 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 22

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8661651 [details] [diff] [review]
Part C: Fix loading texture data from video elements.

Review of attachment 8661651 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLTextureUpload.cpp
@@ +1135,5 @@
> +
> +    // Check that the image format is suitable for blitting using a
> +    // FBO. This is determined by looking for a valid format usage for
> +    // the effective internal format and checking that it's
> +    // renderable.

Cool.

Comment 23

[Kelsey Gilbert [:jgilbert]]

Almost there!

Comment 24

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #17)
> Comment on attachment 8661633 [details] [diff] [review]
> Part 3: Texel Conversion - RG8 format.
> 
> Review of attachment 8661633 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLTexelConversions.h
> @@ +772,5 @@
> > +    float scaleFactor = src[3] / 255.0f;
> > +    uint8_t srcR = static_cast<uint8_t>(src[0] * scaleFactor);
> > +    uint8_t srcG = static_cast<uint8_t>(src[1] * scaleFactor);
> > +    dst[0] = srcR;
> > +    dst[1] = srcG;
> 
> Why not assign directly to dst? Why have srcR/G temporaries?

Just to keep the "theme" with the existing functions that did it that way.

Comment 25

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #20)
> Comment on attachment 8661648 [details] [diff] [review]
> Part A: Fix TexImage2D and TexSubImage2D with texture conversion. r=gilbert

> @@ +499,5 @@
> >      // GLES 3.0.4, p205-206, "Required Renderbuffer Formats"
> >      FOO(STENCIL_INDEX8);
> >  
> > +    // GLES 3.0.4, p128, table 3.12.
> > +    AddSizedFormat(LOCAL_GL_ALPHA8,            EffectiveFormat::Alpha8);
> 
> ALPHA8 is not a valid sized type. Alpha8 is only an effective type, not a
> sized type. r-

I know that `conceptually` Alpha8 is only an `effective` type, but I had to add this to make the unified "query format tables" work. Off the top-of-my-head, I can't remember why, but I think it may have been to support conversion from what the texture stores into a FormatInfo structure. If this is true, and we instead store a pointer to FormatInfo in the texture, the need to do look-ups would be removed.

Comment 26

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #25)
> (In reply to Jeff Gilbert [:jgilbert] from comment #20)
> > Comment on attachment 8661648 [details] [diff] [review]
> > Part A: Fix TexImage2D and TexSubImage2D with texture conversion. r=gilbert
> 
> > @@ +499,5 @@
> > >      // GLES 3.0.4, p205-206, "Required Renderbuffer Formats"
> > >      FOO(STENCIL_INDEX8);
> > >  
> > > +    // GLES 3.0.4, p128, table 3.12.
> > > +    AddSizedFormat(LOCAL_GL_ALPHA8,            EffectiveFormat::Alpha8);
> > 
> > ALPHA8 is not a valid sized type. Alpha8 is only an effective type, not a
> > sized type. r-
> 
> I know that `conceptually` Alpha8 is only an `effective` type, but I had to
> add this to make the unified "query format tables" work. Off the
> top-of-my-head, I can't remember why, but I think it may have been to
> support conversion from what the texture stores into a FormatInfo structure.
> If this is true, and we instead store a pointer to FormatInfo in the
> texture, the need to do look-ups would be removed.

Please handle these in calling code then. It really messes with the guarantees of the system to hack these in.

Comment 27

[Ethan Lin[:ethlin]]

Is there any work need to be done?

Comment 28

[Kelsey Gilbert [:jgilbert]]

Yes. :kamidphish, can you fix this last r-'d patch, or pass it off to :ethlin?

Comment 29

[Ethan Lin[:ethlin]]

As per irc discussion with kamidphish, I will try to fix this.

Comment 30

[Ethan Lin[:ethlin]]

Attachment: Part 0: Remove copy-ctor from strong type. (carry r+: jgilbert)

rebase the patch

Comment 31

[Ethan Lin[:ethlin]]

Attachment: Part 1: Sort all the WebGLTexelFormats. (carry r+: jgilbert)

rebase the patch

Comment 32

[Ethan Lin[:ethlin]]

Attachment: Part 2: Texel Conversion - RG8 format. (carry r+: jgilbert)

rebase the patch

Comment 33

[Ethan Lin[:ethlin]]

Attachment: Part 3: Texel Conversion - RG16F format. (carry r+: jgilbert)

rebase the patch

Comment 34

[Ethan Lin[:ethlin]]

Attachment: Part 4: Texel Conversion - RG32F format. (carry r+: jgilbert)

rebase the patch

Comment 35

[Ethan Lin[:ethlin]]

Attachment: Part 5: Texel Conversion - RGB11F11F10F format. (carry r+: jgilbert)

rebase the patch.

Comment 36

[Ethan Lin[:ethlin]]

Attachment: Part 6: Fix TexImage2D and TexSubImage2D with texture conversion. r=jgilbert

Add new formats in GetFormatForPackingTuple. Let TexImage2D and TexSubImage2D work with new texture format.

Comment 37

[Ethan Lin[:ethlin]]

Attachment: Part 7: Remove unneeded functions. (carry r+: jgilbert)

rebase the patch.

Comment 38

[Ethan Lin[:ethlin]]

Rebase the original patches and add the new formats into current texImage2D flow. I tried the conformance test "conformance2/textures/canvas" and "conformance2/textures/video" which are all passed.

Comment 39

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8701350 [details] [diff] [review]
Part 6: Fix TexImage2D and TexSubImage2D with texture conversion. r=jgilbert

Review of attachment 8701350 [details] [diff] [review]:
-----------------------------------------------------------------

Use case fallthrough so we don't have a ton of duplicate case blocks.

::: dom/canvas/TexUnpackBlob.cpp
@@ +526,5 @@
> +        case LOCAL_GL_RG:
> +            *out_texelFormat = WebGLTexelFormat::RG8;
> +            return true;
> +
> +        case LOCAL_GL_RG_INTEGER:

Use fallthrough from RG.

@@ +530,5 @@
> +        case LOCAL_GL_RG_INTEGER:
> +            *out_texelFormat = WebGLTexelFormat::RG8;
> +            return true;
> +
> +        case LOCAL_GL_RGB_INTEGER:

Use fallthrough from RGB.

@@ +534,5 @@
> +        case LOCAL_GL_RGB_INTEGER:
> +            *out_texelFormat = WebGLTexelFormat::RGB8;
> +            return true;
> +
> +        case LOCAL_GL_RGBA_INTEGER:

Use fallthrough from RGBA.

Comment 40

[Ethan Lin[:ethlin]]

Attachment: Part 6: Fix TexImage2D and TexSubImage2D with texture conversion. r=jgilbert

Apply jgilbert's comment.

Comment 41

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8702467 [details] [diff] [review]
Part 6: Fix TexImage2D and TexSubImage2D with texture conversion. r=jgilbert

Review of attachment 8702467 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/TexUnpackBlob.cpp
@@ +633,5 @@
>          }
>          break;
>  
> +    case LOCAL_GL_UNSIGNED_INT_10F_11F_11F_REV:
> +      switch (packingFormat) {

4-space indents.

Comment 42

[Ethan Lin[:ethlin]]

Attachment: Part 6: Fix TexImage2D and TexSubImage2D with texture conversion. (carry r+: jgilbert)

Set 4-space indents.

Comment 43

[Ethan Lin[:ethlin]]

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a6bb7973de6

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3ad01d19a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3fb7f0520ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fdcec9e1aff
https://hg.mozilla.org/integration/mozilla-inbound/rev/88c701885500
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2206855967b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3903b1b26739
https://hg.mozilla.org/integration/mozilla-inbound/rev/a083bc5f439d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2133ad4a738

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee3ad01d19a7
https://hg.mozilla.org/mozilla-central/rev/d3fb7f0520ad
https://hg.mozilla.org/mozilla-central/rev/6fdcec9e1aff
https://hg.mozilla.org/mozilla-central/rev/88c701885500
https://hg.mozilla.org/mozilla-central/rev/b2206855967b
https://hg.mozilla.org/mozilla-central/rev/3903b1b26739
https://hg.mozilla.org/mozilla-central/rev/a083bc5f439d
https://hg.mozilla.org/mozilla-central/rev/c2133ad4a738