Eliminate useless GTK painting

RESOLVED FIXED

Status

Core Graveyard
GFX: Gtk
RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

(Blocks: 2 bugs, {perf})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fix])

Attachments

(1 attachment, 3 obsolete attachments)

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.

Updated

16 years ago
Keywords: perf
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.
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.
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)....
XGetWindowAttributes() requires a server round trip.  Yuck!  Why don't you use
the cached coordinates?
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.)
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.
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.
> 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.
Created attachment 113366 [details] [diff] [review]
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.
fwiw, gtk2 actually passed in a region for damaged areas.  Is that available for
use now?

(That's open as bug #121264 fwiw.)
Yep! With this patch, you're all set to go.
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.
Oh, I and I had to edit the patch to get it to apply, since nsRegion.h has moved
to gfx/public
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.

Updated

16 years ago
Blocks: 21762
Well, I could reproduce it for a while but then it went away :-(. I suspect a
resource exhaustion issue with the blending buffers...
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 .
Sadly, that test case works for me now.
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.
Created attachment 113901 [details] [diff] [review]
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).
Attachment #113366 - Attachment is obsolete: true
Comment on attachment 113901 [details] [diff] [review]
Bugs fixed...

going for r+sr from blizzard ... I'll get kmcclusk to review the view changes.
Attachment #113901 - Flags: superreview?(blizzard)
Attachment #113901 - Flags: review?(blizzard)
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.
The nsWindow changes seem to have some transparent window stuff mixed in?  At
least the mIsTransparent and mTransparencyBitmap stuff?
Created attachment 113909 [details] [diff] [review]
better patch

Er, yeah :-).
Attachment #113901 - Attachment is obsolete: true
Attachment #113901 - Flags: superreview?(blizzard)
Attachment #113901 - Flags: review?(blizzard)
Attachment #113909 - Flags: superreview?(blizzard)
Attachment #113909 - Flags: review?(blizzard)
/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.)
Attachment #113909 - Attachment is obsolete: true
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.  ;)

Updated

16 years ago
Blocks: 186465
Status: NEW → ASSIGNED
Whiteboard: [fix]
Attachment #113915 - Flags: superreview?(blizzard)
Attachment #113915 - Flags: review?(blizzard)
Attachment #113909 - Flags: superreview?(blizzard)
Attachment #113909 - Flags: review?(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.
Attachment #113915 - Flags: superreview?(blizzard)
Attachment #113915 - Flags: superreview+
Attachment #113915 - Flags: review?(blizzard)
Attachment #113915 - Flags: review+
> 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.
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 30

16 years ago
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

16 years ago
Can you clue me in as to how/if we might do the same thing for Mac?
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.
For the record, this bug caused the regression in bug 199159.

Updated

14 years ago
No longer blocks: 21762
Blocks: 21762
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.