Closed Bug 1238865 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: mtseng, Assigned: mtseng)

References

()

Details

Attachments

(3 files, 3 obsolete files)

We should pass the conformance2 test 'read-pixels-from-fbo-test'.
Some parts of test are wrong, too. I'll submit a PR to upstream to fix problem.
Status: NEW → ASSIGNED
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;
}
Attachment #8706758 - Flags: review?(jgilbert) → review-
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.
Attachment #8706759 - Flags: review?(jgilbert) → review-
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)
Attachment #8706760 - Flags: review?(jgilbert) → review+
Address jgilbert's comment.
Attachment #8707768 - Flags: review?(jgilbert)
Attachment #8706758 - Attachment is obsolete: true
Attachment #8706759 - Attachment is obsolete: true
Attachment #8706760 - Attachment is obsolete: true
Jeff, I added more format/type then previous patch. Can you review it again? Thanks.
Attachment #8707769 - Flags: review?(jgilbert)
(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.
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.
Attachment #8707801 - Flags: review?(jgilbert)
Attachment #8707768 - Flags: review?(jgilbert) → review+
Attachment #8707769 - Flags: review?(jgilbert) → review+
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?
Attachment #8707801 - Flags: review?(jgilbert) → review+
Depends on: 1242120
You need to log in before you can comment on or make changes to this bug.