Desynchronize windowless plugin painting. This greatly improves the painting...

Desynchronize windowless plugin painting.  This greatly improves the painting
performance when there are multiple plugins, or when scrolling.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=2459

[ananta, 2008-09-19 02:32:55]

Looks good overall. Comments below:-

http://codereview.chromium.org/3133/diff/26/212
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/3133/diff/26/212#newcode11
Line 11: #include "base/gfx/platform_device_win.h"
Please fix the include order.

http://codereview.chromium.org/3133/diff/26/212#newcode22
Line 22: #define kWindowlessPaintFPS 30
Can we make this a const?

http://codereview.chromium.org/3133/diff/26/213
File chrome/plugin/webplugin_proxy.h (right):

http://codereview.chromium.org/3133/diff/26/213#newcode90
Line 90: 
Please add some comments for these functions.

http://codereview.chromium.org/3133/diff/26/216
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/3133/diff/26/216#newcode325
Line 325: // Copied from render_widget.cc
Would it be possible to move this to a common place and have both files use one
implementation?

http://codereview.chromium.org/3133/diff/26/216#newcode341
Line 341: HANDLE handle = NULL;
nit. Perhaps windowless_buffer_handle may be a better name?

http://codereview.chromium.org/3133/diff/26/216#newcode344
Line 344: plugin_rect_.y() != window_rect.y();
It seems that the moved condition will never be true.
The plugin_rect_ = window_rect statement should be moved down?

http://codereview.chromium.org/3133/diff/26/216#newcode347
Line 347: window_rect.height() != windowless_canvas_->getDevice()->height())) {
Space missing.

http://codereview.chromium.org/3133/diff/26/216#newcode348
Line 348: // Create a shared memory section that the plugin paints
asynchronously.
The code to setup the canvas could be in its own function. This function seems
to be getting a little long.

http://codereview.chromium.org/3133/diff/26/216#newcode370
Line 370: static_cast<SkScalar>(-plugin_rect_.y()));
The translate call is common for both cases.

http://codereview.chromium.org/3133/diff/26/216#newcode418
Line 418: new_hdc, rect.x(), rect.y(), rect.width(), rect.height(), m_bf);
A comment or two explaining what we are doing here would be helpful.

[jam, 2008-09-19 21:24:39]

http://codereview.chromium.org/3133/diff/26/212
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/3133/diff/26/212#newcode11
Line 11: #include "base/gfx/platform_device_win.h"
On 2008/09/19 02:32:55, ananta wrote:
> Please fix the include order.
> 

Done (I prefer putting subdirectories after files, but I see  this isn't
common).

http://codereview.chromium.org/3133/diff/26/212#newcode22
Line 22: #define kWindowlessPaintFPS 30
On 2008/09/19 02:32:55, ananta wrote:
> Can we make this a const?

Done.

http://codereview.chromium.org/3133/diff/26/213
File chrome/plugin/webplugin_proxy.h (right):

http://codereview.chromium.org/3133/diff/26/213#newcode90
Line 90: 
On 2008/09/19 02:32:55, ananta wrote:
> Please add some comments for these functions.
> 

Done.

http://codereview.chromium.org/3133/diff/26/216
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/3133/diff/26/216#newcode325
Line 325: // Copied from render_widget.cc
On 2008/09/19 02:32:55, ananta wrote:
> Would it be possible to move this to a common place and have both files use
one
> implementation?

Since it's just a trivial one liner, I didn't think it added much benefit..

http://codereview.chromium.org/3133/diff/26/216#newcode341
Line 341: HANDLE handle = NULL;
On 2008/09/19 02:32:55, ananta wrote:
> nit. Perhaps windowless_buffer_handle may be a better name?

Done.

http://codereview.chromium.org/3133/diff/26/216#newcode344
Line 344: plugin_rect_.y() != window_rect.y();
On 2008/09/19 02:32:55, ananta wrote:
> It seems that the moved condition will never be true.
> The plugin_rect_ = window_rect statement should be moved down?

oops, good catch, I was reorganizing the code and did this incorrectly.

http://codereview.chromium.org/3133/diff/26/216#newcode347
Line 347: window_rect.height() != windowless_canvas_->getDevice()->height())) {
On 2008/09/19 02:32:55, ananta wrote:
> Space missing.

Done.

http://codereview.chromium.org/3133/diff/26/216#newcode370
Line 370: static_cast<SkScalar>(-plugin_rect_.y()));
On 2008/09/19 02:32:55, ananta wrote:
> The translate call is common for both cases.

I didn't see value in factoring out what is already a function call..

http://codereview.chromium.org/3133/diff/26/216#newcode418
Line 418: new_hdc, rect.x(), rect.y(), rect.width(), rect.height(), m_bf);
On 2008/09/19 02:32:55, ananta wrote:
> A comment or two explaining what we are doing here would be helpful.

I don't think there's anything tricky in this code that necessitated a comment. 
What were you thinking would be good to comment, that looking at the code
wouldn't explain?

[brettw, 2008-09-19 22:52:11]

http://codereview.chromium.org/3133/diff/33/232
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/3133/diff/33/232#newcode67
Line 67: // and find that there's no invalidates pending?
If we're always checking for invalidates even if we're not sure that anything
has changed, You don't have to fix it in this patch, but I'd say "soon."

http://codereview.chromium.org/3133/diff/33/232#newcode246
Line 246: DWORD wait_result = WaitForSingleObject(windowless_buffer_lock_,
INFINITE);
This means that the renderer could make a plugin infinitely hang, which could in
turn hang other renderers, right? I don't have any brilliant ideas on how to get
around this. The hung plugin dialog should eventually break this chain, right?

http://codereview.chromium.org/3133/diff/33/232#newcode333
Line 333: xf.eDx = (FLOAT)-delegate_->rect().x();
Can you use static_cast<float> here?

http://codereview.chromium.org/3133/diff/33/233
File chrome/plugin/webplugin_proxy.h (right):

http://codereview.chromium.org/3133/diff/33/233#newcode107
Line 107: // Used to desynchronize windowless painting.
I don't see a good description of desynchronized windowless painting anywhere,
and these variables are a little mysterious. Can you put a description of what
goes on here (or maybe in a more appropriate place if you can think of and
reference it here)? It should be clear what the different members are here.

http://codereview.chromium.org/3133/diff/33/236
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/3133/diff/33/236#newcode365
Line 365:
windowless_canvas_->getTopPlatformDevice().fixupAlphaBeforeCompositing();
Are you sure you need this call? It's not clear to me why it should be here.

http://codereview.chromium.org/3133/diff/33/236#newcode410
Line 410: BLENDFUNCTION	m_bf;
Is that a tab in the middle there?

http://codereview.chromium.org/3133/diff/33/236#newcode417
Line 417: AlphaBlend(hdc, rect.x(), rect.y(), rect.width(), rect.height(),
I have a suggestion for how this could be cleaner, although I don't think it
will actually be faster (and you don't have to do it right now).

WebPluginDelegateProxy::Paint can just take an SkCanvas instead of an HDC. Then
use the Skia functions for painting bitmaps to do the blending, since both the
source and the dest are already SkCanvases.

http://codereview.chromium.org/3133/diff/33/237
File chrome/renderer/webplugin_delegate_proxy.h (right):

http://codereview.chromium.org/3133/diff/33/237#newcode155
Line 155: // Used for desynchronized windowless painting.
Can you reference either webplugin_proxy.h or whereever you put a description of
windowless painting?

[jam, 2008-09-19 23:37:04]

http://codereview.chromium.org/3133/diff/33/232
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/3133/diff/33/232#newcode67
Line 67: // and find that there's no invalidates pending?
On 2008/09/19 22:52:11, brettw wrote:
> If we're always checking for invalidates even if we're not sure that anything
> has changed, You don't have to fix it in this patch, but I'd say "soon."

All that happens is that we check if a rectangle is empty, and if so, return
early (see OnPaintTimerFired below).  Do you think that's too heavy weight?

http://codereview.chromium.org/3133/diff/33/232#newcode246
Line 246: DWORD wait_result = WaitForSingleObject(windowless_buffer_lock_,
INFINITE);
It can't infinitely hang because we have the renderer and plugin hang detector. 
I've give this some thought but I don't think it's important to guard against,
since if someone wanted to hang the renderer, it would be much easier to write a
flash file that busy loops than figure out how to hack a renderer and then cause
it to embed a windowless plugin and block this call.

http://codereview.chromium.org/3133/diff/33/232#newcode333
Line 333: xf.eDx = (FLOAT)-delegate_->rect().x();
On 2008/09/19 22:52:11, brettw wrote:
> Can you use static_cast<float> here?

Done.

http://codereview.chromium.org/3133/diff/33/233
File chrome/plugin/webplugin_proxy.h (right):

http://codereview.chromium.org/3133/diff/33/233#newcode107
Line 107: // Used to desynchronize windowless painting.
On 2008/09/19 22:52:11, brettw wrote:
> I don't see a good description of desynchronized windowless painting anywhere,
> and these variables are a little mysterious. Can you put a description of what
> goes on here (or maybe in a more appropriate place if you can think of and
> reference it here)? It should be clear what the different members are here.

Added comments.

http://codereview.chromium.org/3133/diff/33/236
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/3133/diff/33/236#newcode365
Line 365:
windowless_canvas_->getTopPlatformDevice().fixupAlphaBeforeCompositing();
On 2008/09/19 22:52:11, brettw wrote:
> Are you sure you need this call? It's not clear to me why it should be here.

without doing this, all the pixel values are set to the magic transparency value
from this code in bitmap_platform_device_win.cc:

sk_memset32(static_cast<uint32_t*>(data), kMagicTransparencyColor, width *
height);

http://codereview.chromium.org/3133/diff/33/236#newcode410
Line 410: BLENDFUNCTION	m_bf;
On 2008/09/19 22:52:11, brettw wrote:
> Is that a tab in the middle there?

ahh, that's what the funny character is for..  Fixed.

http://codereview.chromium.org/3133/diff/33/236#newcode417
Line 417: AlphaBlend(hdc, rect.x(), rect.y(), rect.width(), rect.height(),
On 2008/09/19 22:52:11, brettw wrote:
> I have a suggestion for how this could be cleaner, although I don't think it
> will actually be faster (and you don't have to do it right now).
> 
> WebPluginDelegateProxy::Paint can just take an SkCanvas instead of an HDC.
Then
> use the Skia functions for painting bitmaps to do the blending, since both the
> source and the dest are already SkCanvases.
> 

Interesting.  WebPluginImpl::paint is what gets called by webkit, and and at
that point it gets a WebCore::GraphicsContext.  That can be casted somehow to
get an SKCanvas?

http://codereview.chromium.org/3133/diff/33/237
File chrome/renderer/webplugin_delegate_proxy.h (right):

http://codereview.chromium.org/3133/diff/33/237#newcode155
Line 155: // Used for desynchronized windowless painting.
On 2008/09/19 22:52:11, brettw wrote:
> Can you reference either webplugin_proxy.h or whereever you put a description
of
> windowless painting?

Done.

[brettw, 2008-09-22 20:06:55]

http://codereview.chromium.org/3133/diff/33/232
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/3133/diff/33/232#newcode67
Line 67: // and find that there's no invalidates pending?
I'm concerned about spinning the CPU in a renderer that might not otherwise be
doing any processing, which might keep it from being swapped out or something.

http://codereview.chromium.org/3133/diff/33/232#newcode246
Line 246: DWORD wait_result = WaitForSingleObject(windowless_buffer_lock_,
INFINITE);
Sounds fine.

http://codereview.chromium.org/3133/diff/33/236
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/3133/diff/33/236#newcode365
Line 365:
windowless_canvas_->getTopPlatformDevice().fixupAlphaBeforeCompositing();
I would use
"windowless_canvas_->getTopPlatformDevice()->accessBitmap->eraseARGB(0, 0, 0,
0);" instead. It will have the same effect (filling with all 0's) but is more
logically correct in this situation (and is faster).

http://codereview.chromium.org/3133/diff/33/236#newcode417
Line 417: AlphaBlend(hdc, rect.x(), rect.y(), rect.width(), rect.height(),
Yes, just call graphics_context->platformContext()->canvas().

[ananta, 2008-09-22 20:08:12]

LGTM with one nit

-Ananta

http://codereview.chromium.org/3133/diff/44/243
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/3133/diff/44/243#newcode14
Line 14: #include "chrome/common/gfx/chrome_canvas.h"
Please fix the include order

[jam, 2008-09-22 20:34:28]

http://codereview.chromium.org/3133/diff/33/232
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/3133/diff/33/232#newcode67
Line 67: // and find that there's no invalidates pending?
On 2008/09/22 20:06:56, brettw wrote:
> I'm concerned about spinning the CPU in a renderer that might not otherwise be
> doing any processing, which might keep it from being swapped out or something.

btw This code runs in the plugin process, but thanks for the pushback, it turned
out to be simple to add this.

http://codereview.chromium.org/3133/diff/33/236
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/3133/diff/33/236#newcode365
Line 365:
windowless_canvas_->getTopPlatformDevice().fixupAlphaBeforeCompositing();
On 2008/09/22 20:06:56, brettw wrote:
> I would use
> "windowless_canvas_->getTopPlatformDevice()->accessBitmap->eraseARGB(0, 0, 0,
> 0);" instead. It will have the same effect (filling with all 0's) but is more
> logically correct in this situation (and is faster).

Done.

http://codereview.chromium.org/3133/diff/33/236#newcode417
Line 417: AlphaBlend(hdc, rect.x(), rect.y(), rect.width(), rect.height(),
per our discussion, let's leave this to a separate change since it's unrelated.

[brettw, 2008-09-22 20:45:49]

LGTM