Bug 100714 - Temporarily restrict use of ImageBufferSkia::copyToPlatformTexture until bugs resolved
Summary: Temporarily restrict use of ImageBufferSkia::copyToPlatformTexture until bugs...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on:
Blocks: 100715 100971
  Show dependency treegraph
 
Reported: 2012-10-29 18:11 PDT by Kenneth Russell
Modified: 2012-11-01 11:16 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.96 KB, patch)
2012-10-29 18:19 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (3.36 KB, patch)
2012-10-30 13:34 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2012-10-29 18:11:27 PDT
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.
Comment 1 Kenneth Russell 2012-10-29 18:14:17 PDT
https://bugs.webkit.org/show_bug.cgi?id=100715 filed about re-enabling this code path.
Comment 2 Kenneth Russell 2012-10-29 18:19:06 PDT
Created attachment 171352 [details]
Patch
Comment 3 Kenneth Russell 2012-10-30 12:44:19 PDT
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 .
Comment 4 Kenneth Russell 2012-10-30 13:05:12 PDT
Changing synopsis to indicate that this is no longer a Chromium-specific bug, since the workaround will go in shared code.
Comment 5 Kenneth Russell 2012-10-30 13:34:20 PDT
Created attachment 171512 [details]
Patch
Comment 6 Stephen White 2012-10-30 14:12:57 PDT
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?
Comment 7 Kenneth Russell 2012-10-30 15:24:48 PDT
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.)
Comment 8 Stephen White 2012-10-30 16:35:43 PDT
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.
Comment 9 WebKit Review Bot 2012-10-30 17:00:33 PDT
Comment on attachment 171512 [details]
Patch

Clearing flags on attachment: 171512

Committed r132965: <http://trac.webkit.org/changeset/132965>
Comment 10 WebKit Review Bot 2012-10-30 17:00:37 PDT
All reviewed patches have been landed.  Closing bug.