1240673 - Pass WebGL2 conformance test framebuffer-test

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Ethan Lin[:ethlin]]

we should pass the test

Comment 1

[Ethan Lin[:ethlin]]

Attachment: Fix framebuffer-test problem.

This patch should fix the test.

Comment 2

[Ethan Lin[:ethlin]]

Attachment: Fix framebuffer-test problem.

Check the size of texture and do finalize attachment before getting parameter from attachment.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8709852 [details] [diff] [review]
Fix framebuffer-test problem.

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

::: dom/canvas/WebGLContextGL.cpp
@@ +557,5 @@
> +     *   level must be greater than or equal to zero and no larger than
> +     *   log2 of the value of MAX_TEXTURE_SIZE. Otherwise, an
> +     *   INVALID_VALUE error is generated.
> +     */
> +    if (IsWebGL2()) {

There is an `if (!IsWebGL2() && level != 0) {` that should be merged into an `else` or `else if` attached to this `if` block.

@@ +560,5 @@
> +     */
> +    if (IsWebGL2()) {
> +        if (textarget == LOCAL_GL_TEXTURE_2D) {
> +            if (uint32_t(level) > FloorLog2(mImplMaxTextureSize)) {
> +              ErrorInvalidValue("framebufferTexture2D: level must be 0.");

The text here is wrong.

"framebufferTexture2D: `level` is too large." is fine.

@@ +563,5 @@
> +            if (uint32_t(level) > FloorLog2(mImplMaxTextureSize)) {
> +              ErrorInvalidValue("framebufferTexture2D: level must be 0.");
> +              return;
> +            }
> +        } else {

Assert that textarget is a cubemap TexImageTarget.

@@ +564,5 @@
> +              ErrorInvalidValue("framebufferTexture2D: level must be 0.");
> +              return;
> +            }
> +        } else {
> +          uint32_t cudeSize = MINVALUE_GL_MAX_CUBE_MAP_TEXTURE_SIZE;

'cube' not 'cude', but this should just be:
const auto maxSize = mImplMaxCubeMapTextureSize;

Comment 4

[Ethan Lin[:ethlin]]

Attachment: Fix framebuffer-test problem.

Address jgilbert's comment.

Comment 5

[Ethan Lin[:ethlin]]

Attachment: framebuffer-test problem.

Re-upload patch.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8710232 [details] [diff] [review]
framebuffer-test problem.

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

::: dom/canvas/WebGLContextGL.cpp
@@ +560,5 @@
> +              ErrorInvalidValue("framebufferTexture2D: level is too large.");
> +              return;
> +            }
> +        } else if (textarget >= LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_X &&
> +                   textarget <= LOCAL_GL_TEXTURE_CUBE_MAP_NEGATIVE_Z) {

You shouldn't need to check this at runtime. You should only need to assert it. textarget should have been validated in ValidateFramebufferTarget or elsewhere. If it's not, we need to validate it.

This means this code may be incomplete, but it should be a strict improvement over the present code.

Comment 7

[Ethan Lin[:ethlin]]

Attachment: fix framebuffer-test problem.

The textarget check is behind the size check. I move the textarget check position and use assertion when checking size. Please help review again.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8710281 [details] [diff] [review]
fix framebuffer-test problem.

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

::: dom/canvas/WebGLContextGL.cpp
@@ +564,5 @@
> +         */
> +
> +        if (textarget == LOCAL_GL_TEXTURE_2D) {
> +            if (uint32_t(level) > FloorLog2(mImplMaxTextureSize)) {
> +              ErrorInvalidValue("framebufferTexture2D: level is too large.");

4-space indents please

Comment 9

[Ethan Lin[:ethlin]]

Attachment: fix framebuffer-test problem. (carry r+: jgilbert)

Address jgilbert's comments.

Comment 10

[Ethan Lin[:ethlin]]

try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=868ed726e8df

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8dad9491cc3

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e8dad9491cc3