WebGL's SVGImage buffer should be cleared

WebGL's SVGImage buffer should be cleared

When building SVG image-backed WebGL textures, we're currently drawing
into a reusable buffer.  Since SVG images are not guaranteed to be
opaque, we should clear the buffer as needed.

Update the existing test to cover this logic (the updated version fails
at ToT but passes with the patch).

Also reactivate the imageForRender null-check (ineffective after
http://crrev.com/29203004), and change drawImageIntoBuffer() to take a
ref instead of a raw ptr.

TEST=fast/canvas/webgl/tex-image-and-sub-image-2d-with-svg-image.html

BUG=500180
R=kbr@chromium.org,pdr@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201929

[pdr., 2015-09-08 18:33:58]

https://codereview.chromium.org/1315233003/diff/40001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1315233003/diff/40001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContextBase.cpp:4140: if
(!image->currentFrameKnownToBeOpaque())
Why wasn't this needed for transparent animated gifs where the first frame has
transparency?

[f(malita), 2015-09-08 18:44:45]

https://codereview.chromium.org/1315233003/diff/40001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1315233003/diff/40001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContextBase.cpp:4140: if
(!image->currentFrameKnownToBeOpaque())
On 2015/09/08 18:33:58, pdr wrote:
> Why wasn't this needed for transparent animated gifs where the first frame has
> transparency?

We only ever call drawImageIntoBuffer for SVG images.  All other types are
handled directly by WebGLImageConversion::ImageExtractor and don't need to go
through intermediate buffers.

[pdr., 2015-09-08 18:47:36]

On 2015/09/08 at 18:44:45, fmalita wrote:
>
https://codereview.chromium.org/1315233003/diff/40001/Source/modules/webgl/We...
> File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):
> 
>
https://codereview.chromium.org/1315233003/diff/40001/Source/modules/webgl/We...
> Source/modules/webgl/WebGLRenderingContextBase.cpp:4140: if
(!image->currentFrameKnownToBeOpaque())
> On 2015/09/08 18:33:58, pdr wrote:
> > Why wasn't this needed for transparent animated gifs where the first frame
has
> > transparency?
> 
> We only ever call drawImageIntoBuffer for SVG images.  All other types are
handled directly by WebGLImageConversion::ImageExtractor and don't need to go
through intermediate buffers.

Ahh. And now the comment makes sense.

LGTM

[Ken Russell (switch to Gerrit), 2015-09-08 19:34:01]

kbr@chromium.org changed reviewers:
+ zmo@chromium.org

[Ken Russell (switch to Gerrit), 2015-09-08 19:34:01]

Thanks for this fix. LGTM with one minor comment.

https://codereview.chromium.org/1315233003/diff/40001/LayoutTests/fast/canvas...
File
LayoutTests/fast/canvas/webgl/resources/tex-image-and-sub-image-2d-with-svg-image.js
(right):

https://codereview.chromium.org/1315233003/diff/40001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/webgl/resources/tex-image-and-sub-image-2d-with-svg-image.js:12:
var poisonImageLoaded = false;
zmo: we should port these changes to the WebGL conformance suite.

https://codereview.chromium.org/1315233003/diff/40001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1315233003/diff/40001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContextBase.cpp:4131: ASSERT(image);
If this is being changed to take PassRefPtr<Image> then please make a RefPtr for
it upon entry. Otherwise it's easy to accidentally call the copy constructor of
PassRefPtr, zeroing its internal field and causing crashes later.

[f(malita), 2015-09-08 19:51:42]

https://codereview.chromium.org/1315233003/diff/40001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1315233003/diff/40001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContextBase.cpp:4131: ASSERT(image);
On 2015/09/08 19:34:01, Ken Russell wrote:
> If this is being changed to take PassRefPtr<Image> then please make a RefPtr
for
> it upon entry. Otherwise it's easy to accidentally call the copy constructor
of
> PassRefPtr, zeroing its internal field and causing crashes later.

Done.

[f(malita), 2015-09-08 19:51:46]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2015-09-08 19:51:46]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1315233003/#ps60001
(title: "review comments")

[commit-bot: I haz the power, 2015-09-08 19:51:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1315233003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1315233003/60001

[Zhenyao Mo, 2015-09-08 20:07:24]

On 2015/09/08 19:34:01, Ken Russell wrote:
> Thanks for this fix. LGTM with one minor comment.
> 
>
https://codereview.chromium.org/1315233003/diff/40001/LayoutTests/fast/canvas...
> File
>
LayoutTests/fast/canvas/webgl/resources/tex-image-and-sub-image-2d-with-svg-image.js
> (right):
> 
>
https://codereview.chromium.org/1315233003/diff/40001/LayoutTests/fast/canvas...
>
LayoutTests/fast/canvas/webgl/resources/tex-image-and-sub-image-2d-with-svg-image.js:12:
> var poisonImageLoaded = false;
> zmo: we should port these changes to the WebGL conformance suite.

Noted.  I will add this to my TODO list.

[commit-bot: I haz the power, 2015-09-08 21:17:07]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201929

[Ken Russell (switch to Gerrit), 2015-09-08 21:20:35]

On 2015/09/08 20:07:24, Zhenyao Mo wrote:
> On 2015/09/08 19:34:01, Ken Russell wrote:
> > Thanks for this fix. LGTM with one minor comment.
> > 
> >
>
https://codereview.chromium.org/1315233003/diff/40001/LayoutTests/fast/canvas...
> > File
> >
>
LayoutTests/fast/canvas/webgl/resources/tex-image-and-sub-image-2d-with-svg-image.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/1315233003/diff/40001/LayoutTests/fast/canvas...
> >
>
LayoutTests/fast/canvas/webgl/resources/tex-image-and-sub-image-2d-with-svg-image.js:12:
> > var poisonImageLoaded = false;
> > zmo: we should port these changes to the WebGL conformance suite.
> 
> Noted.  I will add this to my TODO list.

Thanks. I filed https://github.com/KhronosGroup/WebGL/issues/1193 to track it.