Closed Bug 682172 Opened 13 years ago Closed 13 years ago

texImage2DBadArgs.html test failures

Categories

(Core :: Graphics: CanvasWebGL, defect)

Product:
Core
Component:
Graphics: CanvasWebGL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla9

People

(Reporter: drs, Assigned: drs)

References

Details

Attachments

(1 file, 4 obsolete files)

Patch v1.4, WebGL texImage2D validation/0-size fix.
14.33 KB, patch
drs
: review+
Details | Diff | Splinter Review 
Two compliance errors occur within texImage2DBadArgs.html:

FAIL: zero size (in assertOk)

(function () { gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, 0, 0, 0, gl.RGBA, gl.UNSIGNED_BYTE, null); })

Error: GL error INVALID_OPERATION in texImage2D
FAIL: bad format (in assertGLError: expected: INVALID_ENUM actual: INVALID_OPERATION)

(function () { gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, 1, 1, 0, gl.FLOAT, gl.UNSIGNED_BYTE, null); })

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/more/functions/texImage2DBadArgs.html

The first error is caused by a supposed mishandling of a zero-size texture, while the second is caused by a mishandling of an enum type mismatch (i.e. an incorrect parameter is passed from an incorrect enum).

The first error is dubious; the source code for this test has the following note:
    // The spec says zero size is OK. If you disagree please list the section
    // in the spec that details this issue.

Indeed, 0 height/width objects have been a source of problems before (see bug 680724). However, this case is very different. Quoting OpenGL ES 2.0.25, pg. 69:

"The level argument to TexImage2D is an integer level-of-detail number. Levels of detail are discussed below, under Mipmapping. The main texture image has a level of detail number of 0 and is known as the level zero array (or the image array of level zero). If level is less than zero, the error INVALID_VALUE is generated. If level is greater than zero, and either width or height is not a power of two, the error INVALID_VALUE is generated."

0 is not a power of 2, thus passing a height or width of 0 should error. However, this only applies when the level parameter is above 0, which in this test it is not. 

The spec also states (same page):
"If wt and ht are the specified image width and height, and if either wt or ht are less than zero, then the error INVALID_VALUE is generated."

Again, they are not less than zero. I can only conclude that the conformance test is wrong in this case.
Assignee: nobody → dsherk
Severity: minor → normal
I made a mistake in my last comment. We are actually *rejecting* this and we're supposed to *accept* it. From the source code for this test:

    // The spec says zero size is OK. If you disagree please list the section
    // in the spec that details this issue.
    assertOk("zero size", function(){
        gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, 0,0,0,gl.RGBA,gl.UNSIGNED_BYTE, null);
    });

This is the only assertOk() in this file. Thus, I'm going to fix this.
There was actually some surrounding logic breaking 0-size textures. It was because there was code that basically checked "did uint=negative_num*other_vars overflow". For incorrect validation, two copies of the same variable (one stored internally and one passed in) were available to the function, but the one stored internally was being validated, while the version passed in wasn't. The fix for this was simply checking the passed var instead.
Attachment #555923 - Flags: review?(bjacob)
Try run for c5fa58e7f432 is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=c5fa58e7f432
Results (out of 28 total builds):
    success: 27
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dsherk@mozilla.com-c5fa58e7f432
I think that was just a random failure. It seemed to have nothing to do with what I changed.
(In reply to Doug Sherk from comment #5)
> I think that was just a random failure. It seemed to have nothing to do with
> what I changed.

Indeed, this sometimes happends. I've retriggered the build. You can do that by clicking it then clicking the blue "+".
Comment on attachment 555923 [details] [diff] [review]
Patch v1.0, WebGL texImage2D validation/0-size fix.

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

I need at least some more explanation:

::: content/canvas/src/WebGLContextGL.cpp
@@ +4336,5 @@
>  
>      CheckedUint32 checked_neededByteLength
>          = (height-1) * checked_alignedRowSize + checked_plainRowSize;
>  
> +    if (height > 0 && !checked_neededByteLength.valid())

If !checked_neededByteLength.valid(), that means that just computing the byte length (which is width*height*bytes_per_pixel) results in an integer overflow. In this case, for sure we want to generate an error.

It's not clear to me how this change can have fixed a test failures. What were the values of width and height here?

Anyway, unless there's a very specific reason not to (that would then require an explanatory comment) I would implement the fix as a separate if()-condition.
Attachment #555923 - Flags: review?(bjacob) → review-
Refactored the "bytes needed" check to a utility function, WebGLContext::GetPackedOrUnpackedBytesNeeded() and also altered the original fix for 0-size to store a size checked_neededByteLength of 0 if height is 0 (instead of continuing past the error as if nothing happened, when it actually overflowed).
Attachment #555923 - Attachment is obsolete: true
Attachment #556183 - Flags: review?(bjacob)
Try run for 0d9bf28a9a4f is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=0d9bf28a9a4f
Results (out of 30 total builds):
    success: 28
    warnings: 1
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dsherk@mozilla.com-0d9bf28a9a4f
Comment on attachment 556183 [details] [diff] [review]
Patch v1.1, WebGL texImage2D validation/0-size fix.

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

::: content/canvas/src/WebGLContext.h
@@ +433,5 @@
> +                                                 PRUint32 texelSize,
> +                                                 PRUint32 packOrUnpackAlignment,
> +                                                 CheckedUint32 *checked_alignedRowSize = NULL, 
> +                                                 CheckedUint32 *checked_plainRowSize = NULL);
> +

Let me propose a different design:

0) It's only going to be confusing to have signed WebGLsizei and unsigned Uint32. Let's have only PRUint32's around here. Anyways the caller is supposed to have validated against negative arguments earlier.

1) make a helper function to round a size up to the next multiple of a given integer. Like:
CheckedUint32 RoundToNextMultipleOf(PRUint32 x, PRUint32 y)

This can be used to compute the alignedRowSizes.

2) Rename your GetPackedOrUnpackedBytesNeeded function to just GetBytesNeeded, rename texelSize to pixelSize as it's more generic, and remove the 2 last pointer parameters; instead just have this function call RoundToNextMultipleOf to compute the alignedRowSize. This is NOT performance critical. The cost of doing some checked integer arithmetic like that is nothing compared to the cost of a texImage2D call.
Attachment #556183 - Flags: review?(bjacob) → review-
After talking with bjacob, we ended up deciding that the review changes should be slightly different. Here is the list of changes I made: 1) rename texelSize to pixelSize, 2) refactor alignedRowSize calculation to RoundToNextMultipleOf(), 3) move plainRowSize calculation back to service function if it needs it, 4) rename GetPackedOrUnpackedBytesNeeded() to GetImageSize(), 5) remove last two parameters from GetImageSize().
Attachment #556183 - Attachment is obsolete: true
Attachment #557008 - Flags: review?(bjacob)
Comment on attachment 557008 [details] [diff] [review]
Patch v1.2, WebGL texImage2D validation/0-size fix.

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

We're getting there but we'll need 1 more itereation , see especially the bug introduced by calling value() instead of forwarding checked integers:

::: content/canvas/src/WebGLContext.h
@@ +432,5 @@
> +                               WebGLsizei width, 
> +                               PRUint32 pixelSize,
> +                               PRUint32 packOrUnpackAlignment);
> +
> +    // Rounds x to the next multiple of y.

This comment suggests it modifies x. Rather say "Returns x rounded to..."

@@ +433,5 @@
> +                               PRUint32 pixelSize,
> +                               PRUint32 packOrUnpackAlignment);
> +
> +    // Rounds x to the next multiple of y.
> +    CheckedUint32 RoundToNextMultipleOf(CheckedUint32 x, CheckedUint32 y) {

Oh and actuall I agree that the problem is inherent to this function's name, RoundToNextMultipleOf. How about just NextMultipleOf or RoundedToNextMultipleOf ?

::: content/canvas/src/WebGLContextGL.cpp
@@ +2994,5 @@
> +    CheckedUint32 checked_neededByteLength =
> +        GetImageSize(height, 
> +                     width, 
> +                     size,    
> +                     mPixelStorePackAlignment);

maybe no need to use 1 line per argument here, but it's up to you.

@@ +3000,4 @@
>      CheckedUint32 checked_plainRowSize = CheckedUint32(width) * size;
>  
> +    CheckedUint32 checked_alignedRowSize = 
> +        RoundToNextMultipleOf(checked_plainRowSize.value(), mPixelStorePackAlignment);

RoundToNextMultipleOf already takes Checked arguments, so no need to call value(). It's generally preferable to stay withing Checked ints as it preserve the valid-flag.

@@ +3061,5 @@
>          // now, same computation as above to find the size of the intermediate buffer to allocate for the subrect
>          // no need to check again for integer overflow here, since we already know the sizes aren't greater than before
>          PRUint32 subrect_plainRowSize = subrect_width * size;
> +        PRUint32 subrect_alignedRowSize = (subrect_plainRowSize + mPixelStorePackAlignment-1) &
> +            ~PRUint32(mPixelStorePackAlignment-1);

Since this is not performance critical, why not also use RoundToNextMultipleOf here too..? That would remove 1 awkward line of code.

@@ +4321,5 @@
> +    CheckedUint32 checked_neededByteLength = 
> +        GetImageSize(height, 
> +                     width, 
> +                     texelSize, 
> +                     mPixelStoreUnpackAlignment);

Again, these arguments are short enough that I'd put them all on 1 line.

@@ +4521,5 @@
> +    CheckedUint32 checked_neededByteLength = 
> +        GetImageSize(height, 
> +                     width, 
> +                     texelSize, 
> +                     mPixelStoreUnpackAlignment);

Same.

@@ +4557,5 @@
>      int actualSrcFormat = srcFormat == WebGLTexelFormat::Auto ? dstFormat : srcFormat;
>      size_t srcStride = srcStrideOrZero ? srcStrideOrZero : checked_alignedRowSize.value();
>  
>      size_t dstPlainRowSize = texelSize * width;
> +    size_t dstStride = ((dstPlainRowSize + mPixelStoreUnpackAlignment-1) / mPixelStoreUnpackAlignment) * mPixelStoreUnpackAlignment;

Again, I'd use the NextMultipleOf helper here.

::: content/canvas/src/WebGLContextUtils.cpp
@@ +125,5 @@
> +{
> +    CheckedUint32 checked_plainRowSize = CheckedUint32(width) * pixelSize;
> +
> +    // alignedRowSize = row size rounded up to next multiple of packAlignment
> +    CheckedUint32 checked_alignedRowSize = RoundToNextMultipleOf(checked_plainRowSize.value(), packOrUnpackAlignment);

Fail! By calling value() here you're losing the information of whether the above multiplication overflowed.

@@ +129,5 @@
> +    CheckedUint32 checked_alignedRowSize = RoundToNextMultipleOf(checked_plainRowSize.value(), packOrUnpackAlignment);
> +
> +    // if height is 0, we don't need any memory to store this; without this check, we'll get an overflow
> +    CheckedUint32 checked_neededByteLength
> +        = height > 0 ? (height-1) * checked_alignedRowSize + checked_plainRowSize : 0;

How about replacing

height > 0 ? some_long_expression : 0

by

height <= 0 ? 0 : some_long_expression

for readability?
Attachment #557008 - Flags: review?(bjacob) → review-
Addresses issues listed above by bjacob.
Attachment #557008 - Attachment is obsolete: true
Attachment #557073 - Flags: review?(bjacob)
Comment on attachment 557073 [details] [diff] [review]
Patch v1.3, WebGL texImage2D validation/0-size fix.

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

Good thanks. Just a couple of nits, it would be nice if you could address them an carry forward the r+

::: content/canvas/src/WebGLContext.h
@@ +427,5 @@
>      void DoFakeVertexAttrib0(WebGLuint vertexCount);
>      void UndoFakeVertexAttrib0();
>      void InvalidateFakeVertexAttrib0();
>  
> +    CheckedUint32 GetImageSize(WebGLsizei height, 

Something just occured to me: these 2 methods don't need anything in the class so they should either be static or become global functions (still declared in the same header).

@@ +430,5 @@
>  
> +    CheckedUint32 GetImageSize(WebGLsizei height, 
> +                               WebGLsizei width, 
> +                               PRUint32 pixelSize,
> +                               PRUint32 packOrUnpackAlignment);

I would just write 'alignment' instead of packOrUnpackAlignment.
Attachment #557073 - Flags: review?(bjacob) → review+
+r carried from bjacob
Attachment #557073 - Attachment is obsolete: true
Attachment #557246 - Flags: review+
Try run for 7168cdf2f10e is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=7168cdf2f10e
Results (out of 30 total builds):
    success: 29
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dsherk@mozilla.com-7168cdf2f10e
http://hg.mozilla.org/mozilla-central/rev/f56356ee98a4
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: