Bug 87920 – [chromium] Software compositor initialization and base classes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alexandre Elias]

[chromium] Software compositor initialization and base classes

[Alexandre Elias]

Created attachment 144978 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Dana Jansens]

Comment on attachment 144978 [details]
Patch

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

nice!

> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:126
> +    if (!context3D) {
> +        // ASSERT(false);

How come the assert is commented out here, and in a number of other places?

Also, ASSERT(false) is generally done via ASSERT_NOT_REACHED() I think, or assert on opposite thing as in the if, before doing the if()return.

[Adrienne Walker]

Comment on attachment 144978 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:79
> +class CCRenderer {

Please put this in a separate file.  Same with the definition.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:99
> +    const FloatQuad& sharedGeometryQuad() const { return m_sharedGeometryQuad; }

Does this belong in the base class?  It seems like a GL implementation detail.  Are you really planning to use this for software mode?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:122
> +    static void toGLMatrix(float*, const TransformationMatrix&);

Same with this.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:132
> +    virtual void drawTexturedQuad(const TransformationMatrix& layerMatrix,
> +                          float width, float height, float opacity, const FloatQuad&,
> +                          int matrixLocation, int alphaLocation, int quadLocation) = 0;

Same with this.

> Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.cpp:95
> +    if (!context3D) {
> +        // ASSERT(false);
> +        return;
> +    }

I'm guessing that these are just early outs so that you can incrementally get software compositing working.  If so, can you remove the commented assert and instead add a "FIXME: enable this path for software compositing" or some such, here and in a number of other places in the code?

[James Robinson]

Comment on attachment 144978 [details]
Patch

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

This does end up being pretty nice, I think it'll definitely work out.  Some things need addressing of the more nitpicky variety.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:252
> +                                             PassRefPtr<CCGraphicsContext> context,

Why does LRC need a CCGraphicsContext? It seems to me like it'd make more sense for whoever constructs a LRC to have already extracted the GraphicsContext3D and pass that in, since they've clearly already decided to use the GC3D-specific CCRenderer implementation.  I don't think we should need any changes in the implementation of any LayerRendererChromium functions due to CCGraphicsContext

>> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:126
>> +        // ASSERT(false);
> 
> How come the assert is commented out here, and in a number of other places?
> 
> Also, ASSERT(false) is generally done via ASSERT_NOT_REACHED() I think, or assert on opposite thing as in the if, before doing the if()return.

What Dana says is truth. Also, normally we just ASSERT() our invariants and keep going, we don't add a release mode branch for it if we expect it to be a "can never happen" scenario. If we want to make sure we explicitly crash instead of potentially doing weird things we add CRASH() instead of just return, but that's rare

> Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:32
> +#if USE(ACCELERATED_COMPOSITING)

I'd not bother with this guard - we're in the chromium compositor directory, so obviously we have accelerated_compositing enabled. We can't even compile the chromium port these days without USE(ACCELERATED_COMPOSITING) and I don't think we will ever want to, so having the guard is just line noise

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:552
> +    if (!context3d) {
> +        ASSERT(false);
> +        return false;

same comments here RE assert()s

[Alexandre Elias]

Created attachment 145234 [details]
Patch

[Alexandre Elias]

All done.


> Why does LRC need a CCGraphicsContext? It seems to me like it'd make more sense for whoever constructs a LRC to have already extracted the GraphicsContext3D and pass that in, since they've clearly already decided to use the GC3D-specific CCRenderer implementation.  I don't think we should need any changes in the implementation of any LayerRendererChromium functions due to CCGraphicsContext

I needed it because some holders of CCRenderer assumed they could get a CCGraphicsContext from it.  But there weren't too many, mainly CCLayerImpl::willDraw() overriders and CCLayerTreeHostImpl.  I changed to keep around an extra CCGraphicsContext ref in CCLayerTreeHostImpl instead.

> 
> >> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:126
> >> +        // ASSERT(false);
> > 
> > How come the assert is commented out here, and in a number of other places?
> > 
> > Also, ASSERT(false) is generally done via ASSERT_NOT_REACHED() I think, or assert on opposite thing as in the if, before doing the if()return.
> 
> What Dana says is truth. Also, normally we just ASSERT() our invariants and keep going, we don't add a release mode branch for it if we expect it to be a "can never happen" scenario. If we want to make sure we explicitly crash instead of potentially doing weird things we add CRASH() instead of just return, but that's rare

OK, I added an early return with a FIXME in all the places I had that, except I put a CRASH() in WebViewImpl createContext functions, because I want to be alerted if some random component tries to create a context3D in software mode.

[Alexandre Elias]

Created attachment 145363 [details]
Patch

[Alexandre Elias]

Rebased.

[James Robinson]

Comment on attachment 145363 [details]
Patch

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

R=me

> Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:36
> +class CCGraphicsContext : public RefCounted<CCGraphicsContext> {

As a follow-up, could you please make this class be single-ownership instead of refcounted?

[Alexandre Elias]

Agreed, I'm somewhat uneasy about all the refcounting as well.  I put that on my todo list.

[WebKit Review Bot]

Comment on attachment 145363 [details]
Patch

Rejecting attachment 145363 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:

Source/WebKit/chromium/tests/GraphicsLayerChromiumTest.cpp:65: note:   because the following virtual functions are pure within '<unnamed>::MockLayerTreeHostClient':
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:63: note: 	virtual WTF::PassRefPtr<WebCore::GraphicsContext3D> WebCore::CCLayerTreeHostClient::createContext3D()
make: *** [out/Debug/obj.target/webkit_unit_tests/Source/WebKit/chromium/tests/GraphicsLayerChromiumTest.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/12873400

[Alexandre Elias]

Created attachment 145388 [details]
Patch

[Alexandre Elias]

Rebased again and resolved the conflict.

[James Robinson]

Comment on attachment 145388 [details]
Patch

Take 2

[Dana Jansens]

It looks like some layout tests are failing?

[WebKit Review Bot]

Comment on attachment 145388 [details]
Patch

Rejecting attachment 145388 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
FAILED at 69.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/chromium/tests/GraphicsLayerChromiumTest.cpp.rej
patching file Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp
patching file Source/WebKit/chromium/tests/TextureCopierTest.cpp
patching file Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'James Robi..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12859885

[Alexandre Elias]

Created attachment 145426 [details]
Patch

[WebKit Review Bot]

Comment on attachment 145426 [details]
Patch

Clearing flags on attachment: 145426

Committed r119313: <http://trac.webkit.org/changeset/119313>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.