Closed Bug 1252877 Opened 4 years ago Closed 3 years ago

Add support for taking plugin window captures at the start of a scroll

Categories

(Core :: Graphics, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
e10s + ---
firefox50 --- fixed

People

(Reporter: jimm, Assigned: bobowen)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted], sbwc1)

Attachments

(7 files, 1 obsolete file)

Currently on Windows we take captures shortly after load and after a scroll operation completes. With some additional plumbing in our layers system it might be possible to take a capture prior to the start of a scroll. A discussion I had with matt is copies below.

requesting triage to get a priority set.

--


<jimm> currently I'm grabbing a capture when I can, but we can't do it at the start of the scroll.
<jimm> we'd like to though, which means moving all the capture logic over to the chrome process main thread.
<mattwoodrow> jimm: I don’t think it’d be too hard
<mattwoodrow> You can’t access the layer from chrome, since it doesn’t exist there
<mattwoodrow> so you’d have to create the TextureClient for your image so that it’s shared with the compositor
<mattwoodrow> and then add some new ipc messages
<mattwoodrow> one from child -> chrome to get an identifier for the layer you want to attach it to
<mattwoodrow> and then one from chrome -> compositor to attach your TextureClient to the layer that matches that identifier
<jimm> mattwoodrow: ok so the identifier of the layer I want to attach to is over in the content process and is associated with the plugin frame. I guess that's the layer I'm currently creating in the content process... it has an identifier I'd need.
<jimm> how would I go about attaching a TextureClient to an remotely managed ImageLayer?
<mattwoodrow> I don’t know if it has an appropriate identifier at the moment
<jimm> mattwoodrow: should I create something other than an image layer in the content process? seems like I might be wasting resources by doing that.
<jimm> (creating an imaqe layer that nothing paints to)
<mattwoodrow> It won’t use any significant amount of memory
<mattwoodrow> I think you want it so you can set the position and size etc
<jimm> ok
<jimm> so attaching a TextureClient to this ImageLayer, is that something the layer system currently supports?
<mattwoodrow> yes
<jimm> can you point me at a use of that for an example?
<mattwoodrow> if you construct an ImageContainer with the ASYNCHRONOUS flag, then that should do all the TextureClient creation for you
<mattwoodrow> you just provide it with a layer::Image (one of the impl of Image is just a wrapper around SourceSurface)
<jimm> I'm already doing that now, although I'm not sure if I'm getting the ASYNCHRONOUS type
<mattwoodrow> jimm: ok, I think this works. Once you have your async ImageContainer in chrome, it should be sending the images to the compositor
<mattwoodrow> You’ll want to grab the value of GetAsyncContainerID() from that ImageContainer and send it down to content
<jimm> ok..
<mattwoodrow> What normally happens is that we’d call ClientImageLayer::SetContainer to attach the container to the layer
<mattwoodrow> ClientImageLayer::Render will then construct an ImageClientBridge (if the container was async) and call ImageClientBridge::UpdateImage
<mattwoodrow> and that sends the IPC message linking the async containers id with the layer
<mattwoodrow> You should be able to get those last few steps working with just the ID, not the ImageContainer*
<mattwoodrow> maybe adding a ClientImageLayer::SetRemoteContainer(uint64_t aAsyncContainerID) or similar
Priority: -- → P1
I don't understand what P1 and 47 affected means - this is blocking E10S with plug-ins?
Whiteboard: [gfx-noted]
No it's not blocking, but it's high on the priority list of work we want to try and tackle after rollout.
See Also: → 1257911
We want this to fix bug 1257911 for the sandbox.

After reading through the patches in bug 1232181, a bit of debugging and the explanation from Matt here, I think I have a very rough idea of what we need to do here, so I'll have a stab at this.
Assignee: nobody → bobowen.code
Status: NEW → ASSIGNED
Here's a try push that has both the plugin capture in the parent and the improved scrolling without hiding.
I've also cleaned up some things that I don't think we need in the child any more.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3cabf27615cf6e8d6f5726598cee73abaa2093d
You have to create the pref gfx.e10s.hide-plugins-for-scroll and set it to false, to turn off the hide on scroll.
See Also: 1257911
Duplicate of this bug: 1257911
Whiteboard: [gfx-noted] → [gfx-noted], sbwc1
Hey Barbara, I'm looking for a product decision here. With sandboxing we've had to re-architect the way we handle windowed plugins when scrolling with APZ. The current e10s/APZ behavior we're rolling out with can be seen in Nightly with the sandbox turned off:

Current behavior (Windows and Linux only):
1) Set security.sandbox.content.level = 0 and make sure e10s is enabled, restart. Do not test on a touch screen device.
2) visit any of the windowed plugin test cases below 
3) scroll using the mouse wheel, page, or arrow keys

What we current do is hide the plugin window during a scroll, and attempt to capture images of the window's content when we can and paint that in the windows place.

With sandboxing the capturing of the window content had to move to a different process, and we were also able to change the timing a bit to improve the effect. While working on this, bowen also worked on getting scrolling windows async without hiding them working. Both approaches have advantages and disadvantages.  Below is a try build with both features available, what I'd like to do is get your opinion on which method you think we should land on nightly and start testing with by default.

Note if we run into major issues with one approach, we can switch to the other. I'm looking for an initial call.

Download the zip build on Windows or Linux to test. 

Testing steps:
1) Set the pref 'plugins.rewrite_youtube_embeds' to false
2) test using captures, which is the default
3) set 'gfx.e10s.hide-plugins-for-scroll' = false, restart
4) test using async scrolling

For captures, I like the smoothness of the scroll, however we still have to put up with windows that fill in as they come into the view. See this example for a good test case:

http://www.stalker.pl/forum/viewtopic.php?f=9&t=134&start=6276

For scrolling, I see some jank depending on the content. Here's a good jank test case:

http://www.gplanet.co.il/

Misc windowed test cases here:

http://mathies.com/mozilla/testcases/
Flags: needinfo?(bbermes)
MozReview-Commit-ID: BeUFWZOwkvh
Attachment #8769677 - Flags: review?(matt.woodrow)
MozReview-Commit-ID: ElE0GD3tLah
Attachment #8769678 - Flags: review?(matt.woodrow)
Attachment #8769678 - Flags: review?(jmathies)
MozReview-Commit-ID: DaG3Iasy1vY
Attachment #8769679 - Flags: review?(jmathies)
MozReview-Commit-ID: 2tHtOxx7jKa
Attachment #8769680 - Flags: review?(jmathies)
MozReview-Commit-ID: 6r4uQQqtwz9
Attachment #8769681 - Flags: review?(jmathies)
MozReview-Commit-ID: EqAhU20Vkxm
Attachment #8769682 - Flags: review?(jmathies)
Also, apply plugin updates when they arrive not just during composition.

MozReview-Commit-ID: FZJYiPqb5uZ
Attachment #8769683 - Flags: review?(jmathies)
Attachment #8769683 - Flags: review?(dvander)
Comment on attachment 8769678 [details] [diff] [review]
Part 2: On Windows capture an image for windowed plugins to be displayed during APZ scroll

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

::: widget/windows/nsWindow.cpp
@@ +1214,5 @@
> +  }
> +  ::GdiFlush();
> +  snapshot->Flush();
> +
> +  return gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(nullptr,

When the drawing backend is skia (the default for unaccelerated windows now I believe), this is going to trigger an extra allocation and copy of the whole surface. The same is true for d2d, but that copy is into VRAM and probably what we want.

Ideally we'd allocate a DataSourceSurface, and then wrap a DC around that memory so the BitBlt writes directly into the right memory. Is that possible?

Even better we'd get the ImageContainer to allocate the TextureClient first (which allocates shmem) and write directly into that, skipping another copy.
Comment on attachment 8769678 [details] [diff] [review]
Part 2: On Windows capture an image for windowed plugins to be displayed during APZ scroll

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

generally looks good. there's an open question from matt about painting optimization that sounds like something we want to address if we can..

::: widget/nsBaseWidget.cpp
@@ +2195,5 @@
> +  if (!mScrollCaptureContainer || mBounds.width <= 0 || mBounds.height <= 0) {
> +    return;
> +  }
> +
> +  RefPtr<gfx::SourceSurface> snapshot = CreateScrollSnapshot();

lets add a comment here explaining that this can fail (often) due to window clipping.

::: widget/nsBaseWidget.h
@@ +367,5 @@
>    void Shutdown();
>  
> +#if defined(XP_WIN)
> +  uint64_t CreateScrollCaptureContainer() override;
> +#endif  

nit - whitespace
Attachment #8769678 - Flags: review?(jmathies) → review+
Attachment #8769679 - Flags: review?(jmathies) → review+
Attachment #8769680 - Flags: review?(jmathies) → review+
Comment on attachment 8769681 [details] [diff] [review]
Part 5: Fix use of gfx.e10s.hide-plugins-for-scroll

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

I don't think this pref was ever hooked up right.
Attachment #8769681 - Flags: review?(jmathies) → review+
Attachment #8769682 - Flags: review?(jmathies) → review+
Attachment #8769683 - Flags: review?(jmathies) → review+
(In reply to Jim Mathies [:jimm] from comment #16)

> generally looks good. there's an open question from matt about painting
> optimization that sounds like something we want to address if we can..

Thanks for all the reviews Jim.
Yes, I think I can make sure we only have one copy (on top of the BitBlt).
Trying to BitBlt straight into the TextureClient would require quite a bit of extra plumbing, which I don't think is worth it for something we want to get rid of soon.

However, while I was looking at this I realised that sometimes we can end up using a partial capture as fallback, when we shouldn't, so I'm re-working that bit, should have an updated patch 2 tomorrow.
(In reply to Jim Mathies [:jimm] from comment #7)
> Hey Barbara, I'm looking for a product decision here. With sandboxing we've
> had to re-architect the way we handle windowed plugins when scrolling with
> APZ. The current e10s/APZ behavior we're rolling out with can be seen in
> Nightly with the sandbox turned off:
> 
> Current behavior (Windows and Linux only):
> 1) Set security.sandbox.content.level = 0 and make sure e10s is enabled,
> restart. Do not test on a touch screen device.
> 2) visit any of the windowed plugin test cases below 
> 3) scroll using the mouse wheel, page, or arrow keys
> 
> What we current do is hide the plugin window during a scroll, and attempt to
> capture images of the window's content when we can and paint that in the
> windows place.
> 
> With sandboxing the capturing of the window content had to move to a
> different process, and we were also able to change the timing a bit to
> improve the effect. While working on this, bowen also worked on getting
> scrolling windows async without hiding them working. Both approaches have
> advantages and disadvantages.  Below is a try build with both features
> available, what I'd like to do is get your opinion on which method you think
> we should land on nightly and start testing with by default.
> 
> Note if we run into major issues with one approach, we can switch to the
> other. I'm looking for an initial call.
> 
> Download the zip build on Windows or Linux to test. 
> 
> Testing steps:
> 1) Set the pref 'plugins.rewrite_youtube_embeds' to false
> 2) test using captures, which is the default
> 3) set 'gfx.e10s.hide-plugins-for-scroll' = false, restart
> 4) test using async scrolling
> 
> For captures, I like the smoothness of the scroll, however we still have to
> put up with windows that fill in as they come into the view. See this
> example for a good test case:
> 
> http://www.stalker.pl/forum/viewtopic.php?f=9&t=134&start=6276
> 
> For scrolling, I see some jank depending on the content. Here's a good jank
> test case:
> 
> http://www.gplanet.co.il/
> 
> Misc windowed test cases here:
> 
> http://mathies.com/mozilla/testcases/


I'm on a Mac, is there a remote machine I can log into or virtual box somewhere?
Flags: needinfo?(bbermes) → needinfo?(jmathies)
(In reply to Barbara Bermes [:barbara] from comment #19)
> I'm on a Mac, is there a remote machine I can log into or virtual box
> somewhere?

Not sure, I have local hardware. You're in Toronto right? Is Jeff in the same office? I think he has a Surface Book for testing that he might be able to lend you.
Flags: needinfo?(jmathies) → needinfo?(jgriffiths)
As I mentioned I realised that with the old code we would sometimes end up using a partial snapshot as a fallback incorrectly.
This meant I had to change things around, so that the derived class was responsible for the fallback as well.
I also changed it, so we only recreate the gfxWindowsSurfaces when we need to.

(In reply to Matt Woodrow (:mattwoodrow) from comment #15)

> > +  return gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(nullptr,
> 
> When the drawing backend is skia (the default for unaccelerated windows now
> I believe), this is going to trigger an extra allocation and copy of the
> whole surface. The same is true for d2d, but that copy is into VRAM and
> probably what we want.
> 
> Ideally we'd allocate a DataSourceSurface, and then wrap a DC around that
> memory so the BitBlt writes directly into the right memory. Is that possible?

I've changed this so we're creating a Cairo SourceSurface, which I think means we only copy in the image container.

> Even better we'd get the ImageContainer to allocate the TextureClient first
> (which allocates shmem) and write directly into that, skipping another copy.

This would be nice, but it looked like it would take a fair amount of extra work to create the TexturClient upfront and also to get an HDC from it that we could copy into.
I'm not sure this is worth it, given that we want to get rid of windowed plugins soon anyway.


(In reply to Jim Mathies [:jimm] from comment #16)

> > +  RefPtr<gfx::SourceSurface> snapshot = CreateScrollSnapshot();
> 
> lets add a comment here explaining that this can fail (often) due to window
> clipping.

The Windows specific code has more comments about this now.
It's possible that other platforms wouldn't have this issue, so I don't want to comment about it here.

> > +#endif  
> 
> nit - whitespace

Fixed, thanks.


MozReview-Commit-ID: ElE0GD3tLah
Attachment #8770968 - Flags: review?(matt.woodrow)
Attachment #8770968 - Flags: review?(jmathies)
Attachment #8769678 - Attachment is obsolete: true
Attachment #8769678 - Flags: review?(matt.woodrow)
Attachment #8769677 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8770968 [details] [diff] [review]
Part 2: On Windows capture an image for windowed plugins to be displayed during APZ scroll

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

Looks great!

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +2616,5 @@
>    mPluginWindowsHidden = true;
> +
> +#if defined(XP_WIN)
> +  // We will get an async reply that this has happened and then send hide.
> +  Unused << SendCaptureAllPlugins(parentWidget);

Passing pointers through ipdl is pretty sad, this might break with out of process compositing.

::: widget/windows/nsWindow.cpp
@@ +1219,5 @@
> +    clip.right = mBounds.width;
> +    clip.bottom = mBounds.height;
> +    return GetFallbackScrollSnapshot(clip);
> +  }
> +    

Trailing whitespace, here and a few other places.
Attachment #8770968 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #22)
> ::: gfx/layers/ipc/CompositorBridgeParent.cpp
> @@ +2616,5 @@
> >    mPluginWindowsHidden = true;
> > +
> > +#if defined(XP_WIN)
> > +  // We will get an async reply that this has happened and then send hide.
> > +  Unused << SendCaptureAllPlugins(parentWidget);
> 
> Passing pointers through ipdl is pretty sad, this might break with out of
> process compositing.

It should be safe since the pointer is just making a round trip.
Try push with the latest patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7af6458dad4c

Builds are complete, I think that a try issue is holding up the tests on Mac and Windows.
(In reply to Jim Mathies [:jimm] from comment #20)
> (In reply to Barbara Bermes [:barbara] from comment #19)
> > I'm on a Mac, is there a remote machine I can log into or virtual box
> > somewhere?
> 
> Not sure, I have local hardware. You're in Toronto right? Is Jeff in the
> same office? I think he has a Surface Book for testing that he might be able
> to lend you.

I'll ask jlin. Thanks
Flags: needinfo?(jlin)
Flags: needinfo?(bbermes)
Comment on attachment 8770968 [details] [diff] [review]
Part 2: On Windows capture an image for windowed plugins to be displayed during APZ scroll

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

nice work.

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +2616,5 @@
>    mPluginWindowsHidden = true;
> +
> +#if defined(XP_WIN)
> +  // We will get an async reply that this has happened and then send hide.
> +  Unused << SendCaptureAllPlugins(parentWidget);

This is an HWND, so it should be ok.

::: widget/nsBaseWidget.cpp
@@ +2199,5 @@
> +  if (!mScrollCaptureContainer || mBounds.width <= 0 || mBounds.height <= 0) {
> +    return;
> +  }
> +
> +  RefPtr<gfx::SourceSurface> snapshot = CreateScrollSnapshot();

nit - comment me, this can fail often.

::: widget/windows/nsWindow.cpp
@@ +1186,5 @@
>                      (LPARAM)aCallback);
>  }
>  
> +static already_AddRefed<SourceSurface>
> +CreateSourceSurfaceForGfxSurface(gfxASurface* aSurface)

nit - assert on null aSurface

@@ +1260,5 @@
> +{
> +  gfx::IntSize snapshotSize(mBounds.width, mBounds.height);
> +
> +  // If the current snapshot is the correct size and covers the required clip,
> +  // just keep that by returning null?

did you intend to leave this phrased as a question?
Attachment #8770968 - Flags: review?(jmathies) → review+
Attachment #8769683 - Flags: review?(dvander) → review+
Thanks all, for the reviews.
Changes made locally.

I'm going to land this as is, as it fixes and improves the scroll capture when sandboxed and this is the status quo.
If we decide we want to go with not hiding then we can just flip the pref afterwards.

(In reply to Matt Woodrow (:mattwoodrow) from comment #22)
> Comment on attachment 8770968 [details] [diff] [review]

> ::: widget/windows/nsWindow.cpp
> > +    return GetFallbackScrollSnapshot(clip);
> > +  }
> > +    
> 
> Trailing whitespace, here and a few other places.

Fixed, thanks.

(In reply to Jim Mathies [:jimm] from comment #26)
> Comment on attachment 8770968 [details] [diff] [review]

> > +  RefPtr<gfx::SourceSurface> snapshot = CreateScrollSnapshot();
> 
> nit - comment me, this can fail often.

Comment added.
 
> ::: widget/windows/nsWindow.cpp
> @@ +1186,5 @@
> >                      (LPARAM)aCallback);
> >  }
> >  
> > +static already_AddRefed<SourceSurface>
> > +CreateSourceSurfaceForGfxSurface(gfxASurface* aSurface)
> 
> nit - assert on null aSurface

Added, thanks.

> @@ +1260,5 @@
> > +{
> > +  gfx::IntSize snapshotSize(mBounds.width, mBounds.height);
> > +
> > +  // If the current snapshot is the correct size and covers the required clip,
> > +  // just keep that by returning null?
> 
> did you intend to leave this phrased as a question?

No, I re-worded after a change to the code and missed removing it, thanks.
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3b167f0505d
Part 1: Allow ImageContainer to act as a proxy for another async ImageConatiner. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/4010b27b7c25
Part 2: On Windows capture an image for windowed plugins to be displayed during APZ scroll. r=jimm, r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/499de859b4b9
Part 3: Remove plugin capture code from child. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1c3f87e64f3
Part 4: Remove notification of plugins about scrolling from child. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc671a60392c
Part 5: Fix use of gfx.e10s.hide-plugins-for-scroll. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/91de47bdb077
Part 6: Fix plugin frame check for paint skipping and short circuit the search for plugin frames. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb81358d1e47
Part 7: Wait for plugin updates during active animations. r=dvander, r=jimm
(In reply to Jim Mathies [:jimm] from comment #7)
> Hey Barbara, I'm looking for a product decision here. With sandboxing we've
> had to re-architect the way we handle windowed plugins when scrolling with
> APZ. The current e10s/APZ behavior we're rolling out with can be seen in
> Nightly with the sandbox turned off:

> 
> Note if we run into major issues with one approach, we can switch to the
> other. I'm looking for an initial call.
> 
> Download the zip build on Windows or Linux to test. 
> 
> Testing steps:
> 1) Set the pref 'plugins.rewrite_youtube_embeds' to false
> 2) test using captures, which is the default
> 3) set 'gfx.e10s.hide-plugins-for-scroll' = false, restart
> 4) test using async scrolling
> 



Jim, I got a Windows PC from IT, and installed and testes as directed.

To be honest, I didn't see much difference between the two versions but it seemed to me that the default (captures) "felt" (as a user not knowing any of that) better. So if there is no eng pushback/major drawback, I'd vote for that.

Scrolling back at the code reviews, not sure if you actually gone ahead with the async scrolling solution after all? Please confirm.
Flags: needinfo?(jmathies)
Flags: needinfo?(bobowen.code)
Flags: needinfo?(bbermes)
(In reply to Barbara Bermes [:barbara] from comment #29)
> (In reply to Jim Mathies [:jimm] from comment #7)
> > Hey Barbara, I'm looking for a product decision here. With sandboxing we've
> > had to re-architect the way we handle windowed plugins when scrolling with
> > APZ. The current e10s/APZ behavior we're rolling out with can be seen in
> > Nightly with the sandbox turned off:
> 
> > 
> > Note if we run into major issues with one approach, we can switch to the
> > other. I'm looking for an initial call.
> > 
> > Download the zip build on Windows or Linux to test. 
> > 
> > Testing steps:
> > 1) Set the pref 'plugins.rewrite_youtube_embeds' to false
> > 2) test using captures, which is the default
> > 3) set 'gfx.e10s.hide-plugins-for-scroll' = false, restart
> > 4) test using async scrolling
> > 
> 
> 
> 
> Jim, I got a Windows PC from IT, and installed and testes as directed.
> 
> To be honest, I didn't see much difference between the two versions but it
> seemed to me that the default (captures) "felt" (as a user not knowing any
> of that) better. So if there is no eng pushback/major drawback, I'd vote for
> that.
> 
> Scrolling back at the code reviews, not sure if you actually gone ahead with
> the async scrolling solution after all? Please confirm.

It's in there but disabled by default. Once this lands in nightly we can test again.
Flags: needinfo?(jmathies)
Flags: needinfo?(jlin)
Flags: needinfo?(jgriffiths)
Flags: needinfo?(bobowen.code)
You need to log in before you can comment on or make changes to this bug.