Jim Mathies [:jimm] Reporter
	Description • 8 years ago

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

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 1 • 8 years ago

I don't understand what P1 and 47 affected means - this is blocking E10S with plug-ins?

	Jim Mathies [:jimm] Reporter
	Comment 2 • 8 years ago

No it's not blocking, but it's high on the priority list of work we want to try and tackle after rollout.

	Bob Owen (:bobowen) Assignee
	Comment 3 • 8 years ago

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.

	Bob Owen (:bobowen) Assignee
	Comment 4 • 8 years ago

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

	Bob Owen (:bobowen) Assignee
	Comment 5 • 8 years ago

You have to create the pref gfx.e10s.hide-plugins-for-scroll and set it to false, to turn off the hide on scroll.

	Jim Mathies [:jimm] Reporter
	Comment 7 • 8 years ago

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/

	Bob Owen (:bobowen) Assignee
	Comment 8 • 8 years ago

MozReview-Commit-ID: BeUFWZOwkvh

	Bob Owen (:bobowen) Assignee
	Comment 9 • 8 years ago

MozReview-Commit-ID: ElE0GD3tLah

	Bob Owen (:bobowen) Assignee
	Comment 10 • 8 years ago

MozReview-Commit-ID: DaG3Iasy1vY

	Bob Owen (:bobowen) Assignee
	Comment 11 • 8 years ago

MozReview-Commit-ID: 2tHtOxx7jKa

	Bob Owen (:bobowen) Assignee
	Comment 12 • 8 years ago

MozReview-Commit-ID: 6r4uQQqtwz9

	Bob Owen (:bobowen) Assignee
	Comment 13 • 8 years ago

MozReview-Commit-ID: EqAhU20Vkxm

	Bob Owen (:bobowen) Assignee
	Comment 14 • 8 years ago

Also, apply plugin updates when they arrive not just during composition.

MozReview-Commit-ID: FZJYiPqb5uZ

	Matt Woodrow (:mattwoodrow)
	Comment 15 • 8 years ago

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.

	Jim Mathies [:jimm] Reporter
	Comment 16 • 8 years ago

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

	Jim Mathies [:jimm] Reporter
	Comment 17 • 8 years ago

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.

	Bob Owen (:bobowen) Assignee
	Comment 18 • 8 years ago

(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.

	Barbara Bermes [:barbara] - NI please!
	Comment 19 • 8 years ago

(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?

	Jim Mathies [:jimm] Reporter
	Comment 20 • 8 years ago

(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.

	Bob Owen (:bobowen) Assignee
	Comment 21 • 8 years ago

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

	Matt Woodrow (:mattwoodrow)
	Comment 22 • 8 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 23 • 8 years ago

(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.

	Bob Owen (:bobowen) Assignee
	Comment 24 • 8 years ago

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.

	Barbara Bermes [:barbara] - NI please!
	Comment 25 • 8 years ago

(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

	Jim Mathies [:jimm] Reporter
	Comment 26 • 8 years ago

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?

	Bob Owen (:bobowen) Assignee
	Comment 27 • 8 years ago

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.

	Pulsebot
	Comment 28 • 8 years ago

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

	Barbara Bermes [:barbara] - NI please!
	Comment 29 • 8 years ago

(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.

	Jim Mathies [:jimm] Reporter
	Comment 30 • 8 years ago

(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.

	Carsten Book [:Tomcat]
	Comment 31 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f3b167f0505d
https://hg.mozilla.org/mozilla-central/rev/4010b27b7c25
https://hg.mozilla.org/mozilla-central/rev/499de859b4b9
https://hg.mozilla.org/mozilla-central/rev/b1c3f87e64f3
https://hg.mozilla.org/mozilla-central/rev/cc671a60392c
https://hg.mozilla.org/mozilla-central/rev/91de47bdb077
https://hg.mozilla.org/mozilla-central/rev/eb81358d1e47