191474 - Eliminate useless GTK painting

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

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

As I explained in my email, GFX-GTK seems to repaint in response to
Invalidate()s without checking to see if the invalidated area is actually
visible. We should restrict the area to be repainted to the visible area, to the
extent we can.

We should also pass the paint region to the view manager instead of breaking it
up into rects and passing them one at a time. The view manager is quite capable
of painting a region intelligently.

Comment 1

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

OK, I have a patch. It seems to speed up some DHTML (e.g., Viewer demo #13) hugely.

But I can't figure out how to tell anything about whether a window is visible at
all, let alone what the visible area is. I've tried
  XWindowAttributes attrs;
  XGetWindowAttributes(GDK_DISPLAY(), GDK_WINDOW_XWINDOW(aWindow), &attrs);
  return attrs.map_state == IsViewable;
but it seems to return TRUE even when the window is iconized.

Comment 2

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

BTW the speedup seems to come from only doing one paint instead of processing
several paint events, one per rectangle in the region to be updated.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmmm.. As a gross hack we could get the root geometry and window geometry and
try to produce an exposed rect from that....

That won't help with other windows being on top of our window, however.  :(

I'd love to test your patch on those evil "opacity on 200 divs" testcases, btw.
It sounds to me like it should reduce painting from being O(N^2) in the number
of divs involved (which is what it is right now) to O(N)....

Comment 4

[Christopher Blizzard (:blizzard)]

XGetWindowAttributes() requires a server round trip.  Yuck!  Why don't you use
the cached coordinates?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Well, the idea is to find out what part of our window is on-screen and not
covered by other windows.  If we can do that without a server round-trip, great!
 (I'm not seeing a way to really do the second part even _with_ server round-trips.)

Comment 6

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

I just wanted to find out if the window is mapped or not, hoping that virtual
desktops and window iconification unmap nonvisible windows. But it doesn't even
seem to work.

BTW is there any documentation anywhere about which X and GTK calls require
server round trips? I haven't been able to find any.

Comment 7

[Christopher Blizzard (:blizzard)]

There's no way to tell what parts of a window are mapped and what windows are
overlapping it, at least not with the current X API.  Besides, that information
is useless because as soon as you get it it's out of date and someone might move
the windows on you.

If you want to know what windows are mapped and what windows aren't, it's pretty
easy to tell.  We might need to start tracking the GDK_MAP event, but that's
about it.

Oh, and for functions that require an X server round trip, pretty much any
function that includes a "Get" or "Query" require a flush and a roundtrip.

Comment 8

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

> Besides, that information is useless because as soon as you get it it's out of
> date and someone might move the windows on you.

... in which case we'd get expose events and we'd take care of it.

It turns out there is some information available in the form of
VisibilityChanged events which are tracked by the superwin stuff. It keeps a
visibility state GDK_VISIBILITY_FULLY_OBSCURED, GDK_VISIBILITY_PARTIAL, and
GDK_VISIBILITY_UNOBSCURED. I can use that to detect when a Mozilla window is
completely covered.

Unfortunately virtual desktop switching and window iconization give
GDK_VISIBILITY_PARTIAL instead of GDK_VISIBILITY_FULLY_OBSCURED. I'm not sure
why. It turns out that the XGetWindowAttributes that I had does work in this
situation if I query mSuperWin->shell_window. I've tried getting mapped status
from GTK some other way, but I can't find my way around the maze of superwins,
mozareas, shells, nsWindows, gtkwindows, and gdkwindows. I hope GTK2 is cleaner
than this :-).

Anyway, I'll attach the patch I have, which is still darn useful.

Comment 9

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

Attachment: patch

Here ya go. Extra per-platform work will be required to let other platforms
take advantage of whatever paint region info they have. This patch will still
work on all platforms though, if there's no region we just use the rect they
gave us.

Comment 10

[Christopher Blizzard (:blizzard)]

fwiw, gtk2 actually passed in a region for damaged areas.  Is that available for
use now?

(That's open as bug #121264 fwiw.)

Comment 11

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

Yep! With this patch, you're all set to go.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Hmm.... With this patch in my (opt GTK1) tree I see no particular speedups on
http://alladyn.art.pl/gandalf/MozillaTests/dynl-c31.html and I _do_ see a
regression -- the "Set opacity 10%" test does not paint the whole set of divs
but only points a little rect (about 50px tall by 300w wide?) in the top left of
the line of divs.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, I and I had to edit the patch to get it to apply, since nsRegion.h has moved
to gfx/public

Comment 14

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

The old GTK code would coalesce rects into a single "paint bounds rect" if the
region had more than 10 rects in it. So really complex invalid regions will
behave basically the same as before, especially if the the region mostly fills
its bounds rect.

I'm pulling a new tree now to check out the regression.

Comment 15

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

Well, I could reproduce it for a while but then it went away :-(. I suspect a
resource exhaustion issue with the blending buffers...

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

With the patch there is some problems with demo #10 too
(resource:///res/samples/test10.html). 
Images with opacity aren't painted correctly.
Propably the same bug as in 
http://alladyn.art.pl/gandalf/MozillaTests/dynl-c31.html .

Comment 17

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

Sadly, that test case works for me now.

Comment 18

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

Something really bad is happening ... it seems under some circumstances,
gdk_draw_image in nsDrawingSurfaceGTK::Unlock completely screws up when the clip
region in mGC is more than just one rectangle. It's not a problem in the blender
because I can replace the blending code with solid paint and the painting is
still clipped. Typically a large band is clipped out.

Comment 19

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

Attachment: Bugs fixed...

OK, better patch.

This inserts a workaround into nsRenderingContextGTK::CreateDrawingSurface that
clears the current clip region while the surface is created. We don't need that
clip area anyway and this avoids GTK bugs.

This also fixes a bug (which the old code had but was somehow masking) where
the update region was being cleared after DoPaint(). This was wrong because
painting can sometimes trigger further invalidation (e.g., when a native theme
fails to draw a widget and we have to paint it over again with the theme
disabled).

Comment 20

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

Comment on attachment 113901 [details] [diff] [review]
Bugs fixed...

going for r+sr from blizzard ... I'll get kmcclusk to review the view changes.

Comment 21

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

Ccing kmcclusk for view review ... Kevin, you may want to implement the Windows
code for sending the update region back up to the view manager.

Comment 22

[Boris Zbarsky [:bzbarsky]]

The nsWindow changes seem to have some transparent window stuff mixed in?  At
least the mIsTransparent and mTransparencyBitmap stuff?

Comment 23

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

Attachment: better patch

Er, yeah :-).

Comment 24

[Boris Zbarsky [:bzbarsky]]

/home/bzbarsky/mozilla/profile/mozilla/widget/src/gtk/nsWindow.cpp: In method
`nsresult nsWindow::Update()':
/home/bzbarsky/mozilla/profile/mozilla/widget/src/gtk/nsWindow.cpp:866:
`kRegionCID' undeclared (first use this function)
/home/bzbarsky/mozilla/profile/mozilla/widget/src/gtk/nsWindow.cpp:866: (Each
undeclared identifier is reported only once
/home/bzbarsky/mozilla/profile/mozilla/widget/src/gtk/nsWindow.cpp:866: for each
function it appears in.)

Comment 25

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

Attachment: terribly, terribly sorry

Comment 26

[Boris Zbarsky [:bzbarsky]]

Yeah, that works.  ;)  No need to apologize; I know it's hard to untangle
patches.  :(

I wouldn't have even bothered you about this except I was trying to see what
perf impact it has on typical "dhtml" testcase (not much, since they all fall in
the "200 divs" category....).  It does help the viewer demo a lot.  ;)

Comment 27

[Christopher Blizzard (:blizzard)]

Comment on attachment 113915 [details] [diff] [review]
terribly, terribly sorry

>+  mTranMatrix->TransformCoord(&trect.x, &trect.y,
>+                           &trect.width, &trect.height);

Can you fix the indenting here while you're at it?


>+  // Don't paint anything if our window isn't visible.
>+  if (!mSuperWin || mSuperWin->visibility == GDK_VISIBILITY_FULLY_OBSCURED)
>+      //      || !WindowIsVisible(mSuperWin->shell_window))
>+    return;

Since you're not using WindowIsVisible you can remove this line and the
WindowIsVisible function, right?

> 	paintEvent.rect						= &aRect;
>+  paintEvent.region         = nsnull;

More indenting?  (There are lots of other places like this in the patch.)

Other that those little nits, looks good to me.

Comment 28

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

> Since you're not using WindowIsVisible you can remove this line and the
> WindowIsVisible function, right?

Will do. I'll fix the indenting before checking in. Thanks.

Comment 29

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

Checked in.

Comment 30

[Geoff Leach]

Thanks for fixing this. I used to get a weird transient rectangular
copy-shift-paste blitting effect before. Gone now. And seems a little faster too.

Red Hat 8.0, gcc 3.2, cvs build (up to date as of 22/03/2003)

Configure arguments
--enable-svg --enable-crypto --enable-default-toolkit=gtk2 --disable-toolkit-gtk
--disable-toolkit-qt --enable-xft --enable-freetype2 --enable-cpp-rtti
--enable-cpp-exceptions --enable-extensions --with-system-jpeg
--with-system-zlib --with-system-png --with-system-mng

Comment 31

[Simon Fraser [no longer active]]

Can you clue me in as to how/if we might do the same thing for Mac?

Comment 32

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

You don't have to do anything.

If you want to take advantage of this, all you have to do is to fill in the
'region' field of the paint event with the region that needs to be painted. Then
the 'rect' field will be ignored.

Comment 33

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

For the record, this bug caused the regression in bug 199159.