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

RESOLVED FIXED in Firefox 39

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Guillaume Abadie, Assigned: kamidphish)

Tracking

unspecified
mozilla39
x86
All
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [gfx-noted], webgl-conformance)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Comment 1

5 years ago
Working revision : 138979:aeb5c295ead1
Failing revision : 138984:da575285ae04


History between those two revisions (hg log -r aeb5c295ead1::da575285ae04) :
changeset:   138979:aeb5c295ead1
user:        Matt Woodrow <mwoodrow@mozilla.com>
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 <mwoodrow@mozilla.com>
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 <mwoodrow@mozilla.com>
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 <mwoodrow@mozilla.com>
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 <mwoodrow@mozilla.com>
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 <mwoodrow@mozilla.com>
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
Flags: needinfo?(matt.woodrow)
Assignee: nobody → wlitwinczyk
Depends on: 1049960
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.
Flags: needinfo?(matt.woodrow)

Comment 4

3 years ago
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

3 years ago
Can a Mozillian please update the platform to include Windows? Verified on Windows 7.
OS: Mac OS X → All

Comment 6

3 years ago
Also the webgl-conformance flag? Thanks!
(Assignee)

Comment 7

3 years ago
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.
Whiteboard: [gfx-noted], webgl-conformance
(Assignee)

Comment 8

3 years ago
Created attachment 8563955 [details] [diff] [review]
Work around glCopyTexImage2D errors on OSX

This works around the errors with glCopyTexImage2D from framebuffers backed by textures allocated via IOSurface.
Attachment #8563955 - Flags: review?(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?
Attachment #8563955 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 10

3 years ago
(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.
(Assignee)

Comment 11

3 years ago
Created attachment 8565827 [details] [diff] [review]
Work around glCopyTexImage2D errors on framebuffers backed by IOSurface

Put back the missing pre/post copy calls.
Attachment #8565827 - Flags: review?(jgilbert)
(Assignee)

Updated

3 years ago
Attachment #8563955 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Assignee: wlitwinczyk → dglastonbury
Status: NEW → ASSIGNED
(Assignee)

Comment 12

3 years ago
I suppose this will be an issue for sub variant too.
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.
Attachment #8565827 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 16

3 years ago
Created attachment 8578425 [details] [diff] [review]
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)???!
Attachment #8578425 - Flags: review?(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.
Attachment #8578425 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 18

3 years ago
> 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()?
Flags: needinfo?(jgilbert)
(Assignee)

Comment 19

3 years ago
Created attachment 8581458 [details] [diff] [review]
Work around glCopyTexImage2D errors on framebuffers backed by IOSurface.
Attachment #8578425 - Attachment is obsolete: true
Attachment #8581458 - Flags: review?(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.
Flags: needinfo?(jgilbert)
Attachment #8581458 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 21

3 years ago
Created attachment 8582097 [details] [diff] [review]
Skip processing of CopyTexImage2D with 0 width or height.
Attachment #8582097 - Flags: review?(jgilbert)
Attachment #8582097 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/794e1729fb0d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.