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

RESOLVED FIXED in Firefox 46

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kamidphish, Assigned: ethlin)

Tracking

(Blocks: 1 bug)

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(8 attachments, 18 obsolete attachments)

942 bytes, patch
Details | Diff | Splinter Review
65.91 KB, patch
Details | Diff | Splinter Review
6.53 KB, patch
Details | Diff | Splinter Review
6.98 KB, patch
Details | Diff | Splinter Review
7.22 KB, patch
Details | Diff | Splinter Review
13.66 KB, patch
Details | Diff | Splinter Review
5.16 KB, patch
Details | Diff | Splinter Review
3.17 KB, patch
Details | Diff | Splinter Review
This mostly means converting from BGRA/RGBA UNSIGNED_BYTE to the correct format.
Created attachment 8661629 [details] [diff] [review]
Part 0: Remove copy-ctor from strong type.

Checking strong type is sorted on copy-ctor is overkill and annoying when debugging.
Attachment #8661629 - Flags: review?(jgilbert)
Created attachment 8661631 [details] [diff] [review]
Part 1: Sort all the WebGLTexelFormats.

In preparation for adding new formats and conversion code, sort the
existing ones into some semblance of order.
Attachment #8661631 - Flags: review?(jgilbert)
Created attachment 8661632 [details] [diff] [review]
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.
Attachment #8661632 - Flags: review?(jgilbert)
Created attachment 8661633 [details] [diff] [review]
Part 3: Texel Conversion - RG8 format.
Attachment #8661633 - Flags: review?(jgilbert)
Created attachment 8661634 [details] [diff] [review]
Part 4: Texel Conversion - RG16F format.
Attachment #8661634 - Flags: review?(jgilbert)
Created attachment 8661635 [details] [diff] [review]
Part 5: Texel Conversion - RG32F format.
Attachment #8661635 - Flags: review?(jgilbert)
Created attachment 8661636 [details] [diff] [review]
Part 6: Texel Conversion - RGB11F11F10F format.
Attachment #8661636 - Flags: review?(jgilbert)
Created attachment 8661637 [details] [diff] [review]
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.
Attachment #8661637 - Flags: review?(jgilbert)
Created attachment 8661638 [details] [diff] [review]
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.
Attachment #8661638 - Flags: review?(jgilbert)
Created attachment 8661639 [details] [diff] [review]
Part 9: Add BGRA8 and BGRX8 so EffectiveFormat can replace WebGLTexelFormat.
Attachment #8661639 - Flags: review?(jgilbert)
Created attachment 8661648 [details] [diff] [review]
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.
Created attachment 8661650 [details] [diff] [review]
Part B: Remove unneeded functions.
Attachment #8661650 - Flags: review?(jgilbert)
Created attachment 8661651 [details] [diff] [review]
Part C: Fix loading texture data from video elements.
Attachment #8661651 - Flags: review?(jgilbert)
Created attachment 8661652 [details] [diff] [review]
Part D: Zero pad GLenum in error message.
Attachment #8661652 - Flags: review?(jgilbert)
Created attachment 8661653 [details] [diff] [review]
Part E: Fix bytesPerPixel for Alpha/Lum/LumAlpha float formats.
Attachment #8661653 - Flags: review?(jgilbert)
Created attachment 8661654 [details] [diff] [review]
Part F: Unpack from GL_HALF_FLOAT.
Attachment #8661654 - Flags: review?(jgilbert)
Attachment #8661648 - Flags: review?(jgilbert)
Attachment #8661629 - Flags: review?(jgilbert) → review+
Attachment #8661631 - Flags: review?(jgilbert) → review+
Attachment #8661632 - Flags: review?(jgilbert) → review+
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?
Attachment #8661633 - Flags: review?(jgilbert) → review+
Attachment #8661634 - Flags: review?(jgilbert) → review+
Attachment #8661635 - Flags: review?(jgilbert) → review+
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?
Attachment #8661636 - Flags: review?(jgilbert) → review+
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*
Attachment #8661637 - Flags: review?(jgilbert) → review+
Attachment #8661638 - Flags: review?(jgilbert) → review+
Attachment #8661639 - Flags: review?(jgilbert) → review+
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.
Attachment #8661648 - Flags: review?(jgilbert) → review-
Comment on attachment 8661650 [details] [diff] [review]
Part B: Remove unneeded functions.

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

\o/
Attachment #8661650 - Flags: review?(jgilbert) → review+
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.
Attachment #8661651 - Flags: review?(jgilbert) → review+
Attachment #8661652 - Flags: review?(jgilbert) → review+
Attachment #8661653 - Flags: review?(jgilbert) → review+
Attachment #8661654 - Flags: review?(jgilbert) → review+
Almost there!
(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.
(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.
(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.
(Assignee)

Comment 27

3 years ago
Is there any work need to be done?
Flags: needinfo?(jgilbert)
Yes. :kamidphish, can you fix this last r-'d patch, or pass it off to :ethlin?
Flags: needinfo?(jgilbert) → needinfo?(dglastonbury)
(Assignee)

Comment 29

3 years ago
As per irc discussion with kamidphish, I will try to fix this.
Flags: needinfo?(dglastonbury)
(Assignee)

Updated

3 years ago
Assignee: dglastonbury → ethlin
(Assignee)

Comment 30

3 years ago
Created attachment 8701323 [details] [diff] [review]
Part 0: Remove copy-ctor from strong type. (carry r+: jgilbert)

rebase the patch
Attachment #8661629 - Attachment is obsolete: true
(Assignee)

Comment 31

3 years ago
Created attachment 8701324 [details] [diff] [review]
Part 1: Sort all the WebGLTexelFormats. (carry r+: jgilbert)

rebase the patch
Attachment #8661631 - Attachment is obsolete: true
Attachment #8661632 - Attachment is obsolete: true
(Assignee)

Comment 32

3 years ago
Created attachment 8701343 [details] [diff] [review]
Part 2: Texel Conversion - RG8 format. (carry r+: jgilbert)

rebase the patch
Attachment #8661633 - Attachment is obsolete: true
(Assignee)

Comment 33

3 years ago
Created attachment 8701346 [details] [diff] [review]
Part 3: Texel Conversion - RG16F format. (carry r+: jgilbert)

rebase the patch
Attachment #8661634 - Attachment is obsolete: true
(Assignee)

Comment 34

3 years ago
Created attachment 8701348 [details] [diff] [review]
Part 4: Texel Conversion - RG32F format. (carry r+: jgilbert)

rebase the patch
Attachment #8661635 - Attachment is obsolete: true
(Assignee)

Comment 35

3 years ago
Created attachment 8701349 [details] [diff] [review]
Part 5: Texel Conversion - RGB11F11F10F format. (carry r+: jgilbert)

rebase the patch.
Attachment #8661636 - Attachment is obsolete: true
(Assignee)

Comment 36

3 years ago
Created attachment 8701350 [details] [diff] [review]
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.
Attachment #8661638 - Attachment is obsolete: true
Attachment #8661639 - Attachment is obsolete: true
Attachment #8661648 - Attachment is obsolete: true
Attachment #8661651 - Attachment is obsolete: true
Attachment #8661652 - Attachment is obsolete: true
Attachment #8661653 - Attachment is obsolete: true
Attachment #8661654 - Attachment is obsolete: true
Attachment #8661637 - Attachment is obsolete: true
Attachment #8701350 - Flags: review?(jgilbert)
(Assignee)

Comment 37

3 years ago
Created attachment 8701352 [details] [diff] [review]
Part 7: Remove unneeded functions. (carry r+: jgilbert)

rebase the patch.
Attachment #8661650 - Attachment is obsolete: true
(Assignee)

Comment 38

3 years ago
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 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.
Attachment #8701350 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 40

3 years ago
Created attachment 8702467 [details] [diff] [review]
Part 6: Fix TexImage2D and TexSubImage2D with texture conversion. r=jgilbert

Apply jgilbert's comment.
Attachment #8701350 - Attachment is obsolete: true
Attachment #8702467 - Flags: review?(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.
Attachment #8702467 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 42

3 years ago
Created attachment 8702777 [details] [diff] [review]
Part 6: Fix TexImage2D and TexSubImage2D with texture conversion. (carry r+: jgilbert)

Set 4-space indents.
Attachment #8702467 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.