Closed Bug 913503 Opened 11 years ago Closed 11 years ago

Windowed plugin lag behind with OMTC

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29

People

(Reporter: BenWa, Assigned: mattwoodrow)

References

Details

Attachments

(1 file)

Flush plugin geometry changes before scheduling a composite
27.47 KB, patch
roc
: review+
Details | Diff | Splinter Review 
Windowed plugins already lag with scrolling but it gets much worse with OMTC. We need to find a solution for this before shipping.
Can we get the plugin window handle, host it in the layer tree somehow (special image layer?) and on composite we can have the compositor update the plugin' window position with up to date async position? Matt?
Flags: needinfo?(matt.woodrow)
Progress on this:
- nsObjectFrame appears to have its own plugin window
- PluginInstanceChild receives the plugin window from content and creates a child plugin window which has the widget plugin window as its parent

We should be good to move the parent widget plugin window to the layer tree. Just have to sync access to that window between the main thread and the widget where appropriate.
CC Roc & Benjamin in case they have insight here.
OS: Mac OS X → Windows 7
Do you have a test case?

I can't see why OMTC would be any different from main-thread compositing. Do you mean async scrolling with APZC?
Flags: needinfo?(matt.woodrow)
No, but I imagine it will be worse then. It's worse with off main thread compositing because the time where we move the plugin window and where we composite it is further away.

Test case:
1) Using windows 7, I forgot if this is with or without DWM.
2) Turn on OMTC and force basic.
3) Scroll with youtube. On my machine it was 10 to 20 pixels off.
I'm confused about the phrase "sync access to that window between the main thread and the widget".

We move plugins by moving/resizing the widget created by the nsObjectFrame and subsequently send an RPC message (NPP_SetWindow) to the plugin/OOPP plugin host to inform it of the new size.
I'm not talking about resizing here. While scrolling a page where the windowed win32 plugin doesn't move relative to the top left of the document origin (say the plugins top,left property) NPP_SetWindow is NOT called contrary to mac windowless plugins.

It appears that while scrolling the plugin's parent window is moved from the main thread of the content process and that the plugin is not notified of scrolling. Perhaps I'm wrong but I couldn't find any IPC message traffic carrying this information.
I believe that the plugin's parent window should be being moved from the main thread of the chrome process. If you're just moving the parent widget, you don't need to call NPP_SetWindow because neither the position (always 0,0) nor the size if changing.
Yes I agree. To keep the parent window moving with OMTC and async scrolling I think the best way to do this is to have the window in our layer tree and update the position of that window (but not draw using it).
I'm confused about comment 9. Comment 8 describes the way I believe it currently works.
I think BenWa is right in comment #9. Probably the best way to do this would be to create layers for windowed plugins and extend the Layers API with a callback we can call from the compositor thread (or the main thread, for main-thread compositing) when the layer is composited, passing in the window coordinates of the layer, so we can update the plugin position in that callback (assuming it's OK to move the HWND off the main thread ... we'll probably need a lock somewhere to avoid moving/resizing/destroying the HWND from the main thread at the same time as the compositor thread touches it). This callback would basically do what PresShell::WillPaintWindow() does; however we'd have to rework things so no prescontexts, presshells etc are touched, since they aren't thread-safe. This means extracting all the needed data into a stand-alone threadsafe object.

Getting async scrolling to work with windowed plugins would be a lot harder, because it would mean updating plugin positions and clip regions based on async scrolling (and other effects like position:sticky etc). We might be better off just disabling async scrolling while a windowed plugin is visible.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> Getting async scrolling to work with windowed plugins would be a lot harder,
> because it would mean updating plugin positions and clip regions based on
> async scrolling (and other effects like position:sticky etc). We might be
> better off just disabling async scrolling while a windowed plugin is visible.

Why would async scrolling be any harder to handle than regular scrolling if we extend the plugin layer to host the HWND and let it move/clip the parent plugin window? I don't understand what the additional complication is. I'm not sure about position:sticky and how that will work with async scrolling in general.

For things like layers masks I don't know how we handle that with windowed plugins currently. But if we hosted the HWND in the layer tree we could read back via GetDC-like calls if the plugin layer has a mask taking a perf hit but getting correct rendering.
(In reply to Benoit Girard (:BenWa) from comment #12)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> > Getting async scrolling to work with windowed plugins would be a lot harder,
> > because it would mean updating plugin positions and clip regions based on
> > async scrolling (and other effects like position:sticky etc). We might be
> > better off just disabling async scrolling while a windowed plugin is visible.
> 
> Why would async scrolling be any harder to handle than regular scrolling if
> we extend the plugin layer to host the HWND and let it move/clip the parent
> plugin window? I don't understand what the additional complication is. I'm
> not sure about position:sticky and how that will work with async scrolling
> in general.

We'd have to stuff everything we need to properly clip the plugin into the layers system. This includes opaque regions for each layer and information about cliprects.

> For things like layers masks I don't know how we handle that with windowed
> plugins currently. But if we hosted the HWND in the layer tree we could read
> back via GetDC-like calls if the plugin layer has a mask taking a perf hit
> but getting correct rendering.

That wouldn't work because you can only reliably read back the part of the plugin window that's visible, and you can't draw over the top of that part.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> We'd have to stuff everything we need to properly clip the plugin into the
> layers system. This includes opaque regions for each layer and information
> about cliprects.

Aren't these regions already in the layer system like the visible rect and the mask layer? I don't follow what the extra complication is.

> 
> > For things like layers masks I don't know how we handle that with windowed
> > plugins currently. But if we hosted the HWND in the layer tree we could read
> > back via GetDC-like calls if the plugin layer has a mask taking a perf hit
> > but getting correct rendering.
> 
> That wouldn't work because you can only reliably read back the part of the
> plugin window that's visible, and you can't draw over the top of that part.

True. This may no longer be true with DWM, I think it's worth checking.
I'm not sure it can be made safe to move the plugin widget from the compositor thread. The process of moving a window is done through window messages (processed by the main thread). If you call SetWindowPos with the normal flags, the compositor thread will block on the main thread while it sends the WM_MOVE event (and we'd have to think through the deadlock issues with that). If we call SetWindowPos with SWP_ASYNCWINDOWPOS, then the compositor thread won't block on the main thread, but the window won't actually move until the main thread processes the event.
(In reply to Benoit Girard (:BenWa) from comment #14)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> > We'd have to stuff everything we need to properly clip the plugin into the
> > layers system. This includes opaque regions for each layer and information
> > about cliprects.
> 
> Aren't these regions already in the layer system like the visible rect and
> the mask layer? I don't follow what the extra complication is.

They aren't. We can mark entire layers as opaque, but we can't mark subregions of a layer as opaque.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #15)
> I'm not sure it can be made safe to move the plugin widget from the
> compositor thread. The process of moving a window is done through window
> messages (processed by the main thread). If you call SetWindowPos with the
> normal flags, the compositor thread will block on the main thread while it
> sends the WM_MOVE event (and we'd have to think through the deadlock issues
> with that). If we call SetWindowPos with SWP_ASYNCWINDOWPOS, then the
> compositor thread won't block on the main thread, but the window won't
> actually move until the main thread processes the event.

Hmm. This is going to be tough.
Maybe, during an nsRefreshDriver tick, if the paint causes pending plugin window updates, the refresh driver should trigger a sync composite in such a way that the compositor thread can synchronously call back into the main thread to apply the plugin window updates during its composition. I guess that might be difficult to hack into IPDL but I'm not sure.
Wouldn't IPDLs RPC semantics fit that requirement?
See Also: → 923746
:johns you should probably dupe your bug against this. The OMTC fix will cover e10s. The major difference between e10s and OMTC are the setup code differs and create a ThreadLink instead of a ProcessLink IPC channel. Typically most OMTC bugs will cover e10s.
(In reply to Benoit Girard (:BenWa) from comment #19)
> :johns you should probably dupe your bug against this. The OMTC fix will
> cover e10s. The major difference between e10s and OMTC are the setup code
> differs and create a ThreadLink instead of a ProcessLink IPC channel.
> Typically most OMTC bugs will cover e10s.

Do you know that the fix here will make windowed mode plugins fully work in e10s? They currently entirely fail to render, rather than the slow-updates described here.
I'm sure there's more work to do to get windowed plugins working in e10s.
I'm taking a look at this.

What's supposed to happen is we call nsPresContext::ApplyPluginGeometryUpdates from WillPaintWindow at the start of the paint event (where we composite our layers with non-OMTC).

With OMTC we don't get paint events, so this never gets called. Instead plugins are updated when the fallback timer expires (setup by InitApplyPluginGeometryTimer at the end of painting), which has a 2 frame delay).

I tried doing the plugin update directly during painting (rather than just starting the timer). This could potentially mean the plugin would actually get ahead of the content, but we should be able to get our content composited during the same screen refresh interval and not have issues.

This seems to get the plugin moved to the correct spot, but the area that was previously covered by the plugin still has the old content. I haven't figured out why this is yet.

I guess either our content is being clipped out, or the plugin window is doing weird things.
Assignee: nobody → matt.woodrow
Maybe bad things happen when moving the plugin window races with the compositing happening on the other thread?
Blocks: 899785
I tried using synchronous window updates (from bug 930793) to ensure that the plugin gets moved before we composite the main window. No change.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> Maybe bad things happen when moving the plugin window races with the
> compositing happening on the other thread?

I think that is indeed the issue.

I changed the ordering around so that we flush the plugin updates, and then synchronously request a composite. That seems to work and match non-OMTC behaviour.

I doubt it actually needs to be synchronous, we just need to make sure it doesn't start until after we've finished flushing the plugin. I'll try that tomorrow.
This seems to get us behaviour that matches non-OMTC.

It just assumes that any scheduled composite will make it into the same screen refresh interval, and flushes plugin geometry changes immediately.
Attachment #8346356 - Flags: review?(roc)
Comment on attachment 8346356 [details] [diff] [review]
Flush plugin geometry changes before scheduling a composite

Review of attachment 8346356 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.h
@@ +219,5 @@
>    enum EndTransactionFlags {
>      END_DEFAULT = 0,
>      END_NO_IMMEDIATE_REDRAW = 1 << 0,  // Do not perform the drawing phase
> +    END_NO_COMPOSITE = 1 << 1, // Do not composite after drawing thebes layer contents.
> +    END_NO_REMOTE_COMPOSITE = 1 << 2

Document this!

@@ +226,5 @@
>    FrameLayerBuilder* GetLayerBuilder() {
>      return reinterpret_cast<FrameLayerBuilder*>(GetUserData(&gLayerManagerLayerBuilder));
>    }
>  
> +  virtual void Composite() {}

And this!

::: gfx/layers/ipc/CompositorParent.cpp
@@ +491,5 @@
>    }
>  }
>  
>  void
> +CompositorParent::NotifyShadowTreeTransaction(uint64_t aId, bool aIsFirstPaint, bool scheduleComposite)

aScheduleComposite

@@ +663,5 @@
>  void
>  CompositorParent::ShadowLayersUpdated(LayerTransactionParent* aLayerTree,
>                                        const TargetConfig& aTargetConfig,
> +                                      bool isFirstPaint,
> +                                      bool scheduleComposite)

aIsFirstPaint, aScheduleComposite

::: layout/base/nsLayoutUtils.cpp
@@ +2378,5 @@
> +    // We could instead have the compositor send back an equivalent to WillPaintWindow,
> +    // but it should be close enough to now not to matter.
> +    if (layerManager && !layerManager->NeedsWidgetInvalidation()) {
> +      rootPresContext->ApplyPluginGeometryUpdates();
> +    }

If the compositor has an async animation running, how do we ensure that a composite doesn't get scheduled to race with the plugin HWND update here?

::: layout/ipc/RenderFrameParent.cpp
@@ +731,5 @@
>  void
>  RenderFrameParent::ShadowLayersUpdated(LayerTransactionParent* aLayerTree,
>                                         const TargetConfig& aTargetConfig,
> +                                       bool isFirstPaint,
> +                                       bool scheduleComposite)

aIsFirstPaint, aScheduleComposite

::: layout/ipc/RenderFrameParent.h
@@ +78,5 @@
>  
>    virtual void ShadowLayersUpdated(LayerTransactionParent* aLayerTree,
>                                     const TargetConfig& aTargetConfig,
> +                                   bool isFirstPaint,
> +                                   bool scheduleComposite) MOZ_OVERRIDE;

here too
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> 
> ::: layout/base/nsLayoutUtils.cpp
> @@ +2378,5 @@
> > +    // We could instead have the compositor send back an equivalent to WillPaintWindow,
> > +    // but it should be close enough to now not to matter.
> > +    if (layerManager && !layerManager->NeedsWidgetInvalidation()) {
> > +      rootPresContext->ApplyPluginGeometryUpdates();
> > +    }
> 
> If the compositor has an async animation running, how do we ensure that a
> composite doesn't get scheduled to race with the plugin HWND update here?

Nothing!

I don't know if that matters much though. The async animation will be compositing at the same scroll position, so it shouldn't really be any different to leaving the existing content there though.

I guess the risk is that the async animation composition takes up time, causing the following scroll composition to miss the screen refresh interval and gets bumped to the following one. Then we end up showing the plugin scrolling *ahead* of the gecko content for a single frame.

If we start blocking compositions on vsync then it would make this worse.

I think we'll have to disable APZC when we have windowed plugins too, once we get to that point.
Comment on attachment 8346356 [details] [diff] [review]
Flush plugin geometry changes before scheduling a composite

Review of attachment 8346356 [details] [diff] [review]:
-----------------------------------------------------------------

OK
Attachment #8346356 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/a418bb88fa4d
https://hg.mozilla.org/mozilla-central/rev/ed71fd2a3a2d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: