Bug 70646 – Ensure periodic flushing of accelerated canvas drawing context

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Matthew Delaney]

If flushes don't happen regularly enough on a canvas drawing context that is accelerated, subsequent call performance can get pretty bad. "Regularly enough" is geared toward a 30-60hz paint cycle. So, this patch ensures that we're getting flushes of a rate in that range if they're not already happening (e.g. from HTMLCanvasElement::paint calls) in that range.

[Matthew Delaney]

Created attachment 112020 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 112020 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:168
> +    m_context = adoptPtr(new GraphicsContext(cgContext.get()));

Should you set m_data.m_lastFlushTime on context creation?

[Darin Adler]

Comment on attachment 112020 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:184
>  GraphicsContext* ImageBuffer::context() const

Is this really the best bottleneck for the timing logic? Can we instead put this at the ends of the drawing operations that accumulate state in the context? Or are those too many of those. This hook seems particularly awkward since it’s just a getter.

[Darin Adler]

Comment on attachment 112020 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:188
> +        double elapsedTime = currentTimeMS() - m_data.m_lastFlushTime;

What is the performance cost of doing this every time we call context()? Seems like it might be high.

[Matthew Delaney]

(In reply to comment #2)
> Should you set m_data.m_lastFlushTime on context creation?
Yea, that's better.

(In reply to comment #3)
> > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:184
> >  GraphicsContext* ImageBuffer::context() const
> 
> Is this really the best bottleneck for the timing logic? Can we instead put this at the ends of the drawing operations that accumulate state in the context? Or are those too many of those. This hook seems particularly awkward since it’s just a getter.
That's what I had originally, but there are about 5-10 calls and it would require having all this logic in GraphicsContext which becomes a bit more cluttered.


(In reply to comment #4)
> (From update of attachment 112020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112020&action=review
> 
> > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:188
> > +        double elapsedTime = currentTimeMS() - m_data.m_lastFlushTime;
> 
> What is the performance cost of doing this every time we call context()? Seems like it might be high.
Running it locally, I didn't notice any difference in perf on one page I had open. I'll try it out on a handful of other pages now.

[Matthew Delaney]

Couldn't find any performance deficit, so I think we're good. Committing now.

[Matthew Delaney]

Committed r98171: <http://trac.webkit.org/changeset/98171>