Bug 100714 – Temporarily restrict use of ImageBufferSkia::copyToPlatformTexture until bugs resolved

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

Per http://crbug.com/153925, there are failures of the WebGL conformance test https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/tex-image-and-sub-image-2d-with-canvas-rgba5551.html with the accelerated canvas -> WebGL texture upload path because glCopyTextureCHROMIUM doesn't handle all cases -- in particular, when the destination texture is not renderable. While we are determining how to fix either the implementation of this API or the API itself, it's important to get this test passing again, so we should disable the optimized upload path temporarily.

Note that I am not sure why this has not been caught by the layout test fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas-rgba5551.html. Will look into this.

[Kenneth Russell]

https://bugs.webkit.org/show_bug.cgi?id=100715 filed about re-enabling this code path.

[Kenneth Russell]

Created attachment 171352 [details]
Patch

[Kenneth Russell]

Comment on attachment 171352 [details]
Patch

I'm going to try a different approach suggested by senorblanco on http://code.google.com/p/chromium/issues/detail?id=153925 .

[Kenneth Russell]

Changing synopsis to indicate that this is no longer a Chromium-specific bug, since the workaround will go in shared code.

[Kenneth Russell]

Created attachment 171512 [details]
Patch

[Stephen White]

Comment on attachment 171512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171512&action=review

OK, bots willing.  It's a shame that there isn't a failing test that could go green (maybe our Mesa layer allows 5551 to be bound for rendering?)  r=me

> Source/WebCore/platform/graphics/ImageBuffer.h:123
> +        // FIXME: current implementations of this method have the restriction that they only work
> +        // with textures that are RGBA format and UNSIGNED_BYTE type.

Nit:  maybe we should just say color-renderable textures?

[Kenneth Russell]

Comment on attachment 171512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171512&action=review

> It's a shame that there isn't a failing test that could go green (maybe our Mesa layer allows 5551 to be bound for rendering?)

Are we even testing accelerated 2D canvas in the platform/virtual/gpu versions of these tests? If so, then yes, Mesa is probably treating 5551 textures as color-renderable.

>> Source/WebCore/platform/graphics/ImageBuffer.h:123
>> +        // with textures that are RGBA format and UNSIGNED_BYTE type.
> 
> Nit:  maybe we should just say color-renderable textures?

I would rather the comment match the check in the code, and it's infeasible to figure out up front which formats are color-renderable. (On AMD cards on OS X, RGBA/5551 textures are renderable.)

[Stephen White]

Comment on attachment 171512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171512&action=review

>>> Source/WebCore/platform/graphics/ImageBuffer.h:123
>>> +        // with textures that are RGBA format and UNSIGNED_BYTE type.
>> 
>> Nit:  maybe we should just say color-renderable textures?
> 
> I would rather the comment match the check in the code, and it's infeasible to figure out up front which formats are color-renderable. (On AMD cards on OS X, RGBA/5551 textures are renderable.)

But the check is not actually in this code, it's in the caller.  So technically this function would work with anything color-renderable.  You could say something like "current implementations of this method are not guaranteed to work with anything other than RGBA/UNSIGNED_BYTE".  Although if I read the spec correctly, even that is not true, since the GLES2 spec only requires at a minimum that one of RGBA4, RGB5_1 or RGB565 be color-renderable, and it's up to you and a bunch of glCheckFramebufferStatus() to figure it out.  Yuck.

Anyway this is all No Big Deal.  You may go about your business.

[WebKit Review Bot]

Comment on attachment 171512 [details]
Patch

Clearing flags on attachment: 171512

Committed r132965: <http://trac.webkit.org/changeset/132965>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.