Bug 83649 - [chromium] Ensure RateLimiter waits for Swapbuffers completion
Summary: [chromium] Ensure RateLimiter waits for Swapbuffers completion
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Bauman
Depends on:
Reported: 2012-04-10 18:03 PDT by John Bauman
Modified: 2012-04-17 19:57 PDT (History)
8 users (show)

See Also:

Patch (12.53 KB, patch)
2012-04-10 18:59 PDT, John Bauman
no flags Details | Formatted Diff | Diff
Patch (12.55 KB, patch)
2012-04-11 14:16 PDT, John Bauman
no flags Details | Formatted Diff | Diff
Patch (12.74 KB, patch)
2012-04-12 17:53 PDT, John Bauman
no flags Details | Formatted Diff | Diff
Patch (12.91 KB, patch)
2012-04-17 18:38 PDT, John Bauman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Bauman 2012-04-10 18:03:29 PDT
[chromium] Ensure RateLimiter waits for Swapbuffers completion
Comment 1 John Bauman 2012-04-10 18:59:40 PDT
Created attachment 136604 [details]
Comment 2 John Bauman 2012-04-11 14:16:46 PDT
Created attachment 136752 [details]
Comment 3 James Robinson 2012-04-11 16:37:41 PDT
Comment on attachment 136752 [details]

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?
Comment 4 John Bauman 2012-04-11 16:55:56 PDT
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.
Comment 5 Nat Duca 2012-04-11 17:04:09 PDT
Comment on attachment 136752 [details]

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?
Comment 6 John Bauman 2012-04-11 17:10:40 PDT
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.
Comment 7 John Bauman 2012-04-12 17:53:34 PDT
Created attachment 137002 [details]
Comment 8 James Robinson 2012-04-12 18:28:34 PDT
Comment on attachment 137002 [details]

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


> 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)
Comment 9 John Bauman 2012-04-17 18:38:57 PDT
Created attachment 137646 [details]
Comment 10 WebKit Review Bot 2012-04-17 19:57:45 PDT
Comment on attachment 137646 [details]

Clearing flags on attachment: 137646

Committed r114475: <http://trac.webkit.org/changeset/114475>
Comment 11 WebKit Review Bot 2012-04-17 19:57:50 PDT
All reviewed patches have been landed.  Closing bug.