[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
Darin, Peter, I'm digging up this thread because the current Mac behavior turns out to be problematic. Cocoa sets up the proper clipping region before calling drawRect:. We can't just expand the invalid rect and paint wherever we want, since Cocoa will ignore any drawing outside of the original invalid rect. I've coded up another possibility: if we detect a recursive call to DidPaintRect(), defer the second paint until we return to the main loop and the first drawRect: is completed. This seems to work, but I'd like confirmation that it's ok to do the second paint asychronously. The code is up at http://codereview.chromium.org/335029/show . Does this look like something that will cause problems or race conditions? Thanks! Rohit On Mon, Apr 13, 2009 at 12:13 PM, Darin Fisher da...@chromium.org wrote: On Mon, Apr 13, 2009 at 8:43 AM, Avi Drissman a...@chromium.org wrote: I'm still a little lost in this discussion. I'm repeatedly reading the code and the patch, and I'll get back to you when I fully understand it. But what I wanted to say is that there is a significant difference in the paint pipeline as it currently exists on the Mac compared to Win/Lin. On Win, DidPaintRect() calls Redraw() which calls RedrawWindow(...RDW_UPDATENOW...) which (AFAIK) does a synchronous draw via Win32 callback to OnPaint(). On Lin, DidPaintRect() calls Paint() which blits via BackingStore::ShowRect(). Again, synchronously. On Mac, DidPaintRect() calls Redraw() which calls -[NSView setNeedsDisplayInRect:], setting the damaged rect in the view. The call returns, and sometime later, during the next pump of the message loop, Cocoa wakes up, decides to repaint all damaged windows, calls our -drawRect:, which then calls RenderWidgetHost::GetBackingStore(). The Mac implementation is wrong. On a single core machine, the renderer could starve the UI thread of the browser, preventing any pixels from ever being updated on the screen. You need to change the Mac implementation to flush to the screen when DidPaintRect is not called withing drawRect / GetBackingStore. Prior to Peter's change it was not possible for DidPaintRect to be called from within GetBackingStore. Now that is possible, so the consumer needs to take care. -Darin That gap prevents recursion. If for some reason GetBackingStore ends up causing a DidPaintRect() message, what will happen is: - Original DidPaintRect(), adds damage ... message loop ... - -drawRect: -- GetBackingStore() --- New DidPaintRect(), adds more damage -- blitting ... Cocoa clears the damage region, all of it Avi --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
This isn't just about message priorities. It is also about thread priorities. There is no point in having the renderer produces new images of the page if the browser process hasn't put them on the screen yet. -Darin On Mon, Apr 13, 2009 at 11:28 AM, Adam Langley a...@chromium.org wrote: On Mon, Apr 13, 2009 at 11:12 AM, Peter Kasting pkast...@google.com wrote: Perhaps not safe, but not worse. It's not that don't want to deal with this right now, it's that I believe that the current behaviour is correct on Linux. I've reread your patch and email and I haven't understood any arguments about why there's a problem. (I get that there might be something about the priority of Windows WM_PAINT messages that causes an issue, but that obviously doesn't apply with X.) AGL --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
On Mon, Apr 13, 2009 at 11:51 AM, Avi Drissman a...@chromium.org wrote: On Mon, Apr 13, 2009 at 2:47 PM, Peter Kasting pkast...@google.comwrote: it doesn't fire on Mac and at some point Avi will fix the Mac implementation. Side note: I now understand the issue this fixes and the fix to make the Mac impl compliant and to handle this one is item #2 on my todo list. I have now committed the patch here, along with Adam Langley's Linux fix, after the combination passed all three trybots. r13681. At this point I believe Avi's upcoming fix is the only remaining piece here. PK --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
I'm still a little lost in this discussion. I'm repeatedly reading the code and the patch, and I'll get back to you when I fully understand it. But what I wanted to say is that there is a significant difference in the paint pipeline as it currently exists on the Mac compared to Win/Lin. On Win, DidPaintRect() calls Redraw() which calls RedrawWindow(...RDW_UPDATENOW...) which (AFAIK) does a synchronous draw via Win32 callback to OnPaint(). On Lin, DidPaintRect() calls Paint() which blits via BackingStore::ShowRect(). Again, synchronously. On Mac, DidPaintRect() calls Redraw() which calls -[NSView setNeedsDisplayInRect:], setting the damaged rect in the view. The call returns, and sometime later, during the next pump of the message loop, Cocoa wakes up, decides to repaint all damaged windows, calls our -drawRect:, which then calls RenderWidgetHost::GetBackingStore(). That gap prevents recursion. If for some reason GetBackingStore ends up causing a DidPaintRect() message, what will happen is: - Original DidPaintRect(), adds damage ... message loop ... - -drawRect: -- GetBackingStore() --- New DidPaintRect(), adds more damage -- blitting ... Cocoa clears the damage region, all of it Avi --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
On Mon, Apr 13, 2009 at 8:43 AM, Avi Drissman a...@chromium.org wrote: I'm still a little lost in this discussion. I'm repeatedly reading the code and the patch, and I'll get back to you when I fully understand it. But what I wanted to say is that there is a significant difference in the paint pipeline as it currently exists on the Mac compared to Win/Lin. On Win, DidPaintRect() calls Redraw() which calls RedrawWindow(...RDW_UPDATENOW...) which (AFAIK) does a synchronous draw via Win32 callback to OnPaint(). On Lin, DidPaintRect() calls Paint() which blits via BackingStore::ShowRect(). Again, synchronously. On Mac, DidPaintRect() calls Redraw() which calls -[NSView setNeedsDisplayInRect:], setting the damaged rect in the view. The call returns, and sometime later, during the next pump of the message loop, Cocoa wakes up, decides to repaint all damaged windows, calls our -drawRect:, which then calls RenderWidgetHost::GetBackingStore(). The Mac implementation is wrong. On a single core machine, the renderer could starve the UI thread of the browser, preventing any pixels from ever being updated on the screen. You need to change the Mac implementation to flush to the screen when DidPaintRect is not called withing drawRect / GetBackingStore. Prior to Peter's change it was not possible for DidPaintRect to be called from within GetBackingStore. Now that is possible, so the consumer needs to take care. -Darin That gap prevents recursion. If for some reason GetBackingStore ends up causing a DidPaintRect() message, what will happen is: - Original DidPaintRect(), adds damage ... message loop ... - -drawRect: -- GetBackingStore() --- New DidPaintRect(), adds more damage -- blitting ... Cocoa clears the damage region, all of it Avi --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
On Mon, Apr 13, 2009 at 12:13 PM, Darin Fisher da...@chromium.org wrote: You need to change the Mac implementation to flush to the screen when DidPaintRect is not called withing drawRect / GetBackingStore. I'll put it on my list of things to do. (First is fixing a bug that is causing drawing to not work at all.) Research time, though, how to do a direct draw in Cocoa. Avi --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
On Mon, Apr 13, 2009 at 12:13 PM, Darin Fisher da...@chromium.org wrote: The Mac implementation is wrong. On a single core machine, the renderer could starve the UI thread of the browser, preventing any pixels from ever being updated on the screen. That seems wrong--the OS-prompted draw should be just blitting from the backing store, right? But I'm more concerned about this starvation problem--the renderer should not be able to starve the UI thread under any circumstances--we always need to return to the main runloop every second or two or the OS will decide we're unresponsive and beachball. What code path can cause this? --Amanda --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
On Mon, Apr 13, 2009 at 10:59 AM, Adam Langley a...@chromium.org wrote: On Fri, Apr 10, 2009 at 1:16 PM, Peter Kasting pkast...@google.com wrote: I strongly suggest picking Darin's brain directly for more detail as he has a stronger grasp of this than I do :) I don't want to block landing this patch because of Linux. However, I would currently just #ifdef in the old behaviour which I believe is correct for Linux. Certainly doing that is a safe. Perhaps not safe, but not worse. However, I'd rather wait to land than #ifdef. The old behavior is certainly wrong (did you not see my + Darin's emails above?), and ifdefs are ugly. There's nothing urgently pushing me to get this in in the next few hours. PK --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
On Mon, Apr 13, 2009 at 11:12 AM, Peter Kasting pkast...@google.com wrote: Perhaps not safe, but not worse. It's not that don't want to deal with this right now, it's that I believe that the current behaviour is correct on Linux. I've reread your patch and email and I haven't understood any arguments about why there's a problem. (I get that there might be something about the priority of Windows WM_PAINT messages that causes an issue, but that obviously doesn't apply with X.) AGL --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
On Mon, Apr 13, 2009 at 11:28 AM, Adam Langley a...@chromium.org wrote: It's not that don't want to deal with this right now, it's that I believe that the current behaviour is correct on Linux. Yes, I grasped that. I've reread your patch and email and I haven't understood any arguments about why there's a problem. (I get that there might be something about the priority of Windows WM_PAINT messages that causes an issue, but that obviously doesn't apply with X.) The reason I had to write a patch at all is a cross-platform issue, not a Windows-specific one. The Windows-specific stuff in the comments is merely an example of why an API like DidPaintRect() is useful in the first place, but we're not adding that API in this patch, just explaining what's going on. The issue I'm fixing is that if we get an updated rect from the renderer while DidPaintRect() is disabled, we don't repaint any extra damaged area in that rect until the next time it happens to get damaged, because we don't send anyone any notifications. This is because we were putting in an anti-recursion check in the model when instead it should have been in the view. Fixing this means that any platform that (correctly) synchrously paints from DidPaintRect() is now at risk of infinite recursion, and the new DCHECK is designed to detect this. It fires on Linux because Linux was doing the right thing before; it doesn't fire on Mac and at some point Avi will fix the Mac implementation. To fix _this_ issue the platform-specific code needs to have its own anti-recursion check, as Windows now has. This is what I'm asking Linux to write. You write one by detecting (inside DidPaintRect()) that you're already in a paint and not recursively painting again. PK --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
On Mon, Apr 13, 2009 at 11:47 AM, Peter Kasting pkast...@google.com wrote:
The issue I'm fixing is that if we get an updated rect from the renderer
while DidPaintRect() is disabled, we don't repaint any extra damaged area in
that rect until the next time it happens to get damaged, because we don't
send anyone any notifications. This is because we were putting in an
anti-recursion check in the model when instead it should have been in the
view.
I always thought that, when we paint without a backing store, we're
painting the whole thing so this isn't an issue.
After patching in your change, the following makes Linux work for me. Hopefully
I have the correct end of the stick this time.
Gmail will probably mangle this patch, so it's attached as well.
diff --git a/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
b/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
index 32135f9..eb1b966 100644
--- a/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
+++ b/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
@@ -159,6 +159,7 @@
RenderWidgetHostViewGtk::RenderWidgetHostViewGtk(RenderWidgetHost*
widget_host)
parent_(NULL),
popup_signal_id_(0),
activatable_(true),
+ about_to_validate_and_paint_(false),
is_loading_(false) {
host_-set_view(this);
}
@@ -277,7 +278,10 @@ void RenderWidgetHostViewGtk::IMEUpdateStatus(int control,
}
void RenderWidgetHostViewGtk::DidPaintRect(const gfx::Rect rect) {
- Paint(rect);
+ if (about_to_validate_and_paint_)
+invalid_rect_ = invalid_rect_.Union(rect);
+ else
+Paint(rect);
}
void RenderWidgetHostViewGtk::DidScrollRect(const gfx::Rect rect, int dx,
@@ -338,7 +342,13 @@ void
RenderWidgetHostViewGtk::PasteFromSelectionClipboard() {
}
void RenderWidgetHostViewGtk::Paint(const gfx::Rect damage_rect) {
+ DCHECK(!about_to_validate_and_paint_);
+
+ invalid_rect_ = damage_rect;
+ about_to_validate_and_paint_ = true;
BackingStore* backing_store = host_-GetBackingStore();
+ // Calling GetBackingStore maybe have changed |invalid_rect_|...
+ about_to_validate_and_paint_ = false;
GdkWindow* window = view_.get()-window;
if (backing_store) {
@@ -347,7 +357,7 @@ void RenderWidgetHostViewGtk::Paint(const
gfx::Rect damage_rect) {
// Destroy()ed yet and it receives paint messages...
if (window) {
backing_store-ShowRect(
- damage_rect, x11_util::GetX11WindowFromGtkWidget(view_.get()));
+ invalid_rect_, x11_util::GetX11WindowFromGtkWidget(view_.get()));
}
} else {
if (window)
diff --git a/chrome/browser/renderer_host/render_widget_host_view_gtk.h
b/chrome/browser/renderer_host/render_widget_host_view_gtk.h
index 215549d..cd49a8d 100644
--- a/chrome/browser/renderer_host/render_widget_host_view_gtk.h
+++ b/chrome/browser/renderer_host/render_widget_host_view_gtk.h
@@ -101,6 +101,12 @@ class RenderWidgetHostViewGtk : public
RenderWidgetHostView {
// activatable popup: select dropdown. Example of non-activatable popup:
// form autocomplete.
bool activatable_;
+ // This is true when we are currently painting and thus should handle extra
+ // paint requests by expanding the invalid rect rather than actually
+ // painting.
+ bool about_to_validate_and_paint_;
+ // This is the rectangle which we'll paint.
+ gfx::Rect invalid_rect_;
// Whether we are currently loading.
bool is_loading_;
--~--~-~--~~~---~--~~
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---
patch
Description: Binary data
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
On Fri, Apr 10, 2009 at 12:28 PM, Peter Kasting pkast...@google.com wrote: Please let me know if you understand this patch enough to make appropriate changes, and how to coordinate landing various bits so I don't break anyone. PK I patched this in and started fixing things and ended up writing the reverse of your patch. Myabe you want to #ifdef around your changes? I don't get why you want to do this. We have this situation: X sends an expose message to us We ask for a BackingStore We don't have one, so we start a timed wait on the renderer to send us a paint We get the paint, so we create the backing store and copy the bitmap to it. With your patch: We then call back into GTK to invalidate the rect again! Error! Your comment suggests that you want to do more than invalidate in DidPaintRect because of WM_PAINT starvation. I don't get how this helps you. In the case that you're talking about, we're dealing with a message from the renderer without the expose handling on the stack and then that guard that you removed doesn't even come into play. AGL --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
On Fri, Apr 10, 2009 at 1:07 PM, Adam Langley a...@chromium.org wrote: X sends an expose message to us We ask for a BackingStore We don't have one, so we start a timed wait on the renderer to send us a paint We get the paint, so we create the backing store and copy the bitmap to it. With your patch: We then call back into GTK to invalidate the rect again! Error! The question is, has GTK already validated the rect it sent us to paint? If so, we should just enlarge the rect that the paint function will redraw; if not, we should both do that and ensure that that larger rect gets validated. (On Windows, the case is the latter.) In neither case do we really want to come out of painting with a dirty rect from this. Your comment suggests that you want to do more than invalidate in DidPaintRect because of WM_PAINT starvation. I don't get how this helps you. In the case that you're talking about, we're dealing with a message from the renderer without the expose handling on the stack and then that guard that you removed doesn't even come into play. When you paint, the paint function will ask for the updated backing store, which potentially needs to go to the renderer and wait (if it is dirty). When it comes back, with the guards removed, you call DidPaintRect(), which calls Paint(), which can recurse because the backing store may still need updating (e.g. we got an ack of a previous resize but another is waiting, or similar). Darin came up with some thought experiments in which this recursion happens effectively infinitely. On the other hand, if you fix this by making DidPaintRect() not paint, then you'll invalidate the area, but potentially never get a paint callback unless your event system guarantees that you'll paint within a certain period no matter how much the renderer is eating the CPU and spamming you with updates. In short, DidPaintRect() painting rate-limits the renderer from starving paints, and being clever about how we paint and invalidate prevents infinite recursion and/or overpainting during this. I strongly suggest picking Darin's brain directly for more detail as he has a stronger grasp of this than I do :) PK --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
