Closed Bug 626245 Opened 14 years ago Closed 12 years ago

Plugins bounce up and down when being scrolled

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19
Tracking Flags:
Tracking Status
firefox18 + verified
firefox19 --- fixed

People

(Reporter: mozbugz, Assigned: roc)

References

Details

(Whiteboard: verifyme)

Attachments

(14 files, 5 obsolete files)

Screencast A 30fps
1.09 MB, video/ogg
Details
Screencast A 1fps Slow Motion
1.09 MB, video/ogg
Details
Using Mousewheel, scrolling plugin position painting weirdness
97.57 KB, image/jpeg
Details
More visible video of bouncing behavior Recorded 30 fps, played at 1fps:
1.89 MB, video/webm
Details
More visible video of bouncing behavior Recorded 30 fps:
749.25 KB, video/webm
Details
Scrolling time comparison between pages with and without flash using bookmarklet see Comment 29
75.95 KB, image/png
Details
testcase
607 bytes, text/html
Details
Part 1: fix test_plugin_scroll_invalidation
2.88 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Part 2: Fix test_convertpoint
1.30 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Part 3: Make aAncestor const in TransformFrameRectToAncestor
2.74 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Part 5: Move plugin widgets before compositing the window, not after
1.38 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Part 4 v3
62.48 KB, patch
MatsPalmgren_bugz
: review+
bajaj
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Part 3.5: fix nsLayoutUtils::Transform functions
5.46 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Part 4.5 v2
10.21 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review 
I noticed this with and without hardware acceleration.  Open page with a flash plugin such as youtube, and scroll the page either up or down. 

The plugin bounces instead of painting smoothly with the page.
Is this the same as the effect seen in the video in bug 622492?
Its kinda hard to see on those videos, but it might be captured on there.  I usually see it more when testing by holding down the scrollbar arrows. It also bounces from left to right.  I hope to produce a few more so it can be seen better.
Attached video Screencast A 30fps — 
Look at the the difference in attached video files, especially in Slow motion:

As far I can notice flash object on these site:

http://tatactic.be/morse-decoder-encoder-AS3-flash/

Lag to update position when page is scrolled.

But due to potential regression ? (bug 622492)
scrolling page with Youtube player:

http://www.youtube.com/watch?v=Ou6_MkIvKOo

Contains in my opinion the above lag + chopiness introduced by fix bug 590568


Check also longer video link on Youtube:
http://www.youtube.com/watch?v=ZJbo6s9tGdg

Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110121 Firefox/4.0b10pre
This bug was not caused by the fix for 590568.  Lets keep the choppiness in bug 622492.  I think this bug was introduced between b4 and b6.
Slightly more accurate regression range
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e1d55bbd1d1d&tochange=6e3f6d18c124

I wouldn't be surprised if it wasn't one of Bas' two changesets at the top there.
blocking2.0: --- → ?
Component: Graphics → Widget: Win32
QA Contact: thebes → win32
This is gross-looking, but not wholly broken.
Assignee: nobody → bas.schouten
blocking2.0: ? → final+
Whiteboard: [softblocker]
Possible duplicates (?) of this bug:
Bug 557533
Bug 625948
I would say those are both dupes and i noticed that bouncing was worse after that regression range day we noted above.. but its there way back too but more subtle which I did do some testing with May 2010 builds and without D2D or OOPP for this bug and noticed it was subtle, but not as much as it is now, which was similar to what bug 557533 describes.  

If I use the mousewheel and it scrolls big chucks at a time I see a lot of plugin position painting issues with using the most current hourly.  So maybe that exposed more of this bug, or caused new issues so I'll add a screen shot when I used the mousewheel.
Found a good test link, it has 5 flash videos stacked vertically.
http://www.autoblog.com/2011/02/04/videos-chevy-drops-entire-load-of-super-bowl-commercials-on-us/#continued
I am not sure if another bug patch has been applied that partially fixed the Flash bounce, but I am noticing LESS bounce than before with the current Minefield (2/8/11 Windows 32-Bit).
(In reply to comment #14)
> I am not sure if another bug patch has been applied that partially fixed the
> Flash bounce, but I am noticing LESS bounce than before with the current
> Minefield (2/8/11 Windows 32-Bit).

Sorry....should have specified what I am seeing.

Flash ads / banners are okay now.

YouTube videos (which use Flash) are still slightly bouncy.
I'm guessing the ones which are better are windowless flash (youtube when it is not embedded on another page is windowed), they were probably always better.
Yeah, this hasn't changed.  I haven't even took a look at the flash ads or cared about them here, just flash videos only, and this is still pretty gross.
Bouncing also occurs with QuickTime videos and Silverlight plug-in videos, such as on the following site (IE 9 Test Drive Site):

http://ie.microsoft.com/testdrive/Browser/ActiveXFiltering/Default.html?o=1


Should separate bug reports be filed for those issues or can they remain on this page?
Please stay in this bug.
(In reply to comment #19)
> Please stay in this bug.

Okay - thanks ROC!  That's what I thought as this bug is already marked as a Final blocker.  I kind of figured if you are going to take the time to solve the Flash bounce problem, you might as well solve the issue with ALL problem plugins.  Maybe when you come up with a patch for the Flash issue it will automatically solve the other plugin issues.  That would be nice!
I think what roc meant to say is that they are probably all the same problem.
So, I notice this a lot on a trunk debug build, but a lot less on a beta11 release build. This could simply be due to performance differences, but interestingly enough there's a subtle difference in the events on the window:

Beta 11:
WM_MOVE
WM_WINDOWPOSCHANGED
WM_WINDOWPOSCHANGING
WM_ERASEBKGND
WM_WINDOWPOSCHANGED

Nightly:
WM_MOVE
WM_WINDOWPOSCHANGED
WM_WINDOWPOSCHANGING
WM_ERASEBKGND
WM_WINDOWPOSCHANGED
WM_WINDOWPOSCHANGING
WM_ERASEBKGND
WM_WINDOWPOSCHANGED
Summary: Flash Plugin bounces up and down when being scrolled → Plugins bounce up and down when being scrolled
With lot's of flash embedded videos it's pretty much bad:
http://www.pcgamer.com/2011/02/15/10-incredible-minecraft-creations/
This could be related to, or caused by, bug 634844.
Bug 634844 landed, any change in a 2011-02-23 nightly (or newer)?
(In reply to comment #25)
> Bug 634844 landed, any change in a 2011-02-23 nightly (or newer)?

I think that I don't see any noticeable difference in bouncing behavior after Bug 634844 landed:

Please look at the recorded screencast:

Recorded at 5 fps:
http://www.youtube.com/watch?v=rCcer7uhCxU

Recorded at 5 fps played at 1 fps:
http://www.youtube.com/watch?v=4QGHoXdFpIQ

Mozilla/5.0 (Windows NT 6.1; rv:2.0b13pre) Gecko/20110224 Firefox/4.0b13pre
Direct2D Off 
DirectWrite Off
Windows 7 32 bit Aero on

Forced GPU Accelerated Windows 1/1 Direct3D 9:

http://tatactic.be/morse-decoder-encoder-AS3-flash/
Flash element visible bounce when scrolling.
http://www.youtube.com/watch?v=YW8p8JO2hQw
When video (flash plugin) is visible scrolling slow down and is very choppy, when plugin is scrolled outside the visible region scrolling is fluent. Noticeable flash plugin bouncing.

GPU 0/1:

http://tatactic.be/morse-decoder-encoder-AS3-flash/
Bouncing is less visible
http://www.youtube.com/watch?v=YW8p8JO2hQw
Scrolling the same as GPU accelerated, but more noticeable flickering (especially in video control button area)
(see comment 26 for more detail) Gecko/20110224 Firefox/4.0b13pre
(see Comment 26 for more detail) Gecko/20110224 Firefox/4.0b13pre
This scrolling test could explain why the bouncing without hardware acceleration is less visible: 

(Using bookmarklet from Bug 623615, changed value: ++i=500)

javascript:void((function(){var%20i=0;var%20start=Date.now();function%20f(){if(++i==500){alert((Date.now()-start)+"ms");return;}window.scrollTo(0,i*5);setTimeout(f,10);}f();})())

Forced 1/1 direct3d9

Page with flash:
Chrome dev 11: 6759ms
Minefield:24968ms
Page without flash: 
Chrome dev 11: 5677ms
Minefield:7366ms

GPU 0/1

Page with flash:
Chrome dev 11: 6759ms
Minefield:15904ms
Page without flash: 
Chrome dev 11: 5677ms
Minefield:8291ms

Page with flash:
http://www.pcgamer.com/2011/02/15/10-incredible-minecraft-creations/
Page without flash: 
https://bugzilla.mozilla.org/show_bug.cgi?id=623615
I have also seen this on Fennec on my Galaxy S with flash.  Should I file a new bug?
Hmm, fennec doesn't (can't) load flash on android last I saw (and plugins are disabled by default).  Odd.  Yes, new bug sounds better.
kolubinowicki, thanks for the data.

Bas, can you profile kolubinowicki's testcase? This is probably the core of the problem:
Page with flash:
Chrome dev 11: 6759ms
Minefield:24968ms
I checked this on an NVidia ION:

http://www.autoblog.com/2011/02/04/videos-chevy-drops-entire-load-of-super-bowl-commercials-on-us/#continued

With Flash:
D2D off, hw layers off: 13188ms
D2D off, hw layers on(d3d9): 8266ms
D2D on, hw layers on(d3d10): 13679ms

With One windowed Flash (single youtube page, with video playing):
D2D off, hw layers off: 14423ms
D2D off, hw layers on(d3d9): 13476ms
D2D on, hw layers on(d3d10): 11313ms

I'm still working on rendering the page with flash disabled (rendering a different page seems to defy the point of comparing measurements).
Without Flash:
http://www.autoblog.com/2011/02/04/videos-chevy-drops-entire-load-of-super-bowl-commercials-on-us/#continued

D2D off, hw layers off: 12004ms
D2D off, hw layers on(d3d9): 8277ms
D2D on, hw layers on(d3d10): 16158ms

Note that in the D2D case, it appears to be the 'Featured Gallery' thing and other stuff that's slowing things down. That's probably something that could use investigation. I've got no idea why that happens.
Bas, can you compare to Chrome?

But I guess you're just seeing something fundamentally different to what kolubinowicki sees, since for him D3D9 is significantly lower than Basic, and for you it's faster.

Although I do wonder why he said "This scrolling test could explain why the bouncing without hardware acceleration is less visible:"
(In reply to comment #35)

> But I guess you're just seeing something fundamentally different to what
> kolubinowicki sees, since for him D3D9 is significantly lower than Basic, and
> for you it's faster.
> 
> Although I do wonder why he said "This scrolling test could explain why the
> bouncing without hardware acceleration is less visible:"

Maybe this could be helpful?: after landing patch from bug 590568, there is filled bug:
Bug 622492 - Scrolling performance choppiness due to fixing bug 590568 

My computer hardware was affected by bug 590568, so when I forced 1/1 d3d9 acceleration on, I see (subjective to me) bigger bouncing of flash plugin effect when using GPU 0/1 settings
I used build (Gecko/20101230 Firefox/4.0b9pre) before patch to bug 590568 landed (http://hg.mozilla.org/mozilla-central/rev/39db16b78175 on page:
 
http://www.pcgamer.com/2011/02/15/10-incredible-minecraft-creations/

Gecko/20101230 Firefox/4.0b9pre
http://hg.mozilla.org/mozilla-central/rev/ec3e4786877d
1/1 direct 3d9
16729ms

Build after landing patch to bug 590568:
Gecko/20100101 Firefox/4.0b10
http://hg.mozilla.org/mozilla-central/rev/bb9089ae2322
1/1 direct 3d9
20532ms

See also other regression range on:

Bug 636686 - Scrolling page is very slow and lagged on youtube
(In reply to comment #31)
> Hmm, fennec doesn't (can't) load flash on android last I saw (and plugins are
> disabled by default).

Well, it can load them, at least Flash, and they're not disabled by default.

> Odd.  Yes, new bug sounds better.

Filed bug 637585.
** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 7 automatically changed from blocking2.0:final+ to blocking2.0:.x during the endgame of Firefox 4 for the following reasons:

 - it was marked as a soft blocking issue without a requirement for beta coverage
blocking2.0: final+ → .x+
(In reply to comment #11)
> Created attachment 509669 [details]
> Using Mousewheel, scrolling plugin position painting weirdness

Sometimes after scrolling page when embedded Youtube video is playing, video could freeze in some part of plugin area, glitches of video control buttons also could occur: 
http://i.imgur.com/m2b9m.png
http://i.imgur.com/hf73a.png
http://i.imgur.com/OfQN2.png
http://dl.dropbox.com/u/23326175/bug/flash10.3x.theora.ogv
Gecko/20110322 Firefox/4.0b13pre
GPU Accelerated Windows 0/1
Flash 10.3.180.42 (I also saw that behavior on older stable flash) 
Windows 7 32bit
The latest nightly improved this considerably after the layout changes landed with testing hourly built off of http://hg.mozilla.org/mozilla-central/rev/7ccb164032a0
Hmm, really? If someone could narrow down what made it better, that would be interesting.
Well, maybe it just scrolls better, but its still bouncing with glitches on my home pc which is the machine I reported it on, with a dual core 2, ati 5450 w/d3d10.  My work machine is much better with i7.
I think this became even worse with DLBI and Windows 8. Way worse.

Anyway, the Flash Plugin seems rendered out of sync with the page. The page is rendered first and then the flash plugin. Problem with this approach is that when the page has scrolled say 300px, flash may be 300px behind. Which makes Flash look like it is on top of the site menu or something.

Worst is when dragging the scrollbar. Medium when scrolling with the scroll wheel. Best when using arrow keys.

Now here is an extreme case:
It is also horribly visible when I was toggling Firebug on the left side on and off - That causes the site to resize. The Flash Plugins stayed in their old position for a few seconds, then updated. Same thing happens when changing tabs. Obviously part of this was due to the fact that Windows 8 is incompatible with FF OOPP model.

But the async painting issue has been around for years.

It is very distracting. Please make Flash paint with the webpage and NOT escape its bounding frame. Like IE10 does
(In reply to Alex from comment #45)
> Obviously part of this was due to the fact that Windows 8 is
> incompatible with FF OOPP model.

I don't think it is.

> It is very distracting. Please make Flash paint with the webpage and NOT
> escape its bounding frame. Like IE10 does

We've developed a new NPAPI feature for GPU-accelerated rendering without an HWND. Unfortunately Adobe hasn't updated Flash to use it yet.

Anyway, I'm looking into this.
Attached file testcase — 
If you open this testcase (warning: designed to paint very slowly!) and scroll slowly one line at a time (using up/down arrow or scrollbar arrows), everything is perfect (Windows 7, HWA enabled); the plugin and the window contents are perfectly in sync.

If you scroll multiple lines at a time, things quickly get out of sync.
I think what's happening is that right after the window has been painted we move the plugin widget via rootPresContext->UpdatePluginGeometry() in PresShell::DidPaint(). Painting the window draws the window state as of the last refresh driver run, since we do a PaintType_Composite paint, but the position of the plugin might have changed in the frame tree between the last refresh driver tick and the painting of the window.
I think what we really want to do is compute the new plugin window position and clip region in the refresh driver tick (when we do the no-composite layer tree update; we can even reuse that display list) and then apply them in DidPaint.
Why is it that the same page scrolls very quickly in IE8?
Various reasons, including that IE8 doesn't support border-radius or box-shadow.
Attached patch patch (obsolete) — — Splinter Review 
This patch works really well at keeping the plugin positions in sync with the window contents. It's basically perfect for me.
Assignee: bas.schouten → roc
https://tbpl.mozilla.org/?tree=Try&rev=56031a1322ec
There will definitely be some test failures, because some of our tests assume that flushing layout updates the plugin geometry synchronously, and this patch stops doing that. (Synchronous plugin geometry changes on FlushPendingNotifications(Flush_Layout) happen frequently and cause visible glitches, so I think we should stop doing them).
I'm not sure whether it's best to update plugin widgets just before or just after painting the window. Doing it before the paint introduces a delay between updating widgets and painting the window, equal to the time required to composite the window's layer tree. But doing it after the paint means we have to paint (recomposite) again almost immediately if any plugin widgets changed size or position, to fill in any strips in our main window that have been exposed by the plugin widget changes. Those extra paints could be costly especially when GPU-accelerated compositing is not available.

The previous patch makes us update plugin widgets after compositing. This patch makes us update plugin widgets before compositing.
Attachment #669017 - Flags: review?(matspal)
If the results with part 5 are "good enough" in terms of visually keeping plugins in sync with the window content, then I think we should take it to reduce the overall CPU/GPU load.
Attachment #669009 - Flags: review?(matspal) → review+
Attachment #669010 - Flags: review?(matspal) → review+
Attachment #669011 - Flags: review?(matspal) → review+
Karl pointed out that Part 4 actually doesn't update plugin geometry during the composite step, but when we update the layer tree.

This patch makes us actually update plugin geometry during the composite step.
Attachment #669017 - Attachment is obsolete: true
Attachment #669017 - Flags: review?(matspal)
Attachment #669858 - Flags: review?(matspal)
Comment on attachment 669012 [details] [diff] [review]
Part 4: Compute plugin widget geometry updates via the refresh driver's painting, and defer actual widget updates until we've just composited the window.

r- because this breaks rendering of zoomed pages on Linux -- the plugin
does not change size, has wrong position, invalidation problems...
(it seems to work on Win7 though)


>layout/base/nsCSSFrameConstructor.cpp
>-  if (didInvalidate && !didReflow) {

It looks like didInvalidate and didReflow isn't used now.

>layout/base/nsLayoutUtils.cpp
>+  nsRootPresContext* rootPresContext = presContext->GetRootPresContext();

I think I prefer an early return here if rootPresContext is null and
removing the later null-checks, would that work?

>layout/base/nsPresContext.cpp
>+nsRootPresContext::ComputePluginGeometryUpdates
> ...
>+  if (!rootFrame)
>+    return;

Missing {}

>+nsRootPresContext::InitApplyPluginGeometryTimer()
>+{
>+  if (mApplyPluginGeometryTimer)
>+    return;

Missing {}

>layout/base/nsPresContext.h
>+  bool NeedToComputePluginGeometeryUpdates()

s/Geometery/Geometry/

>layout/generic/nsObjectFrame.cpp
>-    configuration[0].mBounds.width = mRect.width;
>+    configuration->mBounds.width = NSAppUnitsToIntPixels(mRect.width, appUnitsPerDevPixel);

Nice.

> nsDisplayPlugin::ComputeVisibility(nsDisplayListBuilder* aBuilder,
>...
>+      for (const nsRect* r = iter.Next(); r; r = iter.Next()) {
>+        nsRect rAncestor =
>+          nsLayoutUtils::TransformFrameRectToAncestor(f, *r, ReferenceFrame());
>+        nsIntRect rPixels = r->ToNearestPixels(appUnitsPerDevPixel)
>+            - f->mNextConfigurationBounds.TopLeft();
>+        if (!rPixels.IsEmpty()) {
>+          f->mNextConfigurationClipRegion.AppendElement(rPixels);
>+        }
>+      }

You're not using rAncestor in this loop.
Did you mean rAncestor->ToNearestPixels ?

> nsDisplayPlugin::GetOpaqueRegion(nsDisplayListBuilder* aBuilder,
> ...
>+      // in ThebesLayers there.
>+  	   return result;

Indentation.
Attachment #669012 - Flags: review?(matspal) → review-
This works pretty well now that we're actually updating just before the composite.
Attachment #669859 - Flags: review?(matspal)
(In reply to Mats Palmgren [:mats] from comment #62)
> r- because this breaks rendering of zoomed pages on Linux -- the plugin
> does not change size, has wrong position, invalidation problems...
> (it seems to work on Win7 though)

I'll look into that.

> >layout/base/nsCSSFrameConstructor.cpp
> >-  if (didInvalidate && !didReflow) {
> 
> It looks like didInvalidate and didReflow isn't used now.

Removed.

> >layout/base/nsLayoutUtils.cpp
> >+  nsRootPresContext* rootPresContext = presContext->GetRootPresContext();
> 
> I think I prefer an early return here if rootPresContext is null and
> removing the later null-checks, would that work?

Yes. Done.

> You're not using rAncestor in this loop.
> Did you mean rAncestor->ToNearestPixels ?

Yes. Fixed. That might possibly fix the Linux issues.

Everything else fixed.
Attached patch Part 4 v2 (obsolete) — — Splinter Review 
This fixes the Linux issues. The problem was in nsDisplayPlugin::ComputeVisibility; visibleRegion was computed relative to the ReferenceFrame(), but then we transformed it via TransformFrameRectToAncestor as assuming it was relative to 'f'. This patch adds
      // Make visibleRegion relative to f
      visibleRegion.MoveBy(-ToReferenceFrame());
(The reason we do this little dance is because simply translating a appunits rectangle by an offset isn't enough if the source and destination coordinate systems have different AppUnitsPerDevPixel.)
Attachment #669012 - Attachment is obsolete: true
Attachment #669910 - Flags: review?(matspal)
The patches here now solve the at least the recent regressions in synchronization of windowed plugin scrolling.

Part 4 seems to have introduced an issue:
When viewing a page with windowed plugins, click on the File menu, and move the pointer over the menuitems to see them highlight.  The plugins flicker or disappear altogether while doing this.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #59)
> I'm not sure whether it's best to update plugin widgets just before or just
> after painting the window. Doing it before the paint introduces a delay
> between updating widgets and painting the window, equal to the time required
> to composite the window's layer tree. But doing it after the paint means we
> have to paint (recomposite) again almost immediately if any plugin widgets
> changed size or position, to fill in any strips in our main window that have
> been exposed by the plugin widget changes. Those extra paints could be
> costly especially when GPU-accelerated compositing is not available.

I didn't observe a noticeable difference on X11.

If we could mess with OS invalidation during moves, then theoretically we would need only one composite if moving plugin widgets before painting the window.
However, OS invalidations will cause another composite either way.  We deal with this by doing a sync update after the DidPaint, so either way there will not be much time between move and paint - they should both happen during the OS expose event.

With and without part 5, I did very occasionally see glitches for tenths of seconds (while playing video in other windows) where the plugin had been scrolled but the content had not yet been repainted.  I'm not sure I can explain this.  I assume we don't do a ForcedRepaint in this situation (which could lead to arbitrary other stuff happening).  Perhaps the X server was just having a rest, or decided to attend to another client amongst our requests (and I don't think we want to grab the server to deal with that).
Attachment #669910 - Attachment is patch: true
Comment on attachment 669910 [details] [diff] [review]
Part 4 v2

Looks good, but zoom on Linux is still broken.
(Linux64, debug, no HW accel, in case that matters)

The comment in nsRootPresContext::RequestUpdatePluginGeometry()
needs updating, it refers to nsPresShell::WillPaint/DidPaint.
Attachment #669910 - Flags: review?(matspal) → review-
Comment on attachment 669858 [details] [diff] [review]
Part 4.5: Forward WillPaintWindow/DidPaintWindow notifications to the presshell and do plugin geometry updates from there

The comment in nsViewManager::WillPaintWindow needs updating
"plugin widget geometry updates by calling WillPaint"
Attachment #669858 - Flags: review?(matspal) → review+
Attachment #669859 - Flags: review?(matspal) → review+
(In reply to Karl Tomlinson (:karlt) from comment #67)
> We deal
> with this by doing a sync update after the DidPaint, so either way there
> will not be much time between move and paint - they should both happen
> during the OS expose event.

GLX layers don't do the sync update after DidPaint and composite the entire window, and so benefit from moving during WillPaint.

D2D layers seem to do the sync update.  Perhaps that's unnecessary.  OS invalidations may cause another composite anyway, but it doesn't need to be immediate, if moving during WillPaint.
The try build shows that this NS_ERROR fires in a few testcases on Linux and Mac (different tests on different platforms though):

+PresShell::WillPaintWindow(bool aWillSendDidPaint)
+{
+  nsRootPresContext* rootPresContext = mPresContext->GetRootPresContext();
+  if (rootPresContext != mPresContext) {
+    NS_ERROR("Non-root prescontext was painted into a widget");

I will look into that, as well as the Linux zoom issue.
Comment on attachment 669858 [details] [diff] [review]
Part 4.5: Forward WillPaintWindow/DidPaintWindow notifications to the presshell and do plugin geometry updates from there

This part changes the nsIPresShell vtable -- please bump the UUID.
Attached patch Part 4 v3 — — Splinter Review 
Comments addressed.

Fixed the issue about plugins disappearing when a popup is showing. The problem was that we would create a display list for the popup, then compute plugin geometry from it; we'd set all the windowed plugins to be hidden, and then not find any of them in the display list. We need to only update plugin geometry for the plugins in the frame subtree we built the display list for, and this patch does that by adding the display list's root frame as a parameter to ComputePluginGeometryUpdates. (We can't have windowed plugins in a popup but we still do support windowless plugins there (although it's probably not tested, so might be broken), so I didn't want to just disable that outright. We need to run ComputePluginGeometryUpdates for windowless plugins to update their visibility state.)
Attachment #669910 - Attachment is obsolete: true
Attachment #670653 - Flags: review?(matspal)
The Linux zooming bug was because the nsLayoutUtils::Transform functions don't convert to the destination frame's appunits correctly.
Attachment #670654 - Flags: review?(matspal)
Attached patch Part 4.5 v2 — — Splinter Review 
We actually do call WillPaintWindow for non-root presshells, which is why the assertions that we don't were firing. That happens when a popup belongs to a non-root presshell.
Attachment #669858 - Attachment is obsolete: true
Attachment #670656 - Flags: review?(matspal)
Comment on attachment 670654 [details] [diff] [review]
Part 3.5: fix nsLayoutUtils::Transform functions

>+    NS_ASSERTION(rpc, "Can't convert root coordinates without a root pres context");
>+    float srcAppUnitsPerDevPixel =
>+      rpc ? rpc->AppUnitsPerDevPixel() : destAppUnitsPerDevPixel;

I don't feel strongly about it, but in general I prefer either MOZ_ASSERT
or a null-check (without NS_ASSERTION).  In this case just a null-check
since we know we can't always find the root pres context.

r=mats
Attachment #670654 - Flags: review?(matspal) → review+
Maybe we should overload the conversion functions (in a follow-up bug):
gfxPoint NSAppUnitsToFloatPixels(nsPoint,
(or perhaps "const nsPoint&" etc is faster?)
gfxSize  NSAppUnitsToFloatPixels(nsSize,
gfxRect  NSAppUnitsToFloatPixels(nsRect,
nsPoint NSFloatPixelsToAppUnits(gfxPoint,
nsSize  NSFloatPixelsToAppUnits(gfxSize,
nsRect  NSFloatPixelsToAppUnits(gfxRect,

It makes the code more readable and prevents typos (which are easy to miss
when reviewing).
Comment on attachment 670653 [details] [diff] [review]
Part 4 v3

The comment in nsRootPresContext::InitApplyPluginGeometryTimer()
needs updating, "(either from nsPresShell::WillPaint or from
nsPresShell::DidPaint, depending on the platform)"
should probably be "nsPresShell::WillPaintWindow".
Attachment #670653 - Flags: review?(matspal) → review+
Comment on attachment 670656 [details] [diff] [review]
Part 4.5 v2

A comment in nsViewManager::ProcessPendingUpdates() says
"plugin widget geometry updates by calling WillPaint"
Attachment #670656 - Flags: review?(matspal) → review+
(In reply to Mats Palmgren [:mats] from comment #78)
> Maybe we should overload the conversion functions (in a follow-up bug):
> gfxPoint NSAppUnitsToFloatPixels(nsPoint,
> (or perhaps "const nsPoint&" etc is faster?)
> gfxSize  NSAppUnitsToFloatPixels(nsSize,
> gfxRect  NSAppUnitsToFloatPixels(nsRect,
> nsPoint NSFloatPixelsToAppUnits(gfxPoint,
> nsSize  NSFloatPixelsToAppUnits(gfxSize,
> nsRect  NSFloatPixelsToAppUnits(gfxRect,

Methods on nsRect, probably
(In reply to Mats Palmgren [:mats] from comment #77)
> Comment on attachment 670654 [details] [diff] [review]
> Part 3.5: fix nsLayoutUtils::Transform functions
> 
> >+    NS_ASSERTION(rpc, "Can't convert root coordinates without a root pres context");
> >+    float srcAppUnitsPerDevPixel =
> >+      rpc ? rpc->AppUnitsPerDevPixel() : destAppUnitsPerDevPixel;
> 
> I don't feel strongly about it, but in general I prefer either MOZ_ASSERT
> or a null-check (without NS_ASSERTION).  In this case just a null-check
> since we know we can't always find the root pres context.

Actually, making this change to TransformRootPointToFrame caused a test failure (I think because nsLayoutUtils::GetEventCoordinatesRelativeTo, its only caller, uses it in a broken way), and it's not used by these patches, so I just took that change out.
Blocks: 797167
We should consider these changes for 18, because they fix the recent regression on 18 - bug 797167.
status-firefox18: --- → affected
tracking-firefox18: --- → ?
Blocks: 786970
Comment on attachment 670653 [details] [diff] [review]
Part 4 v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 539356
User impact if declined: plugins often appear to scroll out of sync with the rest of the page; looks super ugly on a lot of pages
Testing completed (on m-c, etc.): landed on m-c for a couple of days
Risk to taking this patch (and alternatives if risky): this is relatively risky, because it changes the timing of plugin widget modifications, which are always risky. However, we don't have any good alternatives.
String or UUID changes made by this patch: changes nsIPresShell UUID (in part 4.5).

Please consider this an approval request to uplift all the patches in this bug.
Attachment #670653 - Flags: approval-mozilla-aurora?
This is not completely fixed.  You can still observe it on youtube, if you "bounce-scroll" or rapidly scroll.
Depends on: 801763
(In reply to IU from comment #87)
> This is not completely fixed.  You can still observe it on youtube, if you
> "bounce-scroll" or rapidly scroll.

Unfortunately it's not really possible to completely fix it for windowed plugins. We simply can't make the update of the window and the movement of plugin widgets perfectly visually atomic.

We could probably make it a little better by having some kind of callback from the layer manger when it's just about to copy its backbuffer to the window, and move the plugins at that moment. We'd have to do something to ensure the area exposed by the plugins was part of the area composited by the layer manager.

Te most important question to me is whether there's any version of Firefox where it behaved better than it does now.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #88)
> We could probably make it a little better by having some kind of callback
> from the layer manger when it's just about to copy its backbuffer to the
> window, and move the plugins at that moment.

This would require some code restructuring because on Windows we'd have to issue BeginPaint during layer tree composition. We'd probably have to add a BeginPaint method to nsIWidget (no-op for all platforms but Windows), then call that method from the layer manager just before we start drawing to the window. On Mac that wouldn't work since we can't modify the widget hierarchy during the 'draw' callback, so we'd have to carry on moving the plugins in viewWillDraw/WillPaintWindow on Mac.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #88)
> Te most important question to me is whether there's any version of Firefox
> where it behaved better than it does now.

It was fine with the January 16, 2012 nightly build.  It regressed after that: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=047c8ba7d2e4&tochange=34572943a3e4
I should note that the fix here basically takes us back to the point after the original regression.  It does not fix the original regression, which still exists.

Looking through the bug list, from the regression range of comment 90, I can't quickly tell what the likely cause may be, and there are no tinderbox build for that range.
(In reply to IU from comment #90)
> It was fine with the January 16, 2012 nightly build.  It regressed after
> that:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=047c8ba7d2e4&tochange=34572943a3e4

Maybe
f7c8c8ee3fad	Jared Wein — Bug 198964 - Enable smooth scrolling by default. r=gavin
(In reply to Virtual_ManPL [:Virtual] from comment #92)
> Maybe
> f7c8c8ee3fad	Jared Wein — Bug 198964 - Enable smooth scrolling by default.
> r=gavin

That's it.  Disable smooth scrolling and the regression vanishes.
IU, please file a new bug for the smooth scrolling perf issue and make it
block bug 710372.  If you can capture a video of the problem that would
help too.  Thanks.  (Perhaps we should simply not do smooth scrolling on
pages with plugins if it makes us look sluggish).
It sounds like smooth scrolling makes us look worse but our actual rendering code wasn't better before that.
(In reply to Mats Palmgren [:mats] from comment #94)
> IU, please file a new bug for the smooth scrolling perf issue and make it
> block bug 710372.  If you can capture a video of the problem that would
> help too.  Thanks.  (Perhaps we should simply not do smooth scrolling on
> pages with plugins if it makes us look sluggish).

OK. Filed bug 802478
Verified on the 10/16 Nightly:
Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/19.0 Firefox/19.0.

I tested with the steps in comment 0, comment 47, bug 786970, and bug 797167.

On the 10/16 Aurora and on Firefox 17b1, the plugin content bounces very much out of it's container for all the test cases. 

On Nightly: 
- the plugin content of the test case in comment 47 doesn't move at all anymore,
- the plugin contents from the other test cases is still shaking a little when scrolling, but this shake is barely visible.
Comment on attachment 670653 [details] [diff] [review]
Part 4 v3

Approving this patch for aurora as the user impact from our testing is significant after DLBI landed  .
Attachment #670653 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 804606
Depends on: 805331
QA Contact: ioana.budnar
Whiteboard: verifyme
Depends on: 816896
Verified as in comment 97 on Firefox 18 beta 4 - 20121212073002:
Mozilla/5.0 (Windows NT 6.1; rv:18.0) Gecko/18.0 Firefox/18.0
status-firefox18: fixed → verified
Depends on: 820680
Depends on: 858979
Depends on: 888580
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: