Delay UpdateRect until the SwapBuffers callback when accelerated compositing is on.

Delay UpdateRect until the SwapBuffers callback when accelerated compositing is on.

This is also removing the UpdateRect ack in the accelerated compositing case,
because it is not needed (and adds scheduling constraints that reduce
throughput).

This also sends a "dummy" message to the browser to unblock the UI thread if it's waiting on an UpdateRect when the transition from non-accelerated to accelerated happens: the GPU process may need to round trip to the browser UI thread before sending the SwapBuffers callback.

BUG=58782
TEST=reduced jankiness on aura builds when resizing.


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

[jam, 2011-11-11 03:03:44]

I'm not familiar with this code, so I'm not a qualified reviewer. i did just
look through just for nits. please find another reviewer though (even if not
OWNER) and i can do owners lgtm later

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
content/renderer/render_widget.cc:106: while (!updates_pending_swap_.empty()) {
STLDeleteContainerPointers?

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
content/renderer/render_widget.cc:397: if (msg)
here and below, when would updates_pending_swap_ not have a null pointer?

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
File content/renderer/render_widget.h (right):

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
content/renderer/render_widget.h:67: struct ViewHostMsg_UpdateRect_Params;
nit: i thought the convention was to put this before the namespace forward
declarations?

[nduca, 2011-11-11 04:50:13]

Is there a way to not completely do-away with the ack part of things? I don't
like the idea of swamping the browser process with more updateRects than it can
handle.

What if we had something on RW like:

   sendUpdateRect(updatRect updates) {
     if (!ack_pending) {
        ack_pending = true;
        send;
        return;
     }
     pending_update_rect_message.merge(message);
     has_pending_updates = true;
   }

In the updateRectAck, we could say 
   if(has_pending_updates)
      send_again; has_pending_updates = false;

Does this make any sense? Or am I overcomplicating things?

[piman, 2011-11-11 06:28:00]

On Thu, Nov 10, 2011 at 8:50 PM, <nduca@chromium.org> wrote:

> Is there a way to not completely do-away with the ack part of things? I
> don't
> like the idea of swamping the browser process with more updateRects than
> it can
> handle.
>

Today already when accelerated compositing is on, the RenderWidgetHost
doesn't do anything significant before sending the ack. E.g. the plugin
moves happen after (and even then they're very cheap compared to the page
rendering if it does have plugins).


> What if we had something on RW like:
>
>   sendUpdateRect(updatRect updates) {
>     if (!ack_pending) {
>        ack_pending = true;
>        send;
>        return;
>     }
>     pending_update_rect_message.**merge(message);
>     has_pending_updates = true;
>   }
>
> In the updateRectAck, we could say
>   if(has_pending_updates)
>      send_again; has_pending_updates = false;
>
> Does this make any sense? Or am I overcomplicating things?
>

The main reason to remove the ack is that if we need to wait for it, then
it prevents the renderer from pipelining frames - with this patch we only
send the UpdateRect after the frame is finished, but we'd have to wait for
the ack before starting a new one.
I don't really like the idea of delaying the UpdateRects further, because
they're on the critical path for resizes.
We could count the pending acks, like we count the swaps, and allow up to 2
in flight and stop after that, but I think it adds complexity when there's
no evidence that the ack is necessary - I think the cases where we have to
wait for the ack have more to do with IPC latency (because the browser is
busy) rather than the cost of doing the update.



>
>
http://codereview.chromium.**org/8498036/<http://codereview.chromium.org/8498...
>

[nduca, 2011-11-11 06:31:58]

Sorry to be cantankerous, but I think you're being a bit too strong in your
assertion that no-ack is necessary. As I pointed out to you, nytimes.com has
8+ms updateRects on mac. Its there for a reason.

Also, your assertion that it does nothing isn't quite right. Yes, the first one
returns fast.  But then the *second* one doesn't ack because the browser is busy
working on the first.

Can you explain to me how this is on the critical path for resize? You've just
effectively slowed down the updateRect code by sending it later than it was
before --- before, the updateRect was sent when we composited, whereas now we
composite, wait 30ms for the swap ack, then we send the update rect. Isn't that
slower?

[nduca, 2011-11-15 01:57:32]

I think you've convinced me offline that we can try living without
updateRectAck's. So this looks mostly good to me.

Is your thinking that we'll prevent swamping the browser by virtue of the Gpu
process calling up to the browser process to swap? Is there a way we can
guarantee this (maybe as a followup bug) for the OS' where this isn't the case?
E.g. Linux, WinXP, CrOS [current].

http://codereview.chromium.org/8498036/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/8498036/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/render_message_filter.cc:347:
IPC_MESSAGE_HANDLER_GENERIC(ViewHostMsg_UpdateIsDelayed,
Can you route this to a dedicated function and then call DidReceiveUpdateMsg
directly? Might be more clean to read

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
File content/renderer/render_widget.cc (left):

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
content/renderer/render_widget.cc:845: // rects to save the browser process from
doing unecessary work.
Might want to preserve this comment up by the //begin painting comment

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
content/renderer/render_widget.cc:878: if (pending_update_params_.get()) {
Can we just put this in the non-composited half of the if above? I think the
comment about "composite calls onSwapBuffersPosted" fits better into the
composite() call...

[darin (slow to review), 2011-11-15 08:06:04]

http://codereview.chromium.org/8498036/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/8498036/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host.cc:1051: // ACK early so we can
prefetch the next PaintRect if there is a next one.
nit: indentation

http://codereview.chromium.org/8498036/diff/2001/content/common/view_message_...
File content/common/view_message_enums.h (right):

http://codereview.chromium.org/8498036/diff/2001/content/common/view_message_...
content/common/view_message_enums.h:117: static bool needs_ack(int flags) {
it seems a bit awkward to put this flag here.  the above three bits are
indicating that a ViewHostMsg_UpdateRect IPC is in response to a Resize, Restore
or Repaint request made by the browser.  here, needs_ack is saying whether an
UpdateRect_ACK message should be sent in reply to the UpdateRect message.  this
ack has nothing to do with the other acks.

this perhaps suggests that needs_ack should really be part of the
UpdateRect_Params structure as a boolean member.

counterargument:  this member of UpdateRect_Params is named "flags", which is a
very generic sounding field.  that naming seems to welcome putting other bits in
as flags as you have done.  so perhaps i'm a bit torn.  it took me a while to
mentally separate the IS_{RESIZE,RESTORE,REPAINT}_ACK bits from the NEEDS_ACK
bits.  they are completely unrelated and yet look like they must be related :-(

[piman, 2011-11-16 01:00:55]

http://codereview.chromium.org/8498036/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/8498036/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/render_message_filter.cc:347:
IPC_MESSAGE_HANDLER_GENERIC(ViewHostMsg_UpdateIsDelayed,
On 2011/11/15 01:57:32, nduca wrote:
> Can you route this to a dedicated function and then call DidReceiveUpdateMsg
> directly? Might be more clean to read

Done. (I'm not convinced it helps all that much, though I added a comment in
that function explaining why we have this).

http://codereview.chromium.org/8498036/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/8498036/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host.cc:1051: // ACK early so we can
prefetch the next PaintRect if there is a next one.
On 2011/11/15 08:06:04, darin wrote:
> nit: indentation

Done.

http://codereview.chromium.org/8498036/diff/2001/content/common/view_message_...
File content/common/view_message_enums.h (right):

http://codereview.chromium.org/8498036/diff/2001/content/common/view_message_...
content/common/view_message_enums.h:117: static bool needs_ack(int flags) {
On 2011/11/15 08:06:04, darin wrote:
> it seems a bit awkward to put this flag here.  the above three bits are
> indicating that a ViewHostMsg_UpdateRect IPC is in response to a Resize,
Restore
> or Repaint request made by the browser.  here, needs_ack is saying whether an
> UpdateRect_ACK message should be sent in reply to the UpdateRect message. 
this
> ack has nothing to do with the other acks.
> 
> this perhaps suggests that needs_ack should really be part of the
> UpdateRect_Params structure as a boolean member.
> 
> counterargument:  this member of UpdateRect_Params is named "flags", which is
a
> very generic sounding field.  that naming seems to welcome putting other bits
in
> as flags as you have done.  so perhaps i'm a bit torn.  it took me a while to
> mentally separate the IS_{RESIZE,RESTORE,REPAINT}_ACK bits from the NEEDS_ACK
> bits.  they are completely unrelated and yet look like they must be related
:-(

Done.

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
File content/renderer/render_widget.cc (left):

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
content/renderer/render_widget.cc:845: // rects to save the browser process from
doing unecessary work.
On 2011/11/15 01:57:32, nduca wrote:
> Might want to preserve this comment up by the //begin painting comment

Done.

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
content/renderer/render_widget.cc:106: while (!updates_pending_swap_.empty()) {
On 2011/11/11 03:03:45, John Abd-El-Malek wrote:
> STLDeleteContainerPointers?

std::queue doesn't have iterators. I changed to a std::deque, and used
STLDeleteElements.

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
content/renderer/render_widget.cc:397: if (msg)
On 2011/11/11 03:03:45, John Abd-El-Malek wrote:
> here and below, when would updates_pending_swap_ not have a null pointer?

Added a comment.

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
content/renderer/render_widget.cc:878: if (pending_update_params_.get()) {
On 2011/11/15 01:57:32, nduca wrote:
> Can we just put this in the non-composited half of the if above? I think the
> comment about "composite calls onSwapBuffersPosted" fits better into the
> composite() call...

When we lose the context, composite() doesn't call swapbuffers. I want to make
sure we're not missing an UpdateRect, so I'm following an ownership pattern:
- pending_update_params_ is created meaning a message needs to be sent
- if we go through the OnSwapBuffersPosted, the swapbuffers calback mechanism
takes ownership of it, and will send the message either when the callback comes,
or when we lost the context.
- if we get here with pending_update_params_ not reset for any reason (software,
composite failed or didn't swap for any reason), that means we need to send it
now.

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
File content/renderer/render_widget.h (right):

http://codereview.chromium.org/8498036/diff/2001/content/renderer/render_widg...
content/renderer/render_widget.h:67: struct ViewHostMsg_UpdateRect_Params;
On 2011/11/11 03:03:45, John Abd-El-Malek wrote:
> nit: i thought the convention was to put this before the namespace forward
> declarations?

Done.

[piman, 2011-12-02 02:32:10]

Resurrecting this CL (ping!).
I added a mechanism to send a round-trip to the browser UI thread on Swap, so
that we can throttle if it's ever busy with plugin moves.

On linux at least, in case the UI thread is not busy, the round trip doesn't
seem to cost more than .1 ms.

[nduca, 2011-12-02 05:13:21]

This is awesome. LGTM for the scheduling/flow control stuff, with the question:
do all OS' have ImageTransportSurfaces? Or is this for OS' that transport their
image to the UI? I was thinking I'd see the swapped-stuff get put on the
GpuScheduler/Gles2CmdDecoder chain...

Maybe +kbr to review those bits.

[piman, 2011-12-02 08:29:11]

On Thu, Dec 1, 2011 at 9:13 PM, <nduca@chromium.org> wrote:

> This is awesome. LGTM for the scheduling/flow control stuff, with the
> question:
> do all OS' have ImageTransportSurfaces?


With Al's recent change, all OS have an ImageTransportSurface. Mac and Aura
already had it, now, Linux and Windows have it too,
through the PassThroughImageTransportSurface which I modified to also
round-trip.
I'm thinking also this is going to be useful when we want to send the
UpdateRect message from the GPU process without having to go back to the
renderer.


> Or is this for OS' that transport their
> image to the UI? I was thinking I'd see the swapped-stuff get put on the
> GpuScheduler/Gles2CmdDecoder chain...
>
> Maybe +kbr to review those bits.
>
>
http://codereview.chromium.**org/8498036/<http://codereview.chromium.org/8498...
>

[piman, 2011-12-02 08:31:13]

+kbr per Nat's suggestion and apatrick because I'm touching code he recently
added.

[nduca, 2011-12-02 16:27:17]

> With Al's recent change, all OS have an ImageTransportSurface. 
Awesome, didn't catch that. That's elegant.

> I'm thinking also this is going to be useful when we want to send the
> UpdateRect message from the GPU process without having to go back to the
> renderer.
Agreed!

[Ken Russell (switch to Gerrit), 2011-12-02 21:33:37]

LGTM as far as I can tell from a code review. I'm not sure how big the semantic
changes are but please test composited and non-composited content on the
platforms where behavior could be affected.

http://codereview.chromium.org/8498036/diff/27001/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/8498036/diff/27001/content/renderer/render_wid...
content/renderer/render_widget.cc:976: Send(new
ViewHostMsg_UpdateIsDelayed(routing_id_));
OK, but if you're about to send the DidActivateAcceleratedCompositing message
below then why not just unblock the browser's UI thread if the browser receives
one of those messages with is_accelerated_compositing_active_ == true?

[apatrick_chromium, 2011-12-05 19:34:09]

LGTM if it's tested on all platforms. The PbufferImageTransportSurface and
AcceleratedSurface is currently disabled on Windows until I fix the DeferToFence
mechanism. See the TODO on line 171-ish of image_transport_surface_win.cc.

[piman, 2011-12-05 20:51:16]

I tested extensively on linux and aura, but I will need your help to test on
windows and mac.

http://codereview.chromium.org/8498036/diff/27001/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/8498036/diff/27001/content/renderer/render_wid...
content/renderer/render_widget.cc:976: Send(new
ViewHostMsg_UpdateIsDelayed(routing_id_));
On 2011/12/02 21:33:37, kbr wrote:
> OK, but if you're about to send the DidActivateAcceleratedCompositing message
> below then why not just unblock the browser's UI thread if the browser
receives
> one of those messages with is_accelerated_compositing_active_ == true?

I don't want to re-enter the handling of the DidActivateAcceleratedCompositing
message from the place where it's waiting for the UpdateRect message - the side
effects change the window hierarchy, which is a bad idea from a views tree walk
(aura, windows).
I could post a task to avoid re-entering, but then I have to worry about
ordering wrt UpdateRect.
So, sending a message that just means "stop waiting, you won't get the message
you're waiting for" seems safer.

To solve the problem in a nice way, we need to:
- fold the accelerated compositing flag into UpdateRect
- send the UpdateRect from the GPU process when in accelerated mode
- properly handle the re-enter.
I want to get to that, but it's beyond the scope of this CL.

[Ken Russell (switch to Gerrit), 2011-12-06 03:45:30]

On 2011/12/05 20:51:16, piman wrote:
> I tested extensively on linux and aura, but I will need your help to test on
> windows and mac.

I've built TOT Chromium with your patch applied and see a regression in the
following scenario:

1. Visit http://www.khronos.org/webgl/wiki/Demo_Repository
2. Click the san-angeles demo link
3. After the demo loads, click the back buffer

Now the browser window no longer updates. For example, try clicking the link
again; the omnibox contains the demo URL but the demo does not display. Resizing
the browser window, etc. does not clear up the problem.

Can you reproduce this in your configuration or does it sound to you like this
is a Mac-specific regression? I don't see this behavior with the current
Chromium continuous build.

[piman, 2011-12-06 04:20:38]

On Mon, Dec 5, 2011 at 7:45 PM, <kbr@chromium.org> wrote:

> On 2011/12/05 20:51:16, piman wrote:
>
>> I tested extensively on linux and aura, but I will need your help to test
>> on
>> windows and mac.
>>
>
> I've built TOT Chromium with your patch applied and see a regression in the
> following scenario:
>
> 1. Visit
http://www.khronos.org/webgl/**wiki/Demo_Repository<http://www.khronos.org/we...
> 2. Click the san-angeles demo link
> 3. After the demo loads, click the back buffer
>
> Now the browser window no longer updates. For example, try clicking the
> link
> again; the omnibox contains the demo URL but the demo does not display.
> Resizing
> the browser window, etc. does not clear up the problem.
>
> Can you reproduce this in your configuration or does it sound to you like
> this
> is a Mac-specific regression? I don't see this behavior with the current
> Chromium continuous build.
>

Thanks for your help.

Don't see that on linux nor linux/aura.

It's probably a Mac-specific scheduling issue. I suspect either the UI
thinks the web content is still in composited mode when it's not, or that a
race between the DidActivateAcceleratedCompositing(false) message and the
last SwapBuffers causes the latter to not be ACK'd by the browser process
(delaying the UpdateRect indefinitely). I will take a look to see if I can
figure it out.

Antoine


>
>
http://codereview.chromium.**org/8498036/<http://codereview.chromium.org/8498...
>

[piman, 2011-12-06 04:46:17]

On Mon, Dec 5, 2011 at 8:20 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Mon, Dec 5, 2011 at 7:45 PM, <kbr@chromium.org> wrote:
>
>> On 2011/12/05 20:51:16, piman wrote:
>>
>>> I tested extensively on linux and aura, but I will need your help to
>>> test on
>>> windows and mac.
>>>
>>
>> I've built TOT Chromium with your patch applied and see a regression in
>> the
>> following scenario:
>>
>> 1. Visit
http://www.khronos.org/webgl/**wiki/Demo_Repository<http://www.khronos.org/we...
>> 2. Click the san-angeles demo link
>> 3. After the demo loads, click the back buffer
>>
>> Now the browser window no longer updates. For example, try clicking the
>> link
>> again; the omnibox contains the demo URL but the demo does not display.
>> Resizing
>> the browser window, etc. does not clear up the problem.
>>
>> Can you reproduce this in your configuration or does it sound to you like
>> this
>> is a Mac-specific regression? I don't see this behavior with the current
>> Chromium continuous build.
>>
>
> Thanks for your help.
>
> Don't see that on linux nor linux/aura.
>
> It's probably a Mac-specific scheduling issue. I suspect either the UI
> thinks the web content is still in composited mode when it's not, or that a
> race between the DidActivateAcceleratedCompositing(false) message and the
> last SwapBuffers causes the latter to not be ACK'd by the browser process
> (delaying the UpdateRect indefinitely). I will take a look to see if I can
> figure it out.
>

NVM, it started reproducing on linux/aura after a rebase.

Antoine


>
> Antoine
>
>
>>
>>
http://codereview.chromium.**org/8498036/<http://codereview.chromium.org/8498...
>>
>
>

[piman, 2011-12-08 01:57:25]

PTAL, problem was that the RenderWidgetHelper only allowed a single UpdateRect
per renderer in flight.

[Ken Russell (switch to Gerrit), 2011-12-09 03:12:39]

Sorry for the delay. LGTM again as far as I can tell from code review. Sorry, I
wasn't able to re-test this on Mac yet, and it will be a while before I'm able
to, so you should go ahead.

[darin (slow to review), 2011-12-09 17:50:02]

LGTM

[commit-bot: I haz the power, 2011-12-09 17:52:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/8498036/36001

[commit-bot: I haz the power, 2011-12-09 22:19:26]

Change committed as 113861