966624 - WebGL crash [@mozilla::GetWebGLTexelFormat]

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/1e7b2bb3041b

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4a801fc05819
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140128050622
Crash:
http://hg.mozilla.org/integration/mozilla-inbound/rev/de0e3bff81d5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140128060132
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4a801fc05819&tochange=de0e3bff81d5

Comment 3

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

Dan, comment 2's regression window makes this a likely regression from Bug 948002.

Comment 4

[u480271]

Attachment: Hoist valid texture checks in TexSubImage2D_base.

Checking that the texture image is valid occurs quite late in the function. 
Hoisted the checks to before the call to GetWebGLTexelFormat to eliminate many
bad combinations earlier.

Comment 5

[u480271]

(In reply to Benoit Jacob [:bjacob] from comment #3)
> Dan, comment 2's regression window makes this a likely regression from Bug
> 948002.

I believe this is because Jeff had me change the asserts into a crash on unknown combination of format and type.

I've looked at this code and it appears strange that an error occurs where it did. The testcase is invalid WebGL sequence because gl.TexSubImage2D() is called with no corresponding gl.TexImage2D().  The texture is in an invalid state and the TexSubImage should be in error.

I've uploaded a patch, where I moved the valid texture image checks before to before the function that asserts. The rationale is that this adds more checks because TexImage2D checks the combination of format & type for validity, which can then be checked against what is passed to TexSubImage2D, before we get to the point to decided to call GetWebGLTexelFormat.

Comment 6

[u480271]

We should add the fuzzing test case to our suite of conformance tests. I'm not certain what the best place for such a test is.

Comment 7

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

The basic choice he is: do you want this integrated into the official Khronos test suite, or not? The latter is the default choice for a crash testcase, but if you 1) are motivated enough to do that process and 2) think that this testcase could catch other bugs in any WebGL implementation i.e. it's general enough, then you might consider the former.

If you want to do the former, I think that you've already done that a few times, but let me know if you have any questions.

If you want to do the latter (which, again, I'd consider the default choice here), then you want to make this a "crashtest". Crashtests, unlike mochitests, are merely test pages that the browser loads, and the only check is that the browser didn't crash.

To go the crashtest route, I would create a content/canvas/test/crashtest subdirectory, and follow any existing crashtest directory as example (e.g. content/xml/content/crashtest).

Comment 8

[u480271]

Attachment: Refactor test to get better error reporting.

This test failed during my refactoring and the output was incomplete. I
refactored it to work like the more modern tests which made it clear where it
was failing.

Comment 9

[u480271]

Attachment: Refactor Tex Image checks.

Refactored and validated parameter checking for the family of texture image
specifaction functions: TexImage2D, TexSubImage2D, CopyTexImage2D,
CopyTexSubImage2D, CompressedTexImage2D and CompressedTexSubImage2D.

Extracted and merged common checking functionality into
WebGLContextValidate.cpp.

My intention which the clean up was to:
1) Be more consistent with checking for invalid parameters. Fuzzing test that
   caused crashes had a common cause of incomplete parameter checking allowing
   an invalid value to get to code that would assert, or worse, crash the
   driver.
2) Make it easier for people implementing extensions that add texture format
   and types to know where to add required functionality.
3) Show intention of where WebGL 2 extension points should be added. (ie.
   TexImage3D)

Comment 10

[u480271]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=8d1ed9f63890

Comment 11

[u480271]

Hmmm. Guess that didn't work.

Comment 12

[u480271]

Attachment: Refactor Tex Image checks.

Fix compilation errors on other platforms.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8376070 [details] [diff] [review]
Refactor Tex Image checks.

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

I like this patch on principle, but I'll have to review it tomorrow.

::: content/canvas/src/WebGLContext.cpp
@@ +1211,5 @@
>  WebGLContext::DummyFramebufferOperation(const char *info)
>  {
>      GLenum status = CheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER);
> +    if (status != LOCAL_GL_FRAMEBUFFER_COMPLETE)
> +        ErrorInvalidFramebufferOperation("%s: incomplete framebuffer", info);

Thanks, this was actually bugging me.

::: content/canvas/src/WebGLTypes.h
@@ +134,5 @@
>      RGBA32F // OES_texture_float
>  MOZ_END_ENUM_CLASS(WebGLTexelFormat)
>  
> +enum WebGLTexImageSource {
> +    TexImage        = 0x2,

Why are we giving these values, much less such odd and explicitly-hexadecimal ones?

Comment 14

[u480271]

Mostly green: https://tbpl.mozilla.org/?tree=Try&rev=c5fe579ca5de

Comment 15

[u480271]

Attachment: Remove tabs. Replace with spaces.

This should be a simple one.

Comment 16

[u480271]

Attachment: Refactor Tex Image checks.

Fix up silly missing parameters to ErrorInvalidXXX().  Mea culpa.

Comment 17

[u480271]

Attachment: imported patch bug--refactor-texImage2DBadargs.patch

Refreshing patches.

Comment 18

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #13)
> Comment on attachment 8376070 [details] [diff] [review]
> Refactor Tex Image checks.
> 
> Review of attachment 8376070 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like this patch on principle, but I'll have to review it tomorrow.
> 
> ::: content/canvas/src/WebGLTypes.h
> @@ +134,5 @@
> >      RGBA32F // OES_texture_float
> >  MOZ_END_ENUM_CLASS(WebGLTexelFormat)
> >  
> > +enum WebGLTexImageSource {
> > +    TexImage        = 0x2,
> 
> Why are we giving these values, much less such odd and
> explicitly-hexadecimal ones?

Because I use the bits in the enum to signal the class of source function.
    0b0001 - Sub variant
    0b0010 - TexImage
    0b0100 - CopyTexImage
    0b1000 - CompressedTexImage

It's current used by IsSub() helper method in WebGLContextValidate.cpp. There were also other helper functions which I had to determining if special logic was required for TexImage v CopyTexImage v CompressedTexImage.

Comment 19

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #18)
> (In reply to Jeff Gilbert [:jgilbert] from comment #13)
> > Comment on attachment 8376070 [details] [diff] [review]
> > Refactor Tex Image checks.
> > 
> > Review of attachment 8376070 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I like this patch on principle, but I'll have to review it tomorrow.
> > 
> > ::: content/canvas/src/WebGLTypes.h
> > @@ +134,5 @@
> > >      RGBA32F // OES_texture_float
> > >  MOZ_END_ENUM_CLASS(WebGLTexelFormat)
> > >  
> > > +enum WebGLTexImageSource {
> > > +    TexImage        = 0x2,
> > 
> > Why are we giving these values, much less such odd and
> > explicitly-hexadecimal ones?
> 
> Because I use the bits in the enum to signal the class of source function.
>     0b0001 - Sub variant
>     0b0010 - TexImage
>     0b0100 - CopyTexImage
>     0b1000 - CompressedTexImage
> 
> It's current used by IsSub() helper method in WebGLContextValidate.cpp.
> There were also other helper functions which I had to determining if special
> logic was required for TexImage v CopyTexImage v CompressedTexImage.

I would much rather this be a small struct, rather than a more-complicated bitfield.

Comment 20

[Kelsey Gilbert [:jgilbert]]

Specifically:
enum WebGLTexImageBaseFunc {
  TexImage,
  CopyTexImage,
  CompressedTexImage,
}

struct WebGLTexImageFunc {
  WebGLTexImageBaseFunc func;
  bool sub;
};

Comment 21

[Kelsey Gilbert [:jgilbert]]

I think more specifically, I don't like trying to optimize to the extent of having a 'sub' bit.

Comment 22

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #21)
> I think more specifically, I don't like trying to optimize to the extent of
> having a 'sub' bit.

Honestly, I don't see it as an optimisation, other than it saves typing. Less code; easier to comprehend.

Comment 23

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #22)
> (In reply to Jeff Gilbert [:jgilbert] from comment #21)
> > I think more specifically, I don't like trying to optimize to the extent of
> > having a 'sub' bit.
> 
> Honestly, I don't see it as an optimisation, other than it saves typing.
> Less code; easier to comprehend.

It's non-obvious behavior, to me. More obvious is better. Readability is more important than shortness.
Encoding whether or not something is a 'sub' function in the lowest bit is an odd thing to do, unless it's an optimization attempt.

Unless we think it'll be a perf problem, I'd rather have something like:
bool IsSubFunc(funcType) {
  switch (funcType) {
  case TexSubImage:
  case CopyTexSubImage:
  case CompressedTexSubImage:
    return true;
  }
  return false;
}

It's really boring and obvious this way.

Comment 24

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8377304 [details] [diff] [review]
Refactor Tex Image checks.

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +394,5 @@
> +
> +    if (!ValidateTexImage(2, target, level, internalformat,
> +                          xoffset, yoffset, 0,
> +                          width, height, 0,
> +                          0, internalformat, LOCAL_GL_UNSIGNED_BYTE,

I think this changes with color_buffer_float, but that patch is still landing.

@@ +3364,3 @@
>      MakeContextCurrent();
>      gl->fCompressedTexImage2D(target, level, internalformat, width, height, border, byteLength, view.Data());
> +    WebGLTexture* tex = activeBoundTextureForTarget(target);

Assert non-null.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +46,5 @@
> +    case LOCAL_GL_ATC_RGBA_INTERPOLATED_ALPHA:
> +    case LOCAL_GL_COMPRESSED_RGB_S3TC_DXT1_EXT:
> +    case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT:
> +    case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT:
> +    case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT:

No PVRTC?

@@ +109,5 @@
> +        XX(DEPTH_COMPONENT32);
> +        XX(DEPTH_STENCIL);
> +        XX(DEPTH24_STENCIL8);
> +        XX(FLOAT);
> +        XX(HALF_FLOAT);

You'll also want HALF_FLOAT_OES.

@@ +115,5 @@
> +        XX(LUMINANCE_ALPHA);
> +        XX(RGB);
> +        XX(RGB32F);
> +        XX(RGBA);
> +        XX(RGBA32F);

Don't forget RGB(A)16F.

@@ +151,5 @@
> +    const char* name = NameFrom(glenum);
> +    if (name)
> +        ctx->ErrorInvalidEnum("%s: %s %s", InfoFrom(source), msg, name);
> +    else
> +        ctx->ErrorInvalidEnum("%s: %s 0x%04X", InfoFrom(source), msg, glenum);

Awesome.

@@ +165,5 @@
> +    case LOCAL_GL_COMPRESSED_RGB_S3TC_DXT1_EXT:
> +    case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT:
> +    case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT:
> +    case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT:
> +        return source == TexImage || source == CompTexImage || source == CompTexSubImage;

Why does this include TexImage?

@@ +176,5 @@
> +
> +    case LOCAL_GL_ATC_RGB:
> +    case LOCAL_GL_ATC_RGBA_EXPLICIT_ALPHA:
> +    case LOCAL_GL_ATC_RGBA_INTERPOLATED_ALPHA:
> +        return source == CompTexImage;

Is SubImage really not supported for this?

@@ +188,5 @@
> + */
> +static bool
> +IsSub(WebGLTexImageSource source)
> +{
> +    return ((uint32_t) source & 0x1) != 0;

Yeah, it's this which I think's unnecessarily tricky. Why not just a switch over the 'Sub' types?

@@ +275,5 @@
> + *
> + * \return the corresponding \u base internal format (GL_ALPHA, GL_LUMINANCE,
> + * GL_LUMINANCE_ALPHA, GL_RGB, GL_RGBA), or -1 if invalid enum.
> + */
> +GLint

GLenum

@@ +276,5 @@
> + * \return the corresponding \u base internal format (GL_ALPHA, GL_LUMINANCE,
> + * GL_LUMINANCE_ALPHA, GL_RGB, GL_RGBA), or -1 if invalid enum.
> + */
> +GLint
> +WebGLContext::BaseTexFormat(GLint internalFormat) const

GLenum

@@ +347,5 @@
> +            return LOCAL_GL_DEPTH_STENCIL;
> +        }
> +    }
> +
> +    return -1;

Assert here for unhandled types. Also, returning 0 is probably better, since it's still valid, and won't mess with any signed stuff.

@@ +694,5 @@
> +    }
> +
> +    /* OES_texture_float added type */
> +    if (type == LOCAL_GL_FLOAT ||
> +        type == LOCAL_GL_HALF_FLOAT)

HALF_FLOAT_OES

@@ +872,5 @@
> +                                   GLint width, GLint height, GLint depth,
> +                                   WebGLTexImageSource source)
> +{
> +    const GLint maxTextureLevel = MaxTextureLevelForTarget(target);
> +    const GLint maxTextureSize = 1 << (maxTextureLevel - level);

These are both 'TexImage', not 'Texture' values.

We should also do this the other way around. Based on the MAX_TEXTURE_SIZE, calculate the maxTexImageSize for a given level.

size_t MaxTexImageSize(target, level) {
  size_t max = GetMaxByTarget(target);
  max >>= level;
  return max;
}

@@ +876,5 @@
> +    const GLint maxTextureSize = 1 << (maxTextureLevel - level);
> +
> +    if (((target >= LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_X &&
> +          target <= LOCAL_GL_TEXTURE_CUBE_MAP_NEGATIVE_Z) ||
> +         target == LOCAL_GL_TEXTURE_CUBE_MAP) &&

Why are we accepting TEXTURE_CUBE_MAP here? It's not a valid TexImage target.

@@ +891,5 @@
> +    }
> +
> +    if (target == LOCAL_GL_TEXTURE_2D ||
> +        target == LOCAL_GL_TEXTURE_3D ||
> +        target == LOCAL_GL_TEXTURE_CUBE_MAP ||

Not a TexImage target.

@@ +1020,5 @@
>  
> +    /* Known fixed-sized types */
> +    if (type == LOCAL_GL_UNSIGNED_SHORT_4_4_4_4 ||
> +        type == LOCAL_GL_UNSIGNED_SHORT_5_5_5_1 ||
> +        type == LOCAL_GL_UNSIGNED_SHORT_5_6_5)

Can we do a switch statement for these? Really, I think I liked the previous format better. (Big switch statement, only some of which applied the multiplier.

@@ +1028,5 @@
>  
> +    if (type == LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT ||
> +        type == LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT ||
> +        type == LOCAL_GL_ATC_RGBA_EXPLICIT_ALPHA ||
> +        type == LOCAL_GL_ATC_RGBA_INTERPOLATED_ALPHA)

`format` not `type`

@@ +1037,5 @@
> +    if (type == LOCAL_GL_COMPRESSED_RGB_S3TC_DXT1_EXT ||
> +        type == LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT ||
> +        type == LOCAL_GL_ATC_RGB ||
> +        type == LOCAL_GL_COMPRESSED_RGB_PVRTC_4BPPV1 ||
> +        type == LOCAL_GL_COMPRESSED_RGBA_PVRTC_4BPPV1)

`format` not `type`

@@ +1043,5 @@
> +        return 4;
> +    }
> +
> +    if (type == LOCAL_GL_COMPRESSED_RGB_PVRTC_2BPPV1 ||
> +        type == LOCAL_GL_COMPRESSED_RGBA_PVRTC_2BPPV1)

`format` not `type`

@@ +1106,2 @@
>  
> +    bool validCombo = true;

Default to `false`.

@@ +1163,5 @@
> + * ValidateTexImage()
> + */
> +bool
> +WebGLContext::ValidateTexFormatAndType(GLenum format, GLenum type, int jsArrayType,
> +                                       uint32_t* texelSize, WebGLTexImageSource source)

We should try to reduce the overlap between this func and ValidateTexImageFormatAndType.
I think we change this func to ValidateTexInputData, and have it only validate jsArrayType.

@@ +1167,5 @@
> +                                       uint32_t* texelSize, WebGLTexImageSource source)
> +{
> +    // TODO: This function appears to have two responsiblities.
> +    //       1. It checks for a valid combo of format, type and jsArrayType.
> +    //       2. It also returns the texel size (but that's handled in an GetBitsPerTexel too)

Let's drop the 2nd responsibility, if we can.

@@ +1171,5 @@
> +    //       2. It also returns the texel size (but that's handled in an GetBitsPerTexel too)
> +    if (format == LOCAL_GL_DEPTH_COMPONENT) {
> +        if (jsArrayType != -1 &&
> +            ((type == LOCAL_GL_UNSIGNED_SHORT && jsArrayType != js::ArrayBufferView::TYPE_UINT16) ||
> +             (type == LOCAL_GL_UNSIGNED_INT && jsArrayType != js::ArrayBufferView::TYPE_UINT32))) {

Consider dropping the brace to the next line.

@@ +1199,5 @@
> +                                  InfoFrom(source));
> +            return false;
> +        }
> +
> +        *texelSize = 4;

Needs to check that type is UNSIGNED_INT_24_8.

@@ +1356,5 @@
> +    }
> +
> +    /* check internalFormat */
> +    // TODO: Not sure if this is a bit of over kill.
> +    if (BaseTexFormat(internalFormat) < 0) {

Probably, but we should assert in debug builds, and return somehow in release builds.

@@ +1380,5 @@
> +
> +    if (IsSub(source)) {
> +        if (!tex->HasImageInfoAt(target, level)) {
> +            ErrorInvalidOperation("%s: no texture image previously defined for target %s at level %d",
> +                                  info, NameFrom(target), level);

`return false`

@@ +1416,5 @@
> +    }
> +
> +    /* Additional checks for compressed textures */
> +    if (!IsAllowedFromSource(format, source)) {
> +        ErrorInvalidOperation("%s: Invalid format %s",

Maybe "Invalid format %s for this operation."?

@@ +1696,5 @@
>  
>      mGLMaxTextureSize = floorPOT(mGLMaxTextureSize);
>      mGLMaxRenderbufferSize = floorPOT(mGLMaxRenderbufferSize);
>  
> +    mGLMaxTextureLevel = mozilla::FloorLog2(mGLMaxTextureSize);

This still makes me sad, and I think we shouldn't do it. I'll bring it up with the WG again.

Comment 25

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #24)
> Comment on attachment 8377304 [details] [diff] [review]
> Refactor Tex Image checks.
> 
> Review of attachment 8377304 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +394,5 @@
> > +
> > +    if (!ValidateTexImage(2, target, level, internalformat,
> > +                          xoffset, yoffset, 0,
> > +                          width, height, 0,
> > +                          0, internalformat, LOCAL_GL_UNSIGNED_BYTE,
> 
> I think this changes with color_buffer_float, but that patch is still
> landing.

OK. Will add a note to the code.

> ::: content/canvas/src/WebGLContextValidate.cpp
> @@ +46,5 @@
> > +    case LOCAL_GL_ATC_RGBA_INTERPOLATED_ALPHA:
> > +    case LOCAL_GL_COMPRESSED_RGB_S3TC_DXT1_EXT:
> > +    case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT:
> > +    case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT:
> > +    case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT:
> 
> No PVRTC?

From my reading of the specs, PVRTC doesn't have a block size. PVRTC is two textures that are blended together.
 
> @@ +165,5 @@
> > +    case LOCAL_GL_COMPRESSED_RGB_S3TC_DXT1_EXT:
> > +    case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT1_EXT:
> > +    case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT3_EXT:
> > +    case LOCAL_GL_COMPRESSED_RGBA_S3TC_DXT5_EXT:
> > +        return source == TexImage || source == CompTexImage || source == CompTexSubImage;
> 
> Why does this include TexImage?

Because I read the spec that allows it and missed the WebGL version that disallows it.

> 
> @@ +176,5 @@
> > +
> > +    case LOCAL_GL_ATC_RGB:
> > +    case LOCAL_GL_ATC_RGBA_EXPLICIT_ALPHA:
> > +    case LOCAL_GL_ATC_RGBA_INTERPOLATED_ALPHA:
> > +        return source == CompTexImage;
> 
> Is SubImage really not supported for this?

Not according to my reading of the spec. Also the same for ETC1. Unless I'm missing some important wording, somewhere.

> @@ +275,5 @@
> > + *
> > + * \return the corresponding \u base internal format (GL_ALPHA, GL_LUMINANCE,
> > + * GL_LUMINANCE_ALPHA, GL_RGB, GL_RGBA), or -1 if invalid enum.
> > + */
> > +GLint
> 
> GLenum

Isn't GLenum unsigned?
 
> @@ +694,5 @@
> > +    }
> > +
> > +    /* OES_texture_float added type */
> > +    if (type == LOCAL_GL_FLOAT ||
> > +        type == LOCAL_GL_HALF_FLOAT)
> 
> HALF_FLOAT_OES

I hate that HALF_FLOAT and HALF_FLOAT_OES are different values.

> @@ +1163,5 @@
> > + * ValidateTexImage()
> > + */
> > +bool
> > +WebGLContext::ValidateTexFormatAndType(GLenum format, GLenum type, int jsArrayType,
> > +                                       uint32_t* texelSize, WebGLTexImageSource source)
> 
> We should try to reduce the overlap between this func and
> ValidateTexImageFormatAndType.
> I think we change this func to ValidateTexInputData, and have it only
> validate jsArrayType.

That was in the back of my mind.

> @@ +1696,5 @@
> >  
> >      mGLMaxTextureSize = floorPOT(mGLMaxTextureSize);
> >      mGLMaxRenderbufferSize = floorPOT(mGLMaxRenderbufferSize);
> >  
> > +    mGLMaxTextureLevel = mozilla::FloorLog2(mGLMaxTextureSize);
> 
> This still makes me sad, and I think we shouldn't do it. I'll bring it up
> with the WG again.

What does? The spec talks in term of mip-map levels. I added this check because we had a crash caused by a level that was well outside the range (Bug 967354).

Comment 26

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #24)
> Comment on attachment 8377304 [details] [diff] [review]
> Refactor Tex Image checks.
> 
> Review of attachment 8377304 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +872,5 @@
> > +                                   GLint width, GLint height, GLint depth,
> > +                                   WebGLTexImageSource source)
> > +{
> > +    const GLint maxTextureLevel = MaxTextureLevelForTarget(target);
> > +    const GLint maxTextureSize = 1 << (maxTextureLevel - level);
> 
> These are both 'TexImage', not 'Texture' values.
> 
> We should also do this the other way around. Based on the MAX_TEXTURE_SIZE,
> calculate the maxTexImageSize for a given level.
> 
> size_t MaxTexImageSize(target, level) {
>   size_t max = GetMaxByTarget(target);
>   max >>= level;
>   return max;
> }
> @@ +1696,5 @@
> >  
> >      mGLMaxTextureSize = floorPOT(mGLMaxTextureSize);
> >      mGLMaxRenderbufferSize = floorPOT(mGLMaxRenderbufferSize);
> >  
> > +    mGLMaxTextureLevel = mozilla::FloorLog2(mGLMaxTextureSize);
> 
> This still makes me sad, and I think we shouldn't do it. I'll bring it up
> with the WG again.

I re-read the spec, and you're correct. The rules in section 3.7.1 on Pages 68 and 69 don't list a check against the level number. Yet, the man page linked from the WebGL spec. (http://www.khronos.org/opengles/sdk/docs/man/xhtml/glTexImage2D.xml) says:

"GL_INVALID_VALUE may be generated if level is greater than log2(max), where max is the returned value of GL_MAX_TEXTURE_SIZE when target is GL_TEXTURE_2D or GL_MAX_CUBE_MAP_TEXTURE_SIZE when target is not GL_TEXTURE_2D."

Comment 27

[u480271]

Attachment: Refactor Tex Image checks.

Implemented all clean up and suggestions from first round.

Comment 28

[u480271]

Comment on attachment 8378171 [details] [diff] [review]
Refactor Tex Image checks.

There's a stupid bug I introduced. :-(

Comment 29

[u480271]

Attachment: Refactor Tex Image checks.

Fixed up the compilation and conformance tests from implementing :jgilbert's
commments.

Comment 30

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8378823 [details] [diff] [review]
Refactor Tex Image checks.

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

Close enough. Can patch these nits in a follow up patch.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +751,5 @@
> +
> +    GLint blockWidth, blockHeight;
> +    BlockSizeFor(format, &blockWidth, &blockHeight);
> +
> +    if (blockWidth != 1 || blockHeight != 1) {

This will always be true. It would make more sense if you initialized blockWidth and blockHeight to -1, and BlockSizeFor's "fail" value to not modify the values. Or if BlockSizeFor could fail with -1s.

@@ +875,5 @@
> +
> +    const GLuint maxTexImageSize = MaxTextureSizeForTarget(target) >> level;
> +
> +    if (target >= LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_X &&
> +        target <= LOCAL_GL_TEXTURE_CUBE_MAP_NEGATIVE_Z &&

Man, we should get an IsTexImageTargetCubemap() func.

@@ +1041,5 @@
> +        break;
> +
> +    case LOCAL_GL_FLOAT:
> +    case LOCAL_GL_UNSIGNED_INT:
> +    case LOCAL_GL_UNSIGNED_INT_24_8:

Really UNSIGNED_INT_24_8, being a packed value, should go up with UNSIGNED_SHORT_5_6_5, but returning 32.

@@ +1182,3 @@
>  
> +    case LOCAL_GL_HALF_FLOAT:
> +    case LOCAL_GL_HALF_FLOAT_OES:

This is actually not the current spec'd behavior. We're not supposed to be able to upload HALF_FLOAT_OES from TYPE_UINT16 arrays yet. (I'm working on getting the spec fixed to allow this, still)

Comment 31

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #30)
> Comment on attachment 8378823 [details] [diff] [review]
> Refactor Tex Image checks.
> 
> Review of attachment 8378823 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +1182,3 @@
> >  
> > +    case LOCAL_GL_HALF_FLOAT:
> > +    case LOCAL_GL_HALF_FLOAT_OES:
> 
> This is actually not the current spec'd behavior. We're not supposed to be
> able to upload HALF_FLOAT_OES from TYPE_UINT16 arrays yet. (I'm working on
> getting the spec fixed to allow this, still

:(

Comment 32

[u480271]

Attachment: Address Jeff's nits.

Comment 33

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8378851 [details] [diff] [review]
Address Jeff's nits.

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

Awesome, thanks.

Comment 34

[u480271]

Try runs:
https://tbpl.mozilla.org/?tree=Try&rev=4e53d095e024
https://tbpl.mozilla.org/?tree=Try&rev=40e10125c054

Comment 35

[u480271]

Attachment: Refactor TexImage checks (rolledup)

Rolled up version of previous 4 patches.

Carry r=jgilbert.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/063362390b6e

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/063362390b6e

Comment 38

[Lukas Blakk [:lsblakk] use ?needinfo]

Not sure why this didn't get nominated for uplift but it's too late now, we'll have to let it ride the trains.

Comment 39

[Wayne Mery (:wsmwk)]

Crashes happening only in version 31 of Firefox and Thunderbird with  glTexImage2D near top of stack. Are these regressions?

https://crash-stats.mozilla.com/report/list?signature=libGLImage.dylib%400x111f&product=Thunderbird&query_type=is_exactly&range_unit=weeks&process_type=any&hang_type=any&date=2014-08-20+14%3A00%3A00&range_value=4#tab-reports

https://crash-stats.mozilla.com/report/list?signature=libGLImage.dylib%400x111f&product=Firefox&query_type=is_exactly&range_unit=weeks&process_type=any&hang_type=any&date=2014-08-20+14%3A00%3A00&range_value=4#tab-reports