Bug 83649 – [chromium] Ensure RateLimiter waits for Swapbuffers completion

• Component: New Bugs
• Status: RESOLVED FIXED

[John Bauman]

[chromium] Ensure RateLimiter waits for Swapbuffers completion

[John Bauman]

Created attachment 136604 [details]
Patch

[John Bauman]

Created attachment 136752 [details]
Patch

[James Robinson]

Comment on attachment 136752 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        scheduling was causing the RateLimiter not to ratelimit enough. We
> +        need to insert no-op commands in the compositor context, so that we'll
> +        wait for the canvas context and SwapBuffers as well.

I don't really understand what the no-op command is for.  What's the failing case currently?  How does this help? Why are we handling this in the WebCore code instead of somewhere closer to the command buffer?

[John Bauman]

The thing that changed was that now the gpu process doesn't wait for the SwapBuffers to complete processing to process more commands on the canvas context. This means that instead of us putting in two canvas frames and waiting for the previous swapbuffers to finish, we can continually put in more canvas frames until the swapbuffers completes processing.

To avoid that, I'm putting a command in on the canvas context, which forces the SwapBuffers to complete before processing it. Then the timing of the canvas commands is the same as it had been before.

I'd put this code lower down, but the lower layers don't understand the threading and don't understand the relationship between the canvas context and compositor context.

[Nat Duca]

Comment on attachment 136752 [details]
Patch

So since this is releaseblock-stable, I think this makes sense. Do you think that it makes sense to open a post-M19 bug for fixing this in a cleaner way?

[John Bauman]

Definitely. This was intended to be more of a simple stopgap fix, to make it as good as it had been until we figure out what in the world we should do to make it better. That might involve very elaborate changes.

Also, Nat recommended renaming the no-op command to explain its purpose a bit better. Maybe something like "forceSerializeOnSwapBuffersComplete", although that might just be more confusing.

[John Bauman]

Created attachment 137002 [details]
Patch

[James Robinson]

Comment on attachment 137002 [details]
Patch

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

R=me

> Source/WebCore/platform/graphics/chromium/RateLimiter.h:54
> +    explicit RateLimiter(GraphicsContext3D*, RateLimiterClient*);

no explicit for 2-arg c'tors

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:219
> +    void rateLimit();

this is overriding RateLimitClient::rateLimit(), right? would you mind adding virtual and OVERRIDE keywords and documenting what it's overriding here explicitly? (with a short "// RateLimitClient implementation." comment or the like)

[John Bauman]

Created attachment 137646 [details]
Patch

[WebKit Review Bot]

Comment on attachment 137646 [details]
Patch

Clearing flags on attachment: 137646

Committed r114475: <http://trac.webkit.org/changeset/114475>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.