518506 - Horrible flicker when scrolling windowed plugins living out-of-process

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P2)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Navigate a recent (> 2009/09/23) Electrolysis build to the URL above.  The plugin windows appear to go white on every scroll event.

I have no idea how to fix this, and it's not urgent at all, but I'd like to make sure it's something we know how to avoid.

Comment 1

[Karl Tomlinson (:karlt)]

This is a regression from bug 339548 and affects out-of-process plugins such as the java plugin even on mozilla-central and 1.9.2. e.g.
http://java.sun.com/products/plugin/1.5.0/demos/plugin/applets/GraphLayout/example1.html

Comment 2

[Karl Tomlinson (:karlt)]

This is caused by the unmapping and re-mapping of widgets for plugins.  That
worked OK for in-process plugins that used GDK because GDK temporarily removes
the background from descendant GdkWindows when showing windows.

If the backgrounds are still present when the ancestor is mapped, the X
server paints the background when the windows become visible.  However, the
contents are not painted until glib processes idle events.

I think the solution is going to be dividing up the region passed to
gdk_window_move_region so that each sub region can be ordered wrt child
widgets such that no overlapping occurs.

roc and I discussed this and it sounds like this would be best done in XP code
as that is where the sorting algorithm currently is (SortBlitRectsForCopy, for
MS Windows blitting I assume, but only for blit rects) and having the child
widgets inter-sorted may also be useful on MS win32.

Comment 3

[Karl Tomlinson (:karlt)]

The default Window background is actually None with XCreateWindow but some color with XCreateSimpleWindow, black for GDK windows, and dependent on the theme for GDK windows in GTK widgets.  Some documentation:
http://tronche.com/gui/x/xlib/window/attributes/background.html
http://my.safaribooksonline.com/9780596806187/settable_attributes
http://www.sbin.org/doc/Xlib/chapt_04.html

Comment 4

[Damon Sicore (:damons)]

Why would this block 1.9.2?

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to comment #4)
> Why would this block 1.9.2?

It is a noticeable regression from 1.9.1 when scrolling with any of a number of plugins.

This includes out of process plugins such as the Java plugin, and plugins run through nspluginwrapper, which sounds reasonably common on Fedora.

It would also affect any non-GDK or non-XEmbed plugins, such as Swfdec, which AFAIK is the best open-source alternative to Adobe Flash Player.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Need to fix, even if it means backing out the fix that caused this (although that's compositor related, yes?)

Comment 7

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

Karl has a plan for this that will not require a backout.

Comment 8

[Joe Drew (not getting mail)]

Status update?

Comment 9

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

Karl is working on it.

Comment 10

[Joe Drew (not getting mail)]

Karl, will you have this done by the freeze (Wed night pacific)? It's one of the only gfx blockers that hasn't seen much traction.

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: (failed) experimental scrolling with Xcomposite

This is a record of an attempt at a quick solution using the Composite extension to redirect rendering of plugin windows during the scroll.  It possibly works quite well during the scroll, but when unredirecting rendering, the X server shows the background and sends an expose event -> same flicker problem.

Redirecting all (visible) plugin windows permanently would probably scroll well but I don't think we want the extra round trip from when the plugin draws to when we get notified and copy to the visible window.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

For the record, last time I checked gdk_window_set_composited() wasn't in our minimum required gtk version.

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: WIP 0.1.3

This is the work in progress.  It contains some proof of concept code and some
framework for doing this properly (as well as splashes of other ideas - ignore the nsRegion changes).

The approach is as described in comment 2 though its all just in GTK code at
the moment.  (It doesn't use Composite.)

Thus far it proves the idea works.  It orders the blit rects appropriately but
doesn't order the child widgets well.  Pixman is used to order the blit rects.
Pixman doesn't seem to have any documentation, but X relies on the same behavior so I'm not expecting any changes in what it provides.

It works well with rectangular plugin clip regions when scrolling vertically
or horizontally or when there are not many plugins or fixed position elements
around.

Remaining issues are particularly noticeable when scrolling unaligned adjacent plugins diagonally.  That is resolvable.

Still to do:
1) Finish ordering of child widgets for diagonal scrolling.
2) Work out what, if any, should be XP.
3) Properly fix build issues with pixman and --enable-shared.
4) Clean up.

A difference that is possibly not preferred is that, when plugins scroll into
view (either from out of the viewport or from behind a fixed position element)
the X server paints the background for the unpainted region before the plugin
gets the expose event.  Previously the previous screen content remained,
giving a blurring or tiling effect.  I think this difference is reasonable,
given that it is the plugin that chooses what background should be painted
when it is exposed.  What this approach is fixing is the hide and show that
the plugin can't control.

(In reply to comment #10)
> Karl, will you have this done by the freeze (Wed night pacific)?

I working hard on this and hoping to have this done by then.
Depends how reviews go, though.

Comment 14

[Joe Drew (not getting mail)]

To the 1.9.2 drivers: This is both very scary and very important, and, were I in the position to make such a decision, I would _totally_ hold the release for this bug.

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to comment #13)
> Still to do:
> 1) Finish ordering of child widgets for diagonal scrolling.
> 2) Work out what, if any, should be XP.
> 3) Properly fix build issues with pixman and --enable-shared.
> 4) Clean up.

Forgot to mention:
5) Fix bug 528852

Without that fixed, it could be quite obvious when scrolling with mulitple windowed plugins.  That bug shouldn't be as complex as what is happening here.

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: WIP 0.2.1

Filled in some gaps in child widget ordering and pruned out unused stuff.
Diagonal scrolling now usually works well.
Still need to resolve cycles better for complex geometries.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm not sure this blocks; most Linux distros aren't going to pick up Firefox 3.6 for a while, and I'm told that Java on Linux isn't that popular.

Moving back to a nomination to discuss.

Comment 18

[Reed Loden [:reed]]

(In reply to comment #17)
> I'm not sure this blocks; most Linux distros aren't going to pick up Firefox
> 3.6 for a while, and I'm told that Java on Linux isn't that popular.

The first part of that is not really correct. Ubuntu, for example, is already working out plans to upgrade its users to Firefox 3.6 due to upstream's (us) decision to most likely upgrade 3.5 users to 3.6 as a minor update.

However, I don't know how popular Java is, but asac may have some numbers.

Comment 19

[Karl Tomlinson (:karlt)]

Attachment: make public pixman symbols available in --disable-libxul and --enable-system-cairo builds

Comment 20

[Benjamin Smedberg]

Attachment: Wrap pixman.h in non-libxul build, rev. 1

I think this is the simpler way to do what you want.

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 413066 [details] [diff] [review]
Wrap pixman.h in non-libxul build, rev. 1

>diff --git a/config/system-headers b/config/system-headers

>+pixman.h

Thanks, Benjamin.  I tried this first and accidentally left this out of attachment 413042 [details] [diff] [review], but it wasn't enough on its own.

pixman definition source files include "pixman.h" not <pixman.h>, so they get the version in the same directory as the source file, not the dist/system_wrappers version.

I see now that PIXMAN_EXPORT is set up correctly for __GNUC__ and __SUNPRO_C and used in definitions, so that works well with -fvisibility=hidden (and -xldscope=hidden I assume), but PIXMAN_EXPORT is not used in the headers, and "#pragma GCC visibility push(hidden)" seems to affect the declarations which end up overriding the definitions.

I'm not sure whether sorting this out is essential as libpixman users should be using MOZ_CAIRO_LIBS (or similar) anyway, but it seems nice to pick up the definition from libthebes.so if it is already there rather than having another copy.

Comment 22

[Karl Tomlinson (:karlt)]

Attachment: make public pixman symbols available in --disable-libxul and --enable-system-cairo builds v2 [Checkin: See comment 38]

Comment 23

[Karl Tomlinson (:karlt)]

(In reply to comment #17)
> I'm told that Java on Linux isn't that popular.

See also comment 5.  If nspluginwrapper is widely used, then this will show up with Adobe Flash Player.

Comment 24

[Benjamin Smedberg]

Comment on attachment 413152 [details] [diff] [review]
make public pixman symbols available in --disable-libxul and --enable-system-cairo builds v2
[Checkin: See comment 38]

Ugh. That's really ugly makefile magic (please remove the $(warning) before checkin.)

Is there really no way to modify pixman.h so that for example it has something like:

#ifdef USE_VISIBILITY_PRAGMAS
#pragma GCC visibility push(default)
#endif

body of pixman.h

#ifdef USE_VISIBILITY_PRAGMAS
#pragma GCC visibility pop
#endif

Comment 25

[Karl Tomlinson (:karlt)]

(In reply to comment #24)
> (please remove the $(warning) before checkin.)

Oh, yeah.  Thanks.

> Is there really no way to modify pixman.h so that for example it has something
> like:
> 
> #ifdef USE_VISIBILITY_PRAGMAS
> #pragma GCC visibility push(default)
> #endif
> 
> body of pixman.h
> 
> #ifdef USE_VISIBILITY_PRAGMAS
> #pragma GCC visibility pop
> #endif

Something like that could probably be done.
We are using pixman sources designed to use -fvisibility=hidden but dropping their configure that provides the flag and using our own Makefile and magic.  The issue results from our build system being different, so I'd like to fix in our build system if possible.

What about just setting VISIBILITY_FLAGS to -fvisibility=hidden for gcc?
Would there be any disadvantage in that for C code?
The linker seems to resolve DEFAULT UND internal references to the HIDDEN symbols.  Would the calls really go through the PLT?

Comment 26

[Karl Tomlinson (:karlt)]

(In reply to comment #25)
> The linker seems to resolve DEFAULT UND internal references to the HIDDEN
> symbols.  Would the calls really go through the PLT?

OK.  Sounds like that is not optimal.

http://gcc.gnu.org/wiki/Visibility
"This also helps producing more optimised code: when you declare something defined outside the current compilation unit, GCC cannot know if that symbol resides inside or outside the DSO in which the current compilation unit will eventually end up; so, GCC must assume the worst and route everything through the GOT (Global Offset Table) which carries overhead both in code space and extra (costly) relocations for the dynamic linker to perform. To tell GCC a class, struct, function or variable is defined within the current DSO you must specify hidden visibility manually within its header file declaration (using the example above, you declare such things with DLLLOCAL). This causes GCC to generate optimal code."

Comment 27

[Karl Tomlinson (:karlt)]

Attachment: patch v0.5

This is now fully functional.  This version includes support for non-rectangular plugin geometries and resolves cycles appropriately.  It does not include a fix for bug 528852.  It needs cleaning up and more comments.

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Jeff showed the effects of this to me today; doesn't block, IMO, but the fix is great to have, thanks Karl!

Comment 29

[Karl Tomlinson (:karlt)]

Attachment: order child widget moves carefully when scrolling

Comment 30

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

+    if (absDelta.y >= absDelta.x) {

Slightly simpler to just use PR_ABS(aDelta.y) >= PR_ABS(aDelta.x) and dispense with the local variables.

+    // binary merge

You should probably explain why this is necessary.

+            // gdk_window_move_region, up to GDK 2.16 at least, has a ghastly
+            // bug where it doesn't restrict blitting to the given region, and
+            // blits its full bounding box.  So we have to work around that by
+            // blitting one rectangle at a time.

This comment is obsolete now that you've integrated plugin movement.

Comment 31

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

This patch is amazing. In fact, I think it's TOO amazing. Normal people wouldn't even have tried to do this good a job in the pathological situations you handle :-). But I think it's more complex than I really want to maintain (or test). I think we should focus on the cases where a) plugins don't overlap and b) plugin clip regions are at most a single rectangle. Both of these situations were handled very poorly or not at all pre-3.6 and they are very rare in the wild, as far as I know. It seems that given these two assumptions, the code can be vastly simpler; if the assumptions are violated, we'll get flicker but it won't be the end of the world. If we hit some important cases where these assumptions don't hold we can always revive this patch.

Comment 32

[Karl Tomlinson (:karlt)]

Comment on attachment 413152 [details] [diff] [review]
make public pixman symbols available in --disable-libxul and --enable-system-cairo builds v2
[Checkin: See comment 38]

check-sync-dirs.py wants js/src/config/system-headers to be the same as config/system-headers even though js won't use pixman, so that change will need to be made if this patch is still needed for v2.

Comment 33

[Karl Tomlinson (:karlt)]

Attachment: move SortBlitRectsForCopy from nsScrollPortView to nsBaseWidget

This would save two sorts being done with MOZ_WIDGET_GTK2, but that would be the only platform to gain.

Comment 34

[Karl Tomlinson (:karlt)]

Attachment: order child widget moves carefully when scrolling v1.1

This is just the algorithm of attachment 414135 [details] [diff] [review] merged with the fix for bug 528852, which required modification to deal with child window moves.

Comment 35

[Karl Tomlinson (:karlt)]

Attachment: order child widget moves (only) a little carefully when scrolling [Checkin: See comment 39]

This version only takes care about plugin widgets with rectangular clip
regions.  Overlapping rectangular clip regions will still end up going through
the sort, but the sort does not provide any particular order for those
widgets.

This uses essentially the same algorithm as used in SortBlitRectsForCopy(),
changing the code to use a linked list so that the same sort code is used on
all platforms.

(Pixman regions are used here mainly because pixman_region32_init_rects() is
useful, though the sort is also more efficient with banded rectangles.  Doing
an efficient version of pixman_region32_init_rects for nsIntRegion is
non-trivial when the rectangles can overlap, as they may do for child clip
regions.  I'm not sure whether it really makes sense to implement another
version for nsRegion or to use pixman_region32 to implement nsRegion rather
than having two different region implementations in the tree.  Right now it
seems easiest to use pixman regions here.)

Comment 36

[Karl Tomlinson (:karlt)]

Attachment: mochitest [Checkin: See comment 40]

Comment 37

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

Comment on attachment 417177 [details] [diff] [review]
order child widget moves (only) a little carefully when scrolling
[Checkin: See comment 39]

+    // rectangles is moving in the direction of decreasing x and y (left

Rectangles *are* moving

+  PRBool HaveMore() { return mHead != nsnull; }

We tend to use the name "IsDone" here.

Comment 38

[Karl Tomlinson (:karlt)]

Comment on attachment 413152 [details] [diff] [review]
make public pixman symbols available in --disable-libxul and --enable-system-cairo builds v2
[Checkin: See comment 38]

Pushed with the $(warning) removed and the change to js/src/config/system-headers (comment 32).
http://hg.mozilla.org/mozilla-central/rev/d6cf9a15c50f

Comment 39

[Karl Tomlinson (:karlt)]

Comment on attachment 417177 [details] [diff] [review]
order child widget moves (only) a little carefully when scrolling
[Checkin: See comment 39]

Pushed with changes from review in comment 37.
http://hg.mozilla.org/mozilla-central/rev/274480cf21d7

Comment 40

[Karl Tomlinson (:karlt)]

Comment on attachment 417178 [details] [diff] [review]
mochitest
[Checkin: See comment 40]

I changed this from a chrome test to a simple mochitest.  I don't understand why, but the script in the chrome test doesn't seem to have access to the plugin object methods (with the plugin in a document loaded in the iframe).  There's no good reason for this to be a chrome test anyway.

http://hg.mozilla.org/mozilla-central/rev/f20c4c211ac8
http://hg.mozilla.org/mozilla-central/rev/808b422d1274

With MOZ_WIDGET_TOOLKIT == cocoa, the test failed with
OS X 10.5.2 try hg unit test
TEST-UNEXPECTED-FAIL |
 /tests/widget/tests/test_plugin_scroll_invalidation.html |
 no paint on scroll - got 17, expected 10
Perhaps that might be because nsChildView::Scroll is not fussy about the order in which child views are Resized, but then I didn't know that view resizing caused invalidations.

With MOZ_WIDGET_TOOLKIT == windows, the test failed with
TEST-UNEXPECTED-FAIL |
 /tests/widget/tests/test_plugin_scroll_invalidation.html |
 no paint on scroll - got 11, expected 5
I don't know why ScrollWindowEx would be causing invalidations there.

I don't know how to test for the widget implementation from javascript (testing for Linux is not right) in order to use a todo so I've only added the test for MOZ_WIDGET_TOOLKIT == gtk2.  Perhaps the consequences are not as bad on the other windowing systems as they are on X11 anyway.

Comment 41

[Karl Tomlinson (:karlt)]

Attachment: give more time for painting to settle

This was an experimental change to the mochitest, to wait longer for painting to complete, in case resizing cause synchronous (or at least earlier) paints.
With this patch there was still the same number of unnecessary paints.

OS X 10.5.2 try hg unit test
TEST-UNEXPECTED-FAIL |
 /tests/widget/tests/test_plugin_scroll_invalidation.html |
 no paint on scroll - got 14, expected 7

WINNT 5.2 try hg unit test
TEST-UNEXPECTED-FAIL |
 /tests/widget/tests/test_plugin_scroll_invalidation.html |
 no paint on scroll - got 16, expected 10

Comment 42

[James Cook]

This breaks the Qt build with a compilation error caused by http://hg.mozilla.org/mozilla-central/diff/274480cf21d7/widget/src/qt/nsWindow.cpp since it includes an extra brace.

Comment 43

[[e-mail]]

Comment on attachment 417177 [details] [diff] [review]
order child widget moves (only) a little carefully when scrolling
[Checkin: See comment 39]

>+  for (ScrollRect** nextLink = aUnmovedLink; ScrollRect* otherRect = *nextLink; ) {
[Seeing a declaration in a conditional expression confused me, and VC7.1 too!]

Comment 44

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=11588 has been added to Litmus.

Comment 45

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/b2e1a5533d2b

http://hg.mozilla.org/projects/firefox-lorentz/rev/b60caef6244c
Karl, please carefully check this one, especially the nsChildView.mm changes, since there were some branch changes that didn't apply directly.

http://hg.mozilla.org/projects/firefox-lorentz/rev/90524c2db1c1
http://hg.mozilla.org/projects/firefox-lorentz/rev/2630d5fc17f0

Comment 46

[Benjamin Smedberg]

Typo from merge: http://hg.mozilla.org/projects/firefox-lorentz/rev/95ca292c119e

Comment 47

[Karl Tomlinson (:karlt)]

(In reply to comment #45)
> http://hg.mozilla.org/projects/firefox-lorentz/rev/b60caef6244c
> Karl, please carefully check this one, especially the nsChildView.mm changes,
> since there were some branch changes that didn't apply directly.

Just one minor thing that only affects --enable-shared builds (not --enable-ipc):

http://hg.mozilla.org/projects/firefox-lorentz/rev/75e81b7bf3f3

Comment 48

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 49

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430

Comment 50

[Al Billings [:abillings - ex-MoCo]]

Marking verified for 1.9.2 based on passing mochitests.