Bug 90325 – [chromium] Use shallow flushes that don't glFlush

• Component: New Bugs
• Status: RESOLVED FIXED

[Brian Anderson]

Shallow flushes that don't glFlush

[Brian Anderson]

Created attachment 150277 [details]
Patch

[Brian Anderson]

Created attachment 150702 [details]
Patch

[Nat Duca]

Comment on attachment 150702 [details]
Patch

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

You'll need to rebase so you're ToT. This patch isn't cleanly applying.

> Source/Platform/chromium/public/WebGraphicsContext3D.h:266
> +    virtual void shallowFlush() { };

Look at the other extensions, I think you'll find that they suffix these methods with CHROMIUM, e.g. texImageIOSurface2DCHROMIUM. They're also supposed to be put down at the bottom of the file, with a // block with the extension name they're part of.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:690
> +#if PLATFORM(CHROMIUM)

You shouldnt edit gc3d, that's common code.

Chrome-specific extensions go on Extensions3DChromium. Look at some of the methods there and grep for their name to see how the plumbing for that is done.

[Brian Anderson]

Created attachment 152386 [details]
Patch

[Nat Duca]

Comment on attachment 152386 [details]
Patch

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

+kbr for real review. I'm happy +/- minor nits.

> Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:57
> +    void shallowFlushCHROMIUM()

What if the flush extension is not supported? I think since this code wraps a GC3D, we could make it probe the context for support of the extension and store that support as a member var. If its not supported, this could degrade to a flush, which I like more. Then you could make this method name just shallowestFlushIfPossible. Or something like that.

[Brian Anderson]

Comment on attachment 152386 [details]
Patch

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

Rebased patch set incoming.

>> Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:57
>> +    void shallowFlushCHROMIUM()
> 
> What if the flush extension is not supported? I think since this code wraps a GC3D, we could make it probe the context for support of the extension and store that support as a member var. If its not supported, this could degrade to a flush, which I like more. Then you could make this method name just shallowestFlushIfPossible. Or something like that.

I considered degrading to a glFlush, but I think we want to stay away from the overhead of glFlushes if possible.  For example, this extension is not supported by WebGraphicsContext3DInProcessImpl.  Do we want to degrade to glFlush in that case?

I saw a comment somewhere that if an extension is not supported, calling it on a GC3D should not have any side effects.  I could check for it more explicitly though so that it is clear this extension isn't supported by all WGC3D's.

[Nat Duca]

(In reply to comment #6)
> (From update of attachment 152386 [details])
> 
> I considered degrading to a glFlush, but I think we want to stay away from the overhead of glFlushes if possible.  For example, this extension is not supported by WebGraphicsContext3DInProcessImpl.  Do we want to degrade to glFlush in that case?

That's what we're doing now and its a win. But, if you're really worried, then I'd rather that it check for support and not make the call, then have it make a call that inside is assumed to just noop.

You don't want to check for an extension every time you call this. So you should add a method to CCGraphicsContext that caches the value for you, maybe. E.g. supportsShallowFlush?

> I saw a comment somewhere that if an extension is not supported, calling it on a GC3D should not have any side effects.  I could check for it more explicitly though so that it is clear this extension isn't supported by all WGC3D's.

[Brian Anderson]

Created attachment 153165 [details]
Patch

[Brian Anderson]

Comment on attachment 153165 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:86
> +    void shallowFlushWithFallbackOnFlush()

Ended up going with the glFlush fallback but wanted to double check: Why do we want to glFlush() for the in-thread case but not the command buffer case?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:725
> +        m_context->initializeOnImplThread();

I was getting crashes if I didn't initialize on the impl thread.  Is there a better method to initialize the m_context on the impl thread?

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1723
> +                .WillByDefault(Return(WebString("")));

Is there a prettier way of doing this?  CCGraphicsContext initialization fails in this test otherwise.

[Nat Duca]

Comment on attachment 153165 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:54
> +    void initializeOnImplThread()

We do this already by calling makeCurrent. How about making the call in LRC to makeCurrent go through CCGraphicsContext instead of the GC3D. This can then do its one-off init code, and then call down to the wgc3d makecurrent.

>> Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:86
>> +    void shallowFlushWithFallbackOnFlush()
> 
> Ended up going with the glFlush fallback but wanted to double check: Why do we want to glFlush() for the in-thread case but not the command buffer case?

Well, when you put it that way, its a fair point. If we're doing in-process GL, the glFlush is I guess a noop, right? I hadn't really thought that through last night when I was giving you feedback, my apologies. The case I had in mind was downstream folks who use CC for their compositor, e.g. Blackberry used to use Chromium Compositor. For that sort of use case, they dont have all our extensions. When I was thinking about this the other day, i was thinking "gee, they'd still want the flush." But, now I'm sort of realizing that that isn't the case.

Suppose we make this shallowFlushIfNeeded, where if its not supported, its a nop as you originally (correctly) suggested.

Can land the chromium-side of this change first? If we land this side of the patch first, then we'd have a period of time where the compositor simply stops flushing on texture uploads. That's a complication we would be better off avoiding.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:725
>> +        m_context->initializeOnImplThread();
> 
> I was getting crashes if I didn't initialize on the impl thread.  Is there a better method to initialize the m_context on the impl thread?

There should be some code in this same function that makes the context current. Hook that if you can?

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1723
>> +                .WillByDefault(Return(WebString("")));
> 
> Is there a prettier way of doing this?  CCGraphicsContext initialization fails in this test otherwise.

This seems fine. I'm not sure if we have a best-known-method for this. I might just implement the function if I had tried it. I guess I have no opinion. :)

[Brian Anderson]

Comment on attachment 153165 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:54
>> +    void initializeOnImplThread()
> 
> We do this already by calling makeCurrent. How about making the call in LRC to makeCurrent go through CCGraphicsContext instead of the GC3D. This can then do its one-off init code, and then call down to the wgc3d makecurrent.

Looks like doing that is going to get ugly.  I see four options:
1) Use m_context->context3D() everywhere m_context is used in LRC (139 places).
2) Fill out the implementation of CCGraphicsContext.
3) Keep it as is.

1 and 2 are going to be more invasive than 3, but are also less error prone.

I'm going to go with 1.

[Brian Anderson]

Using CCGraphicsContext inside of the LRC is turned out to be more invasive than I thought.  I got it working, but I no longer think it's worth it just for CCGraphicsContext initialization - unless we want to use CCGraphicsContext inside the LRC in the future anyway.

Nat, are you okay with sticking to the initializeOnImplThread() as it is?

[Nat Duca]

(In reply to comment #12)
> Using CCGraphicsContext inside of the LRC is turned out to be more invasive than I thought.  I got it working, but I no longer think it's worth it just for CCGraphicsContext initialization - unless we want to use CCGraphicsContext inside the LRC in the future anyway.
> 
> Nat, are you okay with sticking to the initializeOnImplThread() as it is?


NO

[Brian Anderson]

Created attachment 153370 [details]
Patch

[Brian Anderson]

This patch is on top of the patch in bug 91793 and does initialization in the makeContextCurrent method of CCGraphicsContext.

I agree with waiting for the chromium patches to land first.

[Brian Anderson]

Created attachment 153619 [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.

[Brian Anderson]

(In reply to comment #17)
> 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.

Hmm, I think I may have hit a button I wasn't supposed to hit yet.  I was trying to get the sanity checks to run.  Assuming they're all green though, I think this patch is good to go after http://codereview.chromium.org/10782005 lands.

Note: Bug 91793 is no longer a dependency as there were other commits yesterday that simplified this patch.

[Kenneth Russell]

Comment on attachment 153619 [details]
Patch

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

Looks fine overall. One question. r=me; please mark cq? if the answer to the question above is "yes".

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:105
> +                resourceProvider->shallowFlushIfSupported();

Is the change from "flush" to "shallow flush" correct in all cases? In other words, the flushes before were only to "kick" the command processor, and weren't needed for correctness at the OpenGL level?

[Nat Duca]

Comment on attachment 153619 [details]
Patch

Looks great Brian.

[Kenneth Russell]

OK. Setting cq+ based on Nat's additional feedback.

[Brian Anderson]

Comment on attachment 153619 [details]
Patch

Thanks Ken.  I should have mentioned I was waiting for the chromium side of this patch to land before setting cq+.  It already landed earlier today, so we should be good.

[WebKit Review Bot]

Comment on attachment 153619 [details]
Patch

Clearing flags on attachment: 153619

Committed r123416: <http://trac.webkit.org/changeset/123416>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.