333250 - slow-path GTK2 native theme rendering is used too often

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I see the slow-path GTK2 native theme rendering used a *lot*, and I suspect it's part of the cause of the cairo builds being extremely slow for me (and even locking up other apps on my system).

The reason that the slow path is used is that nsNativeThemeGTK::DrawWidgetBackground does not put gfxXlibNativeRenderer::DRAW_SUPPORTS_CLIP_LIST in rendererFlags, so _draw_with_xlib_direct returns false because _get_rectangular_clip returns false.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So you're seeing a lot of cases where the clip region is more than one rectangle?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yep.  Maybe somewhere around a quarter of the calls -- I don't remember exactly.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Then I guess we'll have to whack gtk2drawing.c to handle clip regions.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We can't just whack all 74 relevant style GCs with a new clip region every time we draw, that would kill us.

We can paint a region by painting one rectangle at a time. Unfortunately Clearlooks doesn't actually honor the cliprect in many cases, so this creates some nasty visual bugs with that common theme.

The way this works pre-cairo is that we do all drawing to our rectangular backbuffer and then apply clipping while copying the backbuffer to the window, which clips out the illegal theme drawing. It does mean that CSS clipping, absent a widget, doesn't work on some themes' widgets, but that hasn't really hurt us.

Emulating that here with cairo is pretty easy. All we have to do is ResetClip() after creating the backbuffer with PushGroup(). This means we lose the ability for cairo to optimize when the region to draw is smaller than its bounding box, but that's OK I guess compared to these theme issues.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #4)
> Emulating that here with cairo is pretty easy. All we have to do is ResetClip()
> after creating the backbuffer with PushGroup(). This means we lose the ability
> for cairo to optimize when the region to draw is smaller than its bounding box,
> but that's OK I guess compared to these theme issues.

BTW, this is safe because PopGroupToSource restores the clip region and then Paint uses it, so drawing outside the clip region is definitely clipped out. It puts us on the fast path because ResetClip() means that most of the time when we're drawing chrome widgets, there isn't going to be a clip region around.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix this bug and also bug 333249

I fixed both of these bugs at once and the fixes conflict so here they are in one patch.

The fix for this bug is just to disable cliprect support in GTK2 themes completely, but add ResetClip() on the backbuffer so most chrome doesn't ever see a clip region.

The fix for bug 333249 is to lazily cache a GdkPixmap for the backbuffer's drawable. A more general fix here would be nice but would be quite difficult, the main problem being we have no easy way to know that a Drawable we see today is in fact the same pixmap as the same value Drawable we saw yesterday ... with the related problem that we could easily delete Drawables while their underlying GdkPixmaps are still around.

With this patch we seem to hit the draw-with-xlib fast path every time for FF chrome, with only one pixmap_foreign_new_for_display roundtrip per paint ... which isn't great, but may be the best we can do, short of modifying nsWindow to not use PushGroup for the backbuffer and create a pixmap explicitly with Gdk instead (assuming that doesn't require a roundtrip, which I'm not sure about).

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #4)
> The way this works pre-cairo is that we do all drawing to our rectangular
> backbuffer and then apply clipping while copying the backbuffer to the
> window, which clips out the illegal theme drawing. It does mean that CSS
> clipping, absent a widget, doesn't work on some themes' widgets, but that
> hasn't really hurt us.
> 
> Emulating that here with cairo is pretty easy. All we have to do is
> ResetClip() after creating the backbuffer with PushGroup().

I should add that this fix doesn't emulate the problems that the old code had with CSS clipping ... that will still happen, and trigger slow-path fallback, that will ensure we clip correctly by going through a temporary pixmap.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Note that if we need to, we can easily have gdk create the backbuffer pixmap -- there's no real reason to use PushGroup/PopGroup for double buffering, it's just convenience -- as long as we only need a RGB24 backbuffer.  If we ever figure out how to do transparent windows with ARGB visuals, we'll need to create an argb pixmap from cairo, but that won't be the common case.  We just need to create a gdk pixmap, wrap it in a gfxXlibSurface, set a device offset on it, and create a new gfxContext for the nsIRenderingContext.  If this avoids the extra sync x call, it's probably worth it.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess you're right. I'll revise the patch.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Updated fix to allocate the backbuffer using GDK. This is ugly but it's performant and nicely localized.

This also exposed a bug in draw_with_xlib and device offsets, which I've fixed.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 218123 [details] [diff] [review]
fix


>-    target = _get_current_target (cr, &device_offset_x, &device_offset_y);
>+    target = _get_current_target (cr);
>+    cairo_surface_get_device_offset (target, &device_offset_x, &device_offset_y);

Are you sure this is right?  I had to change this and have get_current_group_target() compute the offsets because if there are multiple groups active, the offset set on the surface is just relative to the previous group -- it's not cumulative.  get_current_group_target should return the cumulative offset from the top/0,0 offset surface.  It'll look like it works for a first-level group, but not for any higher.

Though.. I see what the problem is, I think.  The toplevel surface now has a device offset, and I think the get_group_target code assumes that the toplevel is 0,0.  The fix should probably go there.  (But I haven't looked at the code to confirm this.)

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> Though.. I see what the problem is, I think.  The toplevel surface now has a
> device offset, and I think the get_group_target code assumes that the toplevel
> is 0,0.  The fix should probably go there.  (But I haven't looked at the code
> to confirm this.)

Yes, that is the problem I thought I was fixing. I will revise.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

Okay, now I'm just computing the correct device offset manually by adding in the base surface offset. I don't really want to go messing around with your group and device-offsets work, because it looks like I'd have to change your internal API names at least. Feel free to fix it and remove this code.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 218211 [details] [diff] [review]
updated patch

Sounds good; I'm not even convinced that the current behaviour is wrong any more; get_group_targetis giving you the offset from the original surface to the group target.. whatever the initial surface offsets are shouldn't matter.  Anyway, let's hope this helps with performance..

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.