749711 - WebGL texture conversions: support conversions increasing texel size, conversion from int to float formats, support RGBA32F, avoid compiling useless paths, templatize pack/unpack routines, clean up, orange juice, potatoes, toothpaste

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Testcase:

http://hg.mozilla.org/users/bjacob_mozilla.com/webgl-perf-tests/raw-file/c08667106fb1/convert-Image-to-rgb-float.html

Result: [13:53:28.751] Error: WebGL: texSubImage2D: not enough data for operation (need 12582912, have 4194304) @ http://hg.mozilla.org/users/bjacob_mozilla.com/webgl-perf-tests/raw-file/c08667106fb1/TestTextureConversion.js:72

This is a multiple bug:

1. in TexImage2D_base and TexSubImage2D_base, the texelSize variable is used for both the source and destination texel size. Validation is bogus; converting to a wider texel size gets wrongly rejected. The reason why we've not hit this case before is that aside from float texture formats, it's pretty hard to hit a case where the conversion increases the texel size. You'd have to be on a platform that uses 16bpp for DOM elements.

2. The whole texture format conversion code is built around the assumption that one never converts between integer and float formats. This seems wrong; nothing in the OES_texture_float spec seems to prevent that. To solve that, we have to rearchitect the texel conversions: it's no longer enough to do just unpack the src and pack to the dst format, as in the case of a int-to-float conversion, we need to insert a 3rd step in the middle where we convert from int to float. It's probably the right time to templatize WebGLTexelConversions.h.

3. The RGBA32F format seems completely unsupported. Not hit by above testcase which does RGB float.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: WIP patch (crashes)

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: WIP patch. works, needs some polishing / commenting

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Yoohoo, with this patch, the size of all the texture conversion code goes down from 44k to 28k. Not that it was big enough to matter, but I was concerned about not increasing code size and the patch is designed to avoid compiling useless paths, so it's nice to see it's working.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: WIP patch. works, needs some polishing / commenting

With this patch, we are down to 17k !

The key (to going down from 28k to 17k) was to remove the useless premult/unpremult paths on SrcFormats that either don't have alpha, or have only alpha.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Redo texture conversions

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 619632 [details] [diff] [review]
Redo texture conversions

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

(Un)Premultiplication is best done with a LUT.

I can provide the bitwise premult/unpremult from my earlier work on this, too.

::: content/canvas/src/WebGLContext.h
@@ +804,5 @@
>      nsresult TexParameter_base(WebGLenum target, WebGLenum pname,
>                                 WebGLint *intParamPtr, WebGLfloat *floatParamPtr);
>  
>      void ConvertImage(size_t width, size_t height, size_t srcStride, size_t dstStride,
> +                      const PRUint8* src, PRUint8 *dst,

Pick either left-stars or right-stars. (I prefer left-stars)

::: content/canvas/src/WebGLTexelConversions.cpp
@@ +20,5 @@
> + */
> +class WebGLImageConverter
> +{
> +    const size_t mWidth, mHeight;
> +    const void * const mSrcStart;

Can we do 'void*' instead of leaving the star hanging in the middle, there?

@@ +224,5 @@
> +            case WebGLTexelConversions::DstFormat: \
> +                return run<SrcFormat, WebGLTexelConversions::DstFormat>(premultiplicationOp);
> +
> +        switch (dstFormat) {
> +            WEBGLIMAGECONVERTER_CASE(R8)

Could you use another macro to iterate over all formats, so as to avoid these duplicate lists?

@@ +293,5 @@
> +void
> +WebGLContext::ConvertImage(size_t width, size_t height, size_t srcStride, size_t dstStride,
> +                           const PRUint8* src, PRUint8 *dst,
> +                           int srcFormat, bool srcPremultiplied,
> +                           int dstFormat, bool dstPremultiplied,

Can we use an enum type instead of ints for the formats here?

@@ +299,5 @@
> +{
> +    if (width <= 0 || height <= 0)
> +        return;
> +
> +    bool FormatsRequireNoPremultiplicationOp =

For bools like this which should never change, can we const them?

@@ +318,5 @@
> +
> +        NS_ABORT_IF_FALSE(mPixelStoreFlipY || srcStride != dstStride, "Performance trap -- should handle this case earlier, to avoid memcpy");
> +
> +        size_t row_size = width * dstTexelSize; // doesn't matter, src and dst formats agree
> +        const PRUint8* i = src;

Since this is a new file, is there any chance we could switch to uint8_t?

Also, please 'itr' instead of 'i'.

@@ +335,5 @@
> +        }
> +        return;
> +    }
> +
> +    PRUint8 *dstStart = dst;

The block above has star-to-the-left. We should be consistent, and I prefer star-to-the-left.
-to-the-right is ok also, but it should be the same throughout the file.

@@ +344,5 @@
> +    }
> +
> +    WebGLImageConverter converter(width, height, src, dstStart, srcStride, signedDstStride);
> +
> +    int premultiplicationOp

This should be some sort of enum type, not 'int'.

@@ +352,5 @@
> +                                                  : WebGLTexelConversions::NoPremultiplicationOp;
> +
> +    converter.run(srcFormat, dstFormat, premultiplicationOp);
> +
> +    if (!converter.Success()) {

I like this.

::: content/canvas/src/WebGLTexelConversions.h
@@ +115,5 @@
> +         bool IsFloat = IsFloatFormat<Format>::Value,
> +         bool Is16bpp = Is16bppFormat<Format>::Value>
> +struct DataTypeForFormat
> +{
> +    typedef uint8_t Type;

Dangerously clever, but helps with verbosity later, so let's go with it.

@@ +141,5 @@
> +    switch (format) {
> +        case WebGLTexelConversions::R8:
> +        case WebGLTexelConversions::A8:
> +        case WebGLTexelConversions::R32F:
> +        case WebGLTexelConversions::A32F:

If RA32F below is 8, R32F and A32F should be 4.

Also, consider renaming the function to 'TexelBytesForFormat'.

@@ +221,5 @@
>      destination[3] = source[3];
>  }
>  
> +template<> FORCE_INLINE void
> +unpack<RGB8>(const uint8_t* __restrict source, uint8_t* __restrict destination)

Strongly consider a macro for this huge number of near-duplicate declarations.
Also, I would prefer the less-verbose 'src' and 'dest' (possibly 'dst').

@@ +253,2 @@
>  {
>      uint16_t packedValue = source[0];

I think '*source' is more appropriate than 'source[0]', since 'source[1]' is nonsense here, and we're unpacking the whole texel from *source.

@@ +253,3 @@
>  {
>      uint16_t packedValue = source[0];
>      uint8_t r = packedValue >> 11;

I would prefer that we mask this as well, and rely on the compiler to optimize it out.

@@ +266,3 @@
>  {
>      uint16_t packedValue = source[0];
>      uint8_t r = packedValue >> 12;

Please mask this too.

@@ +268,5 @@
>      uint8_t r = packedValue >> 12;
>      uint8_t g = (packedValue >> 8) & 0x0F;
>      uint8_t b = (packedValue >> 4) & 0x0F;
>      uint8_t a = packedValue & 0x0F;
>      destination[0] = r << 4 | r;

'(r << 4) | r' please. I believe the order of operations is fine, but let's be explicit.

@@ +280,3 @@
>  {
>      uint16_t packedValue = source[0];
>      uint8_t r = packedValue >> 11;

Mask please.

@@ +311,2 @@
>  {
>      destination[0] = 0x0;

We don't need '0x0', '0' is fine.

@@ +329,5 @@
>  {
>      destination[0] = source[0];
>      destination[1] = source[1];
>      destination[2] = source[2];
>      destination[3] = 1;

Can we be extra clear that this is 1.0f?

@@ +401,3 @@
>  {
>      float scaleFactor = source[3] / 255.0f;
>      uint8_t sourceR = static_cast<uint8_t>(static_cast<float>(source[0]) * scaleFactor);

Don't manually inline this (un/)premultmath. Pull it out into FORCE_INLINE'd premult/unpremult funcs.
Also, we have the bitwise tricks to premult and unpremult in the range of [0-255]x[0-255], so we should use those instead of bringing floats into byte-land.

@@ +524,5 @@
>      uint8_t sourceB = static_cast<uint8_t>(static_cast<float>(source[2]) * scaleFactor);
>      *destination = (((sourceR & 0xF0) << 8)
>                      | ((sourceG & 0xF0) << 4)
>                      | (sourceB & 0xF0)
>                      | (source[3] >> 4));

Can we be paranoid and mask these too?

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: redo texture conversions

(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> Comment on attachment 619632 [details] [diff] [review]
> Redo texture conversions
> 
> Review of attachment 619632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (Un)Premultiplication is best done with a LUT.
> 
> I can provide the bitwise premult/unpremult from my earlier work on this,
> too.

Cool, sure! But that can be done as a separate patch. Please assess the impact on performance using 
http://hg.mozilla.org/users/bjacob_mozilla.com/webgl-perf-tests/raw-file/tip/webgl-performance-tests.html

> 
> ::: content/canvas/src/WebGLContext.h
> @@ +804,5 @@
> >      nsresult TexParameter_base(WebGLenum target, WebGLenum pname,
> >                                 WebGLint *intParamPtr, WebGLfloat *floatParamPtr);
> >  
> >      void ConvertImage(size_t width, size_t height, size_t srcStride, size_t dstStride,
> > +                      const PRUint8* src, PRUint8 *dst,
> 
> Pick either left-stars or right-stars. (I prefer left-stars)

OK, let's do left-stars. It's also what mfbt/STYLE recommends.

> 
> ::: content/canvas/src/WebGLTexelConversions.cpp
> @@ +20,5 @@
> > + */
> > +class WebGLImageConverter
> > +{
> > +    const size_t mWidth, mHeight;
> > +    const void * const mSrcStart;
> 
> Can we do 'void*' instead of leaving the star hanging in the middle, there?

Yep.

> 
> @@ +224,5 @@
> > +            case WebGLTexelConversions::DstFormat: \
> > +                return run<SrcFormat, WebGLTexelConversions::DstFormat>(premultiplicationOp);
> > +
> > +        switch (dstFormat) {
> > +            WEBGLIMAGECONVERTER_CASE(R8)
> 
> Could you use another macro to iterate over all formats, so as to avoid
> these duplicate lists?

Yep.

> 
> @@ +293,5 @@
> > +void
> > +WebGLContext::ConvertImage(size_t width, size_t height, size_t srcStride, size_t dstStride,
> > +                           const PRUint8* src, PRUint8 *dst,
> > +                           int srcFormat, bool srcPremultiplied,
> > +                           int dstFormat, bool dstPremultiplied,
> 
> Can we use an enum type instead of ints for the formats here?

Very good point. Just one thing: that forced me to move back to WebGLContext.h the WebGLTexelFormat enum. (It is not possible to forward-declare an enum type in c++98 as the compiler can't know its storage type. Becomes possible in c++11 with explicitly typed enums).

> 
> @@ +299,5 @@
> > +{
> > +    if (width <= 0 || height <= 0)
> > +        return;
> > +
> > +    bool FormatsRequireNoPremultiplicationOp =
> 
> For bools like this which should never change, can we const them?

OK.

> 
> @@ +318,5 @@
> > +
> > +        NS_ABORT_IF_FALSE(mPixelStoreFlipY || srcStride != dstStride, "Performance trap -- should handle this case earlier, to avoid memcpy");
> > +
> > +        size_t row_size = width * dstTexelSize; // doesn't matter, src and dst formats agree
> > +        const PRUint8* i = src;
> 
> Since this is a new file, is there any chance we could switch to uint8_t?

OK, let's do that. We want to do a wholesale switch to stdint soon anyways, but let's start here.


> 
> Also, please 'itr' instead of 'i'.

Used 'ptr'.

> 
> @@ +335,5 @@
> > +        }
> > +        return;
> > +    }
> > +
> > +    PRUint8 *dstStart = dst;
> 
> The block above has star-to-the-left. We should be consistent, and I prefer
> star-to-the-left.
> -to-the-right is ok also, but it should be the same throughout the file.

OK.

> 
> @@ +344,5 @@
> > +    }
> > +
> > +    WebGLImageConverter converter(width, height, src, dstStart, srcStride, signedDstStride);
> > +
> > +    int premultiplicationOp
> 
> This should be some sort of enum type, not 'int'.

OK.

> 
> @@ +352,5 @@
> > +                                                  : WebGLTexelConversions::NoPremultiplicationOp;
> > +
> > +    converter.run(srcFormat, dstFormat, premultiplicationOp);
> > +
> > +    if (!converter.Success()) {
> 
> I like this.
> 
> ::: content/canvas/src/WebGLTexelConversions.h
> @@ +115,5 @@
> > +         bool IsFloat = IsFloatFormat<Format>::Value,
> > +         bool Is16bpp = Is16bppFormat<Format>::Value>
> > +struct DataTypeForFormat
> > +{
> > +    typedef uint8_t Type;
> 
> Dangerously clever, but helps with verbosity later, so let's go with it.

It's actually a very common metaprogramming idiom. If you don't like it, I have a stale patch to add an equivalent of std::conditional (aka meta_if) to mfbt in bug 715155.

> 
> @@ +141,5 @@
> > +    switch (format) {
> > +        case WebGLTexelConversions::R8:
> > +        case WebGLTexelConversions::A8:
> > +        case WebGLTexelConversions::R32F:
> > +        case WebGLTexelConversions::A32F:
> 
> If RA32F below is 8, R32F and A32F should be 4.

Wow, good catch!

> 
> Also, consider renaming the function to 'TexelBytesForFormat'.

OK.

> 
> @@ +221,5 @@
> >      destination[3] = source[3];
> >  }
> >  
> > +template<> FORCE_INLINE void
> > +unpack<RGB8>(const uint8_t* __restrict source, uint8_t* __restrict destination)
> 
> Strongly consider a macro for this huge number of near-duplicate
> declarations.

Not trivial as there are slight variations in each. I would be interested in a patch to do that, but please treat as orthogonal to this patch. Also, this code originally comes from WebKit and for a while we avoided touching it so as to ease syncing. Now it doesn't seem to matter anymore as AFAIK there have not been changes on WebKit side.

> Also, I would prefer the less-verbose 'src' and 'dest' (possibly 'dst').

OK.

> 
> @@ +253,2 @@
> >  {
> >      uint16_t packedValue = source[0];
> 
> I think '*source' is more appropriate than 'source[0]', since 'source[1]' is
> nonsense here, and we're unpacking the whole texel from *source.

I beg to differ: in a context that has lots of array indexing like here, src[0] is easier for me to parse. I would like to keep it this way. Also, that's already the way it was so I'm not changing anything here.

> 
> @@ +253,3 @@
> >  {
> >      uint16_t packedValue = source[0];
> >      uint8_t r = packedValue >> 11;
> 
> I would prefer that we mask this as well, and rely on the compiler to
> optimize it out.

The code was already like that. Why do you prefer it masked? By masked, do you mean

   (packedValue & 0xf800) >> 11

? I would have to check if compilers actually optimize this out, and I'm not sure I understand the potential benefit. In fact, this introduces a new place where it's possible to get things wrong (have to set the correct mask value).

> 
> @@ +266,3 @@
> >  {
> >      uint16_t packedValue = source[0];
> >      uint8_t r = packedValue >> 12;
> 
> Please mask this too.
> 
> @@ +268,5 @@
> >      uint8_t r = packedValue >> 12;
> >      uint8_t g = (packedValue >> 8) & 0x0F;
> >      uint8_t b = (packedValue >> 4) & 0x0F;
> >      uint8_t a = packedValue & 0x0F;
> >      destination[0] = r << 4 | r;
> 
> '(r << 4) | r' please. I believe the order of operations is fine, but let's
> be explicit.

OK.


> 
> @@ +280,3 @@
> >  {
> >      uint16_t packedValue = source[0];
> >      uint8_t r = packedValue >> 11;
> 
> Mask please.

Same as above.

> 
> @@ +311,2 @@
> >  {
> >      destination[0] = 0x0;
> 
> We don't need '0x0', '0' is fine.

OK

> 
> @@ +329,5 @@
> >  {
> >      destination[0] = source[0];
> >      destination[1] = source[1];
> >      destination[2] = source[2];
> >      destination[3] = 1;
> 
> Can we be extra clear that this is 1.0f?
> 
> @@ +401,3 @@
> >  {
> >      float scaleFactor = source[3] / 255.0f;
> >      uint8_t sourceR = static_cast<uint8_t>(static_cast<float>(source[0]) * scaleFactor);
> 
> Don't manually inline this (un/)premultmath. Pull it out into FORCE_INLINE'd
> premult/unpremult funcs.

I realized that the static_cast<float> here were useless. Removing them, the rest didn't seem big enough anymore to warrant an inline function: just a mul and a cast to uint8.

> Also, we have the bitwise tricks to premult and unpremult in the range of
> [0-255]x[0-255], so we should use those instead of bringing floats into
> byte-land.

OK, see beginning of this comment.

> 
> @@ +524,5 @@
> >      uint8_t sourceB = static_cast<uint8_t>(static_cast<float>(source[2]) * scaleFactor);
> >      *destination = (((sourceR & 0xF0) << 8)
> >                      | ((sourceG & 0xF0) << 4)
> >                      | (sourceB & 0xF0)
> >                      | (source[3] >> 4));
> 
> Can we be paranoid and mask these too?

See above comment about masking.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 620932 [details] [diff] [review]
redo texture conversions

By the way, I hate Bugzilla's enter-submits-form-immediately behavior.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Redo texture conversions

Fixes some build errors relating to the int -> WebGLTexelFormat enum type switch.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 620939 [details] [diff] [review]
Redo texture conversions

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

Consider the nits, but nothing seems wrong.

::: content/canvas/src/WebGLTexelConversions.cpp
@@ +164,5 @@
> +        NS_ABORT_IF_FALSE(NumElementsPerDstTexel <= MaxElementsPerTexel, "unhandled format");
> +
> +        // the loop performing the texture format conversion
> +
> +        const ptrdiff_t srcStrideInElements = mSrcStride / sizeof(SrcType);

This only works for sizes that are int multiples of the alignment, or int POT divisions of the alignment.
Since our sizes are 1, 2, and 4, we're fine for now. However, sizes of 3 or 6 will not divide an 4-aligned-padded stride.
This might be worth a debug assert.

@@ +269,5 @@
> +            WEBGLIMAGECONVERTER_CASE_SRCFORMAT(A32F)
> +            WEBGLIMAGECONVERTER_CASE_SRCFORMAT(RA8)
> +            WEBGLIMAGECONVERTER_CASE_SRCFORMAT(RA32F)
> +            WEBGLIMAGECONVERTER_CASE_SRCFORMAT(RGB8)
> +            WEBGLIMAGECONVERTER_CASE_SRCFORMAT(BGRX8)

Can we put BGRX8 at the end, leaving these source and dest format lists the same? At first glance, it looks like these are the same, but actually BGRX8 is only a source format.

@@ +330,5 @@
> +        NS_ABORT_IF_FALSE(mPixelStoreFlipY || srcStride != dstStride, "Performance trap -- should handle this case earlier, to avoid memcpy");
> +
> +        size_t row_size = width * dstTexelSize; // doesn't matter, src and dst formats agree
> +        const uint8_t* ptr = src;
> +        const uint8_t* src_end = src + height * srcStride;

You could do 'const uint8_t* const src_end', since we shouldn't be changing the pointer.

@@ +336,5 @@
> +        uint8_t* dst_row = mPixelStoreFlipY
> +                           ? dst + (height-1) * dstStride
> +                           : dst;
> +        ptrdiff_t dstStrideSigned(dstStride);
> +        ptrdiff_t dst_delta = mPixelStoreFlipY ? -dstStrideSigned : dstStrideSigned;

If we're going const-wild here, this could stand to be const.
Not that this should be hard for the compiler to determine on its own, but it's up to you.

@@ +371,5 @@
> +        NS_RUNTIMEABORT("programming mistake in WebGL texture conversions");
> +    }
> +}
> +
> +}

Mark that this is ending 'namespace mozilla'.

::: content/canvas/src/WebGLTexelConversions.h
@@ +32,5 @@
>  #define __restrict
>  #endif
>  
>  #include "WebGLContext.h"
> +#include "mozilla/StandardInteger.h"

\o/

@@ +218,3 @@
>      uint8_t r = packedValue >> 11;
>      uint8_t g = (packedValue >> 6) & 0x1F;
>      uint8_t b = (packedValue >> 1) & 0x1F;

I meant I would prefer:

uint8_t r = (packedValue >> 11) & 0x1F;

so as to match 'g' and 'b'.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> Comment on attachment 620939 [details] [diff] [review]
> Redo texture conversions
> 
> Review of attachment 620939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Consider the nits, but nothing seems wrong.
> 
> ::: content/canvas/src/WebGLTexelConversions.cpp
> @@ +164,5 @@
> > +        NS_ABORT_IF_FALSE(NumElementsPerDstTexel <= MaxElementsPerTexel, "unhandled format");
> > +
> > +        // the loop performing the texture format conversion
> > +
> > +        const ptrdiff_t srcStrideInElements = mSrcStride / sizeof(SrcType);
> 
> This only works for sizes that are int multiples of the alignment, or int
> POT divisions of the alignment.
> Since our sizes are 1, 2, and 4, we're fine for now. However, sizes of 3 or
> 6 will not divide an 4-aligned-padded stride.
> This might be worth a debug assert.

Assertion added.

> 
> @@ +269,5 @@
> > +            WEBGLIMAGECONVERTER_CASE_SRCFORMAT(A32F)
> > +            WEBGLIMAGECONVERTER_CASE_SRCFORMAT(RA8)
> > +            WEBGLIMAGECONVERTER_CASE_SRCFORMAT(RA32F)
> > +            WEBGLIMAGECONVERTER_CASE_SRCFORMAT(RGB8)
> > +            WEBGLIMAGECONVERTER_CASE_SRCFORMAT(BGRX8)
> 
> Can we put BGRX8 at the end, leaving these source and dest format lists the
> same? At first glance, it looks like these are the same, but actually BGRX8
> is only a source format.

Comment added. Perhaps frivolously, I wanted to keep these cases in the order of declarations of the enum values, to make them easier to find and in a naive attempt to help the compiler do a table-switch.

> 
> @@ +330,5 @@
> > +        NS_ABORT_IF_FALSE(mPixelStoreFlipY || srcStride != dstStride, "Performance trap -- should handle this case earlier, to avoid memcpy");
> > +
> > +        size_t row_size = width * dstTexelSize; // doesn't matter, src and dst formats agree
> > +        const uint8_t* ptr = src;
> > +        const uint8_t* src_end = src + height * srcStride;
> 
> You could do 'const uint8_t* const src_end', since we shouldn't be changing
> the pointer.

My rule of thumb for where to use const in local variables is basically: use pointer-to-const for pointers that are not supposed to be used to modify the pointee; maybe use const in some other exceptional cases where there is a special risk of mistakenly mutating a variable; but aside from that, I prefer not to constipate all the local variables as the cluttering of the code makes it not worth it.

> 
> @@ +336,5 @@
> > +        uint8_t* dst_row = mPixelStoreFlipY
> > +                           ? dst + (height-1) * dstStride
> > +                           : dst;
> > +        ptrdiff_t dstStrideSigned(dstStride);
> > +        ptrdiff_t dst_delta = mPixelStoreFlipY ? -dstStrideSigned : dstStrideSigned;
> 
> If we're going const-wild here, this could stand to be const.
> Not that this should be hard for the compiler to determine on its own, but
> it's up to you.

Same.

> 
> @@ +371,5 @@
> > +        NS_RUNTIMEABORT("programming mistake in WebGL texture conversions");
> > +    }
> > +}
> > +
> > +}
> 
> Mark that this is ending 'namespace mozilla'.

Done.

> 
> ::: content/canvas/src/WebGLTexelConversions.h
> @@ +32,5 @@
> >  #define __restrict
> >  #endif
> >  
> >  #include "WebGLContext.h"
> > +#include "mozilla/StandardInteger.h"
> 
> \o/
> 
> @@ +218,3 @@
> >      uint8_t r = packedValue >> 11;
> >      uint8_t g = (packedValue >> 6) & 0x1F;
> >      uint8_t b = (packedValue >> 1) & 0x1F;
> 
> I meant I would prefer:
> 
> uint8_t r = (packedValue >> 11) & 0x1F;
> 
> so as to match 'g' and 'b'.

Done. FWIW, GCC 4.6 linux x86-64 produces better code with masking,

	shrw	$11, (%rdi)

than without masking,

	movzwl	(%rdi), %eax
	sarl	$11, %eax
	movw	%ax, (%rdi)

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e8f3fd02ba3e

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Backed out for windows compiler error:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ccfb4a0aeb5e

e:\builds\moz2_slave\m-in-w32\build\content\canvas\src\WebGLTexelConversions.h(225) : error C2912: explicit specialization; 'void mozilla::WebGLTexelConversions::unpack<RGBA5551>(const uint16_t *__restrict ,uint8_t *__restrict )' is not a specialization of a function template

This is a MSVC bug; going to try some work-arounds.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=60cb376039dc

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Original landing and backout:
http://hg.mozilla.org/mozilla-central/rev/e8f3fd02ba3e
http://hg.mozilla.org/mozilla-central/rev/ccfb4a0aeb5e

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9f87dbd4d39c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f87dbd4d39c

Comment 19

[Kenneth Russell]

Note: tests verifying the upload path from DOM element and ImageData to floating-point texture have been added to the top of tree WebGL conformance suite. See sdk/tests/conformance/extensions/oes-texture-float-with-*.html. However, I suspect that the upload paths to the packed pixel formats still need to be tested.

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

Many thanks! Keeping webgl-test-needed until the latter point is addressed.

Comment 21

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Kenneth Russell from comment #19)
> Note: tests verifying the upload path from DOM element and ImageData to
> floating-point texture have been added to the top of tree WebGL conformance
> suite. See sdk/tests/conformance/extensions/oes-texture-float-with-*.html.
> However, I suspect that the upload paths to the packed pixel formats still
> need to be tested.

I took a look but didn't see anything obvious to add. The upload paths to the 16 bpp formats are well covered already under conformance/textures. I just renamed this bug from "changing texel size" to "increasing texel size" as that is really where our bug was. Decreasing texel size from 24/32bpp to 16bpp is already well covered by tests. As for increasing texel size from 16bpp to 24/32bpp, in a browser that produces 16bpp bitmaps for DOM elements, the current tests already cover that.

Comment 22

[Kenneth Russell]

Since writing the above comments, I added WebGL conformance tests for uploading image, image data, canvas, and video to the packed pixel formats. Sorry for not updating this bug. I think there might still be an untested path uploading to the RGB8 rather than RGBA8 format.