Closed Bug 966624 Opened 6 years ago Closed 6 years ago

WebGL crash [@mozilla::GetWebGLTexelFormat]

Categories

(Core :: Canvas: WebGL, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 + wontfix
firefox30 --- fixed

People

(Reporter: posidron, Assigned: kamidphish)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Attachments

(3 files, 10 obsolete files)

326 bytes, text/html
Details
2.95 KB, text/plain
Details
85.10 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
Attached file callstack
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
Dan, comment 2's regression window makes this a likely regression from Bug 948002.
Blocks: 948002
Flags: needinfo?(dglastonbury)
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.
Attachment #8369338 - Flags: review?(jgilbert)
Assignee: nobody → dglastonbury
Status: NEW → ASSIGNED
(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.
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.
Flags: needinfo?(dglastonbury) → needinfo?(bjacob)
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).
Flags: needinfo?(bjacob)
Attachment #8369338 - Flags: review?(jgilbert) → review+
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.
Attachment #8375413 - Flags: review?(jgilbert)
Attached patch Refactor Tex Image checks. (obsolete) — Splinter Review
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)
Attachment #8375420 - Flags: review?(jgilbert)
Attachment #8369338 - Attachment is obsolete: true
Hmmm. Guess that didn't work.
Attached patch Refactor Tex Image checks. (obsolete) — Splinter Review
Fix compilation errors on other platforms.
Attachment #8376070 - Flags: review?(jgilbert)
Attachment #8375420 - Attachment is obsolete: true
Attachment #8375420 - Flags: review?(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?
This should be a simple one.
Attachment #8377303 - Flags: review?(jgilbert)
Attached patch Refactor Tex Image checks. (obsolete) — Splinter Review
Fix up silly missing parameters to ErrorInvalidXXX().  Mea culpa.
Attachment #8377304 - Flags: review?(jgilbert)
Attachment #8376070 - Attachment is obsolete: true
Attachment #8376070 - Flags: review?(jgilbert)
Refreshing patches.
Attachment #8377305 - Flags: review?(jgilbert)
Attachment #8375413 - Attachment is obsolete: true
Attachment #8375413 - Flags: review?(jgilbert)
(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.
Attachment #8377303 - Flags: review?(jgilbert) → review+
(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.
Specifically:
enum WebGLTexImageBaseFunc {
  TexImage,
  CopyTexImage,
  CompressedTexImage,
}

struct WebGLTexImageFunc {
  WebGLTexImageBaseFunc func;
  bool sub;
};
I think more specifically, I don't like trying to optimize to the extent of having a 'sub' bit.
(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.
(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 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.
Attachment #8377304 - Flags: review?(jgilbert) → review-
Attachment #8377305 - Flags: review?(jgilbert) → review+
(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).
(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."
Attached patch Refactor Tex Image checks. (obsolete) — Splinter Review
Implemented all clean up and suggestions from first round.
Attachment #8378171 - Flags: review?(jgilbert)
Attachment #8377304 - Attachment is obsolete: true
Comment on attachment 8378171 [details] [diff] [review]
Refactor Tex Image checks.

There's a stupid bug I introduced. :-(
Attachment #8378171 - Attachment is obsolete: true
Attachment #8378171 - Flags: review?(jgilbert)
Attached patch Refactor Tex Image checks. (obsolete) — Splinter Review
Fixed up the compilation and conformance tests from implementing :jgilbert's
commments.
Attachment #8378823 - Flags: review?(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)
Attachment #8378823 - Flags: review?(jgilbert) → review+
(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

:(
Attached patch Address Jeff's nits. (obsolete) — Splinter Review
Attachment #8378851 - Flags: review?(jgilbert)
Comment on attachment 8378851 [details] [diff] [review]
Address Jeff's nits.

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

Awesome, thanks.
Attachment #8378851 - Flags: review?(jgilbert) → review+
Attachment #8377303 - Attachment is obsolete: true
Attachment #8377305 - Attachment is obsolete: true
Attachment #8378823 - Attachment is obsolete: true
Attachment #8378851 - Attachment is obsolete: true
Rolled up version of previous 4 patches.

Carry r=jgilbert.
Attachment #8379349 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/063362390b6e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 975824
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.
You need to log in before you can comment on or make changes to this bug.