Bug 76635 - Refactor canvas drawing to be more data driven
Summary: Refactor canvas drawing to be more data driven
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2012-01-19 07:28 PST by Tim Dresser
Modified: 2012-01-20 11:23 PST (History)
4 users (show)

See Also:

Patch (24.31 KB, patch)
2012-01-19 07:47 PST, Tim Dresser
no flags Details | Formatted Diff | Diff
Patch (9.57 KB, patch)
2012-01-19 14:06 PST, Tim Dresser
no flags Details | Formatted Diff | Diff
Patch for landing (9.57 KB, patch)
2012-01-20 07:52 PST, Tim Dresser
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Dresser 2012-01-19 07:28:57 PST
From bug 73059:

CCLayerImpl-derived classes all currently handle drawing themselves internally, making direct GL calls.  This is problematic for several reasons.  Testing these layers is tricky because testing must be done at a mock GL interface level rather than at a higher abstract level.  It makes culling closely dependent on how each layer draws.  It also makes some optimizations like layer squashing impossible.

To fix this, CCLayerImpl-derived classes should produce a set of quads to render with some basic information in them about how they are to be drawn.  This list can then be processed and drawn in a more data-driven and abstract manner.

CCCanvasLayerImpl still handles drawing internally, and should be transitioned to the data-driven approach.
This builds on the initial refactor from bug 76274.
Comment 1 Tim Dresser 2012-01-19 07:47:13 PST
Created attachment 123126 [details]
Comment 2 Adrienne Walker 2012-01-19 11:06:59 PST
Comment on attachment 123126 [details]

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:687
> +    if (!quad->hasAlpha()) {
> +        // Even though the WebGL layer's texture was likely allocated
> +        // as GL_RGB, disable blending anyway for better robustness.
> +        context()->disable(GraphicsContext3D::BLEND);
> +    } else {
> +        GC3Denum sfactor = quad->premultipliedAlpha() ? GraphicsContext3D::ONE : GraphicsContext3D::SRC_ALPHA;
> +        GLC(context(), context()->blendFunc(sfactor, GraphicsContext3D::ONE_MINUS_SRC_ALPHA));
> +    }

Enabling/disabling alpha is already done by the drawQuad routine above because all quads need that commonly, so no need to do that here.  Can you also make sure blendFunc gets reset properly, since it only is currently set at the beginning of the frame?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:695
> +    if (!quad->hasAlpha())
> +        context()->enable(GraphicsContext3D::BLEND);

Not needed either for the above reason.

> Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:53
> +    const IntSize& bounds() const { return m_sharedQuadState->bounds(); }

I don't think this function needs to be added to either CCDrawQuad or the shared quad state.  Isn't bounds() just quad->quadRect().size()?
Comment 3 Tim Dresser 2012-01-19 14:06:01 PST
Created attachment 123191 [details]
Comment 4 Adrienne Walker 2012-01-19 14:09:28 PST
Comment on attachment 123191 [details]

Thanks! This looks good to me.  jamesr?
Comment 5 James Robinson 2012-01-19 14:15:13 PST
Comment on attachment 123191 [details]

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

R=me, one nit

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:685
> +    drawTexturedQuad(quad->layerTransform(), quad->quadRect().size().width(), quad->quadRect().size().height(), quad->opacity(), sharedGeometryQuad(),

can I get a temporary const reference to size in this piece? this line's pretty long, even by webkit standards
Comment 6 Tim Dresser 2012-01-20 07:52:18 PST
Created attachment 123316 [details]
Patch for landing
Comment 7 WebKit Review Bot 2012-01-20 07:54:06 PST
Comment on attachment 123316 [details]
Patch for landing

Rejecting attachment 123316 [details] from commit-queue.

tdresser@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 8 W. James MacLean 2012-01-20 09:45:02 PST
Comment on attachment 123316 [details]
Patch for landing

Submitting to commit queue on behalf of Tim Dresser.
Comment 9 WebKit Review Bot 2012-01-20 11:23:31 PST
Comment on attachment 123316 [details]
Patch for landing

Clearing flags on attachment: 123316

Committed r105529: <http://trac.webkit.org/changeset/105529>
Comment 10 WebKit Review Bot 2012-01-20 11:23:39 PST
All reviewed patches have been landed.  Closing bug.