1238865 - Pass WebGL2 conformance test read-pixels-from-fbo-test

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

We should pass the conformance2 test 'read-pixels-from-fbo-test'.

Comment 1

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Validate attachments before clearBuffer.

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Only query auxReadType/Format when type is not float or integer.

Comment 3

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 3: Add more format/type checks for WebGL2.

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Some parts of test are wrong, too. I'll submit a PR to upstream to fix problem.

Comment 5

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Upstream PR is here
https://github.com/KhronosGroup/WebGL/pull/1427

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8706758 [details] [diff] [review]
Part 1: Validate attachments before clearBuffer.

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

::: dom/canvas/WebGL2ContextMRTs.cpp
@@ +57,2 @@
>      MakeContextCurrent();
> +    if (!mBoundDrawFramebuffer) {

We should still clear in this case.

I believe you want:
if (mBoundDrawFramebuffer) {
    if (!mBoundDrawFramebuffer->ValidateAndInitAttachments(funcName))
        return;
}

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8706759 [details] [diff] [review]
Part 2: Only query auxReadType/Format when type is not float or integer.

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

::: dom/canvas/WebGLContextGL.cpp
@@ +1534,5 @@
>      // OpenGL ES 2.0 $4.3.1 - IMPLEMENTATION_COLOR_READ_{TYPE/FORMAT} is a valid
>      // combination for glReadPixels().
> +    if (gl->IsSupported(gl::GLFeature::ES2_compatibility) &&
> +        (srcType == webgl::ComponentType::NormInt ||
> +         srcType == webgl::ComponentType::NormUInt))

Why? It seems like this should still be valid.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8706760 [details] [diff] [review]
Part 3: Add more format/type checks for WebGL2.

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

::: dom/canvas/WebGLContextGL.cpp
@@ +1461,2 @@
>      case LOCAL_GL_UNSIGNED_INT:
> +        bytesPerPixel = 4*channels;

Good catch.

@@ +1568,5 @@
> +    bool isValid = mainMatches || auxMatches;
> +
> +    // OpenGL ES 3.0 p194 - When the internal format of the rendering surface is
> +    // RGB10_A2, a third combination of format RGBA and type UNSIGNED_INT_2_10_10_10_REV
> +    // is accepted.

Cool, thanks for saying where you found this in the spec.

Is this really ES 3.0 though? Please use the current newest spec. (3.0.4)

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Validate attachments before clearBuffer.

Address jgilbert's comment.

Comment 10

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add more format/type checks for WebGL2.

Jeff, I added more format/type then previous patch. Can you review it again? Thanks.

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> Comment on attachment 8706759 [details] [diff] [review]
> Part 2: Only query auxReadType/Format when type is not float or integer.
> 
> Review of attachment 8706759 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLContextGL.cpp
> @@ +1534,5 @@
> >      // OpenGL ES 2.0 $4.3.1 - IMPLEMENTATION_COLOR_READ_{TYPE/FORMAT} is a valid
> >      // combination for glReadPixels().
> > +    if (gl->IsSupported(gl::GLFeature::ES2_compatibility) &&
> > +        (srcType == webgl::ComponentType::NormInt ||
> > +         srcType == webgl::ComponentType::NormUInt))
> 
> Why? It seems like this should still be valid.

Yes, I removed this part in my latest patch.

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 3: Prevent IMPLEMENTATION_COLOR_READ_FORMAT return SRGB_ALPHA.

This changes is tricky. If the internal format of frame buffer is SRGB8_ALPHA8. Then
we get SRGB_ALPHA for IMPLEMENTATION_COLOR_READ_FORMAT. But SRGB_ALPHA is not supported
by ReadPixels. So I do a slightly change here to prevent IMPLEMENTATION_COLOR_READ_FORMAT
return SRGB_ALPHA.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8707801 [details] [diff] [review]
Part 3: Prevent IMPLEMENTATION_COLOR_READ_FORMAT return SRGB_ALPHA.

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

::: dom/canvas/WebGLContextState.cpp
@@ +335,5 @@
> +            // If internal format of fbo is SRGB8_ALPHA8, then
> +            // IMPLEMENTATION_COLOR_READ_FORMAT is SRGB_ALPHA which is not supported
> +            // by ReadPixels. So, just return RGBA here.
> +            if (i == LOCAL_GL_SRGB_ALPHA)
> +                i = LOCAL_GL_RGBA;

Can you quote where the spec doesn't include sRGBA for ReadPixels?

Comment 14

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07f9a5e5c342

Comment 15

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

fix failure

https://treeherder.mozilla.org/#/jobs?repo=try&revision=750a3c7f1566

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cd90b7d3dbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/136001a011e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d556ea474a82

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2cd90b7d3dbd
https://hg.mozilla.org/mozilla-central/rev/136001a011e8
https://hg.mozilla.org/mozilla-central/rev/d556ea474a82