Bug 63871 – [Chromium] canvas/philip/tests/2d.gradient.object.update.html is failed on chromium.

• Component: Canvas
• Status: RESOLVED DUPLICATE

[Dongseong Hwang]

From HTML5 canvas spec:
http://dev.w3.org/html5/2dcontext/#colors-and-styles

"When set to a CanvasPattern or CanvasGradient object, the assignment is live, meaning that changes made to the object after the assignment do affect subsequent stroking or filling of shapes."

However, after setting CanvasPattern or CanvasGradient object and changing the object, changes made to the object after the assignment do not affect subsequent stroking or filling of shapes on chromium.

[Dongseong Hwang]

Created attachment 99570 [details]
patch

[Dongseong Hwang]

[Chromium] Update Gradient and Pattern every time to draw similar to PlatformContext::prepareForSoftwareDraw() 
    
GraphicsContextSkia::set[Fill|Stroke][Gradient|Pattern] has the problem that GraphicsContextSkia does not apply gradient or pattern style if the gradient or pattern is changed after GraphicsContextSkia::set[Fill|Stroke][Gradient|Pattern].
 
For example,
 Gradient* gradient;
 gradient->doSomething();
 graphicsContext.setFillGradient(gradient);
 gradient->addColorStop(...);  <- GraphicsContextSkia does not apply this.

It is because GraphicsContextSkia creates new platform gradient or pattern object and it would be different from original gradient or pattern object in the above case.

This patch makes Chromium comform following Canvas spec.
From HTML5 canvas spec:
http://dev.w3.org/html5/2dcontext/#colors-and-styles

"When set to a CanvasPattern or CanvasGradient object, the assignment is live, meaning that changes made to the object after the assignment do affect subsequent stroking or filling of shapes."
        
This patch makes chromium pass canvas/philip/tests/2d.gradient.object.update.html

[Eric Seidel (no email)]

Really?  We need to call that in so many places?

[Dongseong Hwang]

Created attachment 99773 [details]
patch

[Dongseong Hwang]

Comment on attachment 99773 [details]
patch

I think we need to call before all drawing.
I separated stroke and fill updates like CG port.
CG calls those only on fillPath, fillRect, drawRect, strokePath, and strokRect because other drawing functions render through fillPath, fillRect, drawRect, strokePath, and strokRect. However, skia drawing function directly call SkCanvas's drawing functions.

I wonders how I handle drawLineForTextChecking and drawLineForText.
I put both applyStrokePatternOrGradient and applyFillPatternOrGradient in those.
I need advice.

[Mike Reed]

I agree this looks very invasive. Looking further up, I wonder why skia has this dual between graphicscontext->m_state (which has the gradient) and platformcontext which has the skia gradient. I notice the the webkit Gradient object itself holds a cache of the skia gradient, and it is correctly purged when addColorStop is called. If skia always went directly to m_state.fillGradient, I think we could skip all of these pushes.

Another clue that something funny is going on. Look in GraphicsContext.cpp


#if !USE(SKIA)
void GraphicsContext::setPlatformFillGradient(Gradient*)
{
}

void GraphicsContext::setPlatformFillPattern(Pattern*)
{
}

void GraphicsContext::setPlatformStrokeGradient(Gradient*)
{
}

void GraphicsContext::setPlatformStrokePattern(Pattern*)
{
}
#endif

All of these seem to exist solely for skia. If we didn't make a (premature) copy of the gradient, but reached directly into m_state each time, I think we would (a) fix this bug, and (b) avoid this whole layer of indirection/copying.

[Stephen White]

(In reply to comment #6)
> I agree this looks very invasive. Looking further up, I wonder why skia has this dual between graphicscontext->m_state (which has the gradient) and platformcontext which has the skia gradient. I notice the the webkit Gradient object itself holds a cache of the skia gradient, and it is correctly purged when addColorStop is called. If skia always went directly to m_state.fillGradient, I think we could skip all of these pushes.
> 
> Another clue that something funny is going on. Look in GraphicsContext.cpp
> 
> 
> #if !USE(SKIA)
> void GraphicsContext::setPlatformFillGradient(Gradient*)
> {
> }
> 
> void GraphicsContext::setPlatformFillPattern(Pattern*)
> {
> }
> 
> void GraphicsContext::setPlatformStrokeGradient(Gradient*)
> {
> }
> 
> void GraphicsContext::setPlatformStrokePattern(Pattern*)
> {
> }
> #endif
> 
> All of these seem to exist solely for skia. If we didn't make a (premature) copy of the gradient, but reached directly into m_state each time, I think we would (a) fix this bug, and (b) avoid this whole layer of indirection/copying.

I'm to blame for introducing these, in:

http://trac.webkit.org/changeset/46017

It was to refactor a bunch of cut-and-paste code, so that setupPaintForStroking() and setupPaintForFilling() could handle patterns and gradients, rather than having to cut-and-paste the same 5 lines of code we were using everywhere else for gradients and patterns (see the CL desc).

That may not be relevant anymore; I'm not sure.  But anything we come up with should be checked against the tests that were fixed in that CL, at least.

[Mike Reed]

Ah, that helps. Certainly will explicitly check for regressions against those tests (and all of DRT).

I have a preliminary CL https://bugs.webkit.org/show_bug.cgi?id=64178 that is good in its own right, but will simplify a future CL (which I haven't posted yet) where I think we can address these gradient concerns w/o the extra calls proposed here.

[James Robinson]

Comment on attachment 99773 [details]
patch

Too invasive.

[Dongseong Hwang]

I found that we did not need to handle Pattern, because Pattern is mutable only due to Pattern::setPatternSpaceTransform() and setPatternSpaceTransform() already updates platformPattern.

I agree that this patch is too invasive. It is because SkGradientShader is immutable. We have to change platformGradient in GraphicsContext at every time that Gradient is modified. There is two approaches in order to change the platformGradient object.
1. Above approach.
2. Gradient has the callback for updating platformGradient in GraphicsContextSkia.

The second approach will be similar that DrawingBufferChromium uses WillPublishCallback class for managing a PlatformContextSkia object.
However, it will be more complex that a GradientSkia manages GraphicsContextSkia listners than a DrawingBufferChromium manages a PlatformContextSkia because several GraphicsContext objects can share one Gradient object and even one GraphicsContext can use one Gradient twice (i.e. fill and stroke).

I think two approaches are too invasive. It is why I chose the first invasive approach.

Recommend me better idea, please.

[Mike Reed]

proposed alternate fix: https://bugs.webkit.org/show_bug.cgi?id=64281

[Mike Reed]

https://bugs.webkit.org/show_bug.cgi?id=64281

This CL has landed. Can you confirm whether it fixes the issue for you?

[Stephen White]

(In reply to comment #12)
> https://bugs.webkit.org/show_bug.cgi?id=64281
> 
> This CL has landed. Can you confirm whether it fixes the issue for you?

FYI, 2d.gradient.object.update seems to be passing on the canaries:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/10413

I'll update the test expectations.

[Dongseong Hwang]

OMG!

https://bugs.webkit.org/show_bug.cgi?id=64281 solved it.

I'm sorry for confusion.

[Dongseong Hwang]

*** This bug has been marked as a duplicate of bug 64281 ***