Closed Bug 1240673 Opened 4 years ago Closed 4 years ago

Pass WebGL2 conformance test framebuffer-test

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: ethlin, Assigned: ethlin)

References

()

Details

Attachments

(1 file, 5 obsolete files)

we should pass the test
Attached patch Fix framebuffer-test problem. (obsolete) — Splinter Review
This patch should fix the test.
Depends on: 1240662
Component: Canvas: 2D → Canvas: WebGL
Attached patch Fix framebuffer-test problem. (obsolete) — Splinter Review
Check the size of texture and do finalize attachment before getting parameter from attachment.
Attachment #8709314 - Attachment is obsolete: true
Attachment #8709852 - Flags: review?(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;
Attachment #8709852 - Flags: review?(jgilbert) → review-
Attached patch Fix framebuffer-test problem. (obsolete) — Splinter Review
Address jgilbert's comment.
Attachment #8709852 - Attachment is obsolete: true
Attached patch framebuffer-test problem. (obsolete) — Splinter Review
Re-upload patch.
Attachment #8710198 - Attachment is obsolete: true
Attachment #8710232 - Flags: review?(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.
Attachment #8710232 - Flags: review?(jgilbert) → review+
Attached patch fix framebuffer-test problem. (obsolete) — Splinter Review
The textarget check is behind the size check. I move the textarget check position and use assertion when checking size. Please help review again.
Attachment #8710232 - Attachment is obsolete: true
Attachment #8710281 - Flags: review?(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
Attachment #8710281 - Flags: review?(jgilbert) → review+
Address jgilbert's comments.
Attachment #8710281 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e8dad9491cc3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.