896693 - WebGL conformance test 1.0.2 : regression on textures/copy-tex-image-2d-formats.html

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Guillaume Abadie]

This test now fail :

https://www.khronos.org/registry/webgl/conformance-suites/1.0.2/conformance/textures/copy-tex-image-2d-formats.html

Comment 1

[Guillaume Abadie]

Working revision : 138979:aeb5c295ead1
Failing revision : 138984:da575285ae04


History between those two revisions (hg log -r aeb5c295ead1::da575285ae04) :
changeset:   138979:aeb5c295ead1
user:        Matt Woodrow <[e-mail]>
date:        Wed Jul 17 23:24:14 2013 -0400
summary:     Bug 875232 - Make alpha channel optional for MacIOSurface. r=BenWa

changeset:   138980:0db7008c7f9f
user:        Matt Woodrow <[e-mail]>
date:        Wed Jul 17 23:24:15 2013 -0400
summary:     Bug 875232 - Make most of the GLContext helper functions take a texture target parameter so that we can support GL_TEXTURE_RECTANGLE. r=jgilbert

changeset:   138981:925fe6245160
user:        Matt Woodrow <[e-mail]>
date:        Wed Jul 17 23:24:15 2013 -0400
summary:     Bug 875232 - Add SharedSurface_IOSurface for sharing textures on OSX. r=jgilbert

changeset:   138982:6b8139c0dc74
user:        Matt Woodrow <[e-mail]>
date:        Wed Jul 17 23:24:15 2013 -0400
summary:     Bug 875232 - Workaround glReadPixels being broken with framebuffers backed by an IOSurface by copying to a temporary texture first. r=jgilbert

changeset:   138983:62d61e36d610
user:        Matt Woodrow <[e-mail]>
date:        Wed Jul 17 23:24:15 2013 -0400
summary:     Bug 875232 - Add a BGRX shader program that reads from GL_TEXTURE_RECTANGLE. r=jrmuziel

changeset:   138984:da575285ae04
user:        Matt Woodrow <[e-mail]>
date:        Wed Jul 17 23:24:15 2013 -0400
summary:     Bug 888048 - Use CreateThebesSurfaceAliasForDrawTarget_hack to avoid having multiple cairo_surface_quartz objects for a single CGContext. r=nrc

Comment 2

[Walter Litwinczyk [:walter]]

Bug is still present in webgl 1.0.3 conformance test:

http://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/copy-tex-image-2d-formats.html

Comment 3

[Walter Litwinczyk [:walter]]

I have tracked the problem down to:

http://dxr.mozilla.org/mozilla-central/source/gfx/2d/MacIOSurface.cpp#370

For reasons unknown to me when this is used as the data for the texture, calling glCopyTexImage2D with GL_ALPHA does not work. All other formats work fine (minus GL_LUMINANCE), and GL_RGBA even returns the alpha component. Replacing this with code like:

> gl->fTexImage2D(LOCAL_GL_TEXTURE_RECTANGLE_ARB, 0,
>                 ioSurf->HasAlpha() ? 
>                     LOCAL_GL_RGBA : 
>                     LOCAL_GL_RGB,
>                 ioSurf->GetDevicePixelWidth(),
>                 ioSurf->GetDevicePixelHeight(),
>                 0,
>                 LOCAL_GL_BGRA,
>                 LOCAL_GL_UNSIGNED_BYTE,
>                 nullptr);

works, but obviously causes other problems because mSurfaceIOPtr is not set. I did try a few of the suggestions from: http://uri-labs.com/macosx_headers/CGLIOSurface_h/index.html (couldn't find an official Apple document on it) and they didn't do anything.

Comment 4

[Luke]

This test is still failing with the D3D9, D3D11, and native OGL backends on Firefox Nightly38.0a1 (2015-01-30). However it passes on both Chrome 42.0.2291.1 canary and IE 11.

Unfortunately this regression has already made it into the stable release. Should we consider reverting Matt Woodrow's commit?

Comment 5

[Luke]

Can a Mozillian please update the platform to include Windows? Verified on Windows 7.

Comment 6

[Luke]

Also the webgl-conformance flag? Thanks!

Comment 7

[u480271]

Errors in the web console:

"FAIL at (0, 0) expected: 0,0,0,127 was 0,0,0,0" js-test-pre.js:112
"FAIL at (0, 0) expected: 64,64,64,255 was 0,0,0,255" js-test-pre.js:96
"FAIL at (0, 0) expected: 64,64,64,127 was 0,0,0,0" js-test-pre.js:96
Error: WebGL: copyTexImage2D: format ALPHA is not a subset of the current framebuffer format, which is [Unknown enum name]. copy-tex-image-2d-formats.html:161
Error: WebGL: copyTexImage2D: format LUMINANCE_ALPHA is not a subset of the current framebuffer format, which is [Unknown enum name]. copy-tex-image-2d-formats.html:161
Error: WebGL: copyTexImage2D: format RGBA is not a subset of the current framebuffer format, which is [Unknown enum name]. copy-tex-image-2d-formats.html:161
Error: WebGL: copyTexImage2D: format ALPHA is not a subset of the current framebuffer format, which is RGB. copy-tex-image-2d-formats.html:161
Error: WebGL: copyTexImage2D: format LUMINANCE_ALPHA is not a subset of the current framebuffer format, which is RGB. copy-tex-image-2d-formats.html:161
"FAIL at (0, 0) expected: 64,64,64,255 was 0,0,0,255" js-test-pre.js:96
Error: WebGL: copyTexImage2D: format RGBA is not a subset of the current framebuffer format, which is RGB.

Comment 8

[u480271]

Attachment: Work around glCopyTexImage2D errors on OSX

This works around the errors with glCopyTexImage2D from framebuffers backed by textures allocated via IOSurface.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8563955 [details] [diff] [review]
Work around glCopyTexImage2D errors on OSX

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

::: gfx/gl/GLContext.h
@@ -1068,5 @@
>  
> -        BeforeGLReadCall();
> -        raw_fCopyTexImage2D(target, level, internalformat,
> -                            x, y, width, height, border);
> -        AfterGLReadCall();

Where did my Before/AfterReadCalls go?

Comment 10

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #9)
> Comment on attachment 8563955 [details] [diff] [review]
> Work around glCopyTexImage2D errors on OSX
> 
> Review of attachment 8563955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.h
> @@ -1068,5 @@
> >  
> > -        BeforeGLReadCall();
> > -        raw_fCopyTexImage2D(target, level, internalformat,
> > -                            x, y, width, height, border);
> > -        AfterGLReadCall();
> 
> Where did my Before/AfterReadCalls go?

Cut'n'paste error. I'll add them back.

Comment 11

[u480271]

Attachment: Work around glCopyTexImage2D errors on framebuffers backed by IOSurface

Put back the missing pre/post copy calls.

Comment 12

[u480271]

I suppose this will be an issue for sub variant too.

Comment 13

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=8e4286bae76f

Comment 14

[u480271]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f6836b9c122

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8565827 [details] [diff] [review]
Work around glCopyTexImage2D errors on framebuffers backed by IOSurface

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

::: gfx/gl/GLContext.h
@@ -1929,5 @@
>          mSymbols.fCompileShader(shader);
>          AFTER_GL_CALL;
>      }
>  
> -private:

We still want this private around. We really should friend the calling class, instead of removing the private marker. People should really not use this by itself.

Comment 16

[u480271]

Attachment: Skip processing of CopyTexImage2D with 0 width or height

Apparently, according to the manpage, this should create a "NULL" texture.

According to Mesa source code, it doesn't allocate any storage for the image.
Maybe this should result in TexImage2D(..., 0, 0, NULL)???!

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8578425 [details] [diff] [review]
Skip processing of CopyTexImage2D with 0 width or height

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

::: dom/canvas/WebGLContextGL.cpp
@@ +545,5 @@
> +    // Bug 896693: https://www.opengl.org/sdk/docs/man3/xhtml/glCopyTexImage2D.xml
> +    // says that width or height set to 0 results in a NULL texture. Lets not
> +    // do any work then.
> +    if (width == 0 || height == 0)
> +        return;

This isn't really correct though. We need to replace the specified texImage with a 0x0 texture, just as if TexImage2D(width=0, height=0) was called on it. (which is valid as well)

We should just do the work to make CopyTexSubImage2D_base handle 0 for width and/or height.

Comment 18

[u480271]

> We should just do the work to make CopyTexSubImage2D_base handle 0 for width
> and/or height.

So we should call TexImage2D() in that case? With the current patch I get errors because we end up with a "NULL" texture which fails the completeness check when attached to an FBO.

Maybe I should just return false if width or height is 0 and let raw_fCopyTexImage2D handle it at GLContext::fCopyTexImage2D()?

Comment 19

[u480271]

Attachment: Work around glCopyTexImage2D errors on framebuffers backed by IOSurface.

Comment 20

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #18)
> > We should just do the work to make CopyTexSubImage2D_base handle 0 for width
> > and/or height.
> 
> So we should call TexImage2D() in that case? With the current patch I get
> errors because we end up with a "NULL" texture which fails the completeness
> check when attached to an FBO.
As it should. Is the test wrong?
> 
> Maybe I should just return false if width or height is 0 and let
> raw_fCopyTexImage2D handle it at GLContext::fCopyTexImage2D()?
We *should* be able to pass down a zero width/height call to glCopyTexImage2D. In the absence of driver bugs, let's just pass it through to the driver.

Comment 21

[u480271]

Attachment: Skip processing of CopyTexImage2D with 0 width or height.

Comment 22

[u480271]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=80d9bcc4dc07

Comment 23

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/794e1729fb0d

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/794e1729fb0d