Closed Bug 1232181 Opened 8 years ago Closed 8 years ago

Replace plugin windows with static window captures when scrolling

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
e10s m8+ ---
firefox45 --- affected
firefox46 --- wontfix
firefox47 --- fixed

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(8 files, 5 obsolete files)

1005 bytes, text/html
Details
1.20 KB, text/html
Details
60.26 KB, patch
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
bugzilla
: review+
Details
58 bytes, text/x-review-board-request
bugzilla
: review+
Details
Currently when we scroll a page with windowed plugins on linux and Windows, we hide plugin windows during the scroll. This can cause a flickering effect when the background of the page and plugin do not match well. We would like to try to improve this by:

1) inserting a new image layer under each plugin window
2) before a scroll, grab a screen capture of each plugin window in the visible tab
3) paint that capture to the underlying image layer
4) hide the plugin window
Blocks: 1232184
Assignee: nobody → jmathies
No longer blocks: 1232184
Attached patch wip (obsolete) — Splinter Review
Windows implementation wip.

Currently this updates the image in the image layer under the plugin window when we build the layer tree via nsPluginFrame::BuildLayer. This doesn't happen often so the capture gets stale. This layer needs to be set up such that I can swap a new Image outside of the building process, and add some sort of timer or trigger to the plugin instance to regularly update the capture.
There's one short fall to this approach that I've noticed. If the plugin window is outside the viewport of content, the window is clipped completely. With these windows the plugin doesn't have a surface to paint to, and we don't have a window surface to capture.

This means that some plugins (most actually if we're talking about ads) will not have captures if they are scrolled into view from outside the view port.

There's a similar problem with windows that fall on the edge of the viewport.
I've been experimenting with ways to get flash to paint to an offscreen surface but unfortunately I'm not having any luck.

The clipping problem degrades things such that this work isn't worth landing.

I'll keep poking a bit longer but at this point I'd say plugin window captures are off the table as a potential work around.
I had one other idea, it's kind of old school - 

https://msdn.microsoft.com/en-us/library/dd162869.aspx

This appears to return valid bits from the plugin. The notes on the api are a little scary but I'd hope that flash wouldn't try to open any print dialogs or printer drivers or anything crazy in respond to a WM_PRINT message. We can contact them to see. I'm also not sure what the overhead is here, it's probably pretty heavy. We're in the plugin process though so maybe that isn't such a big deal.

I'll keep experimenting.
Attached patch wip (obsolete) — Splinter Review
Latest wip. I'm still not getting good screen captures from flash for instances that are clipped.
Attachment #8713382 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
removed some debug code
Attachment #8717912 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Attachment #8717915 - Attachment is obsolete: true
current status here - we still don't have a way to capture a good window image if the underlying plugin window is clipped. We can capture images whenever the window is visible in the view port.
Attached file test case #1
Attached file test case #2
(In reply to Jim Mathies [:jimm] from comment #10)
> windows builds:
> http://archive.mozilla.org/pub/firefox/try-builds/jmathies@mozilla.com-
> 7374a2294522745feb44d15a676313739db94ee8/try-win32/

After fiddling with this all day my conclusion is that flash-based internet video is pretty terrible generally but that the big three sources of content ( FB, Youtube, Netflix ) no longer use flash for most users. 

I had a hard time reproducing what I think is the intended effect on a windows machine I have but I think that's due to the Windows machine.

If I think of this in terms of an actual scenario:

* for ads, as a user I don't care really if the ad animation seems jumpy / janked as I scroll. having *something* that is visually similar to what the plugin was showing as I scroll is better than a black square.

* for video, it's intuitively more jarring for users to have the image frozen as they scroll. I think the jarring-ness depends heavily on how recent the image is that we captured, and this problem seems rife with complication:

  - few videos will actually look the same when the user scrolls than they did when the plugin loaded
  - you can easily imagine perf problems arising if we sample the output of the plugin every frame or even every second
  - any value we choose between occasional sampling and realtime sampling will decrease the experience for the user

Blassey mentioned in an email thread that getting an async rendering feature from Adobe is the ultimate solution but could take a while? 

Jim: do you think we could create an image and cache it every second or so from the plugin?

Also, to the question of offscreen plugins not being able to be sampled, I think for a user this is not terrible, the scenario is instead that the user has a video playing but they can't see it - if they scroll it into view they will initially see say a black, blank rectangle but then when they stop scrolling the video will appear - correct? In my mind this is, at least for the time being, acceptable.
Flags: needinfo?(jgriffiths) → needinfo?(jmathies)
> * for ads, as a user I don't care really if the ad animation seems jumpy /
> janked as I scroll. having *something* that is visually similar to what the
> plugin was showing as I scroll is better than a black square.

FYI, ads and video are the same to us, we don't differentiate. The window capture code either paints a capture or a gray background in these builds. If you see a black square what you're seeing is a capture that was made before the plugin painted content, and the background color of the plugin window happened to be black. A common situation. We can avoid this by extending out the delay for first capture, although we'll end up with more gray background since users tend to start scrolling soon after a page load.

> * for video, it's intuitively more jarring for users to have the image
> frozen as they scroll. I think the jarring-ness depends heavily on how
> recent the image is that we captured, and this problem seems rife with
> complication:
> 
>   - few videos will actually look the same when the user scrolls than they
> did when the plugin loaded

Yes this is complete random. You never know what you'll get in the capture.

>   - you can easily imagine perf problems arising if we sample the output of
> the plugin every frame or even every second

Currently I do not try to do this specifically for this reason. Measuring what the impact would be might be tough but we could try to put something together.

>   - any value we choose between occasional sampling and realtime sampling
> will decrease the experience for the user
> 
> Blassey mentioned in an email thread that getting an async rendering feature
> from Adobe is the ultimate solution but could take a while? 

Hopefully this summer if all goes according to plan.

> 
> Jim: do you think we could create an image and cache it every second or so
> from the plugin?

See above, I can set a timer and sample more frequently. It risks dragging page performance down though, I'm not sure that's worth it. 

> Also, to the question of offscreen plugins not being able to be sampled, I
> think for a user this is not terrible, the scenario is instead that the user
> has a video playing but they can't see it - if they scroll it into view they
> will initially see say a black, blank rectangle but then when they stop
> scrolling the video will appear - correct? In my mind this is, at least for
> the time being, acceptable.

Initially they should see a gray background (or whatever color we want to use). Once they stop scrolling the window becomes visible and the video displays, and we try to grab a capture right then for the next scroll.
Flags: needinfo?(jmathies)
(In reply to Jim Mathies [:jimm] from comment #14)
> > * for ads, as a user I don't care really if the ad animation seems jumpy /
> > janked as I scroll. having *something* that is visually similar to what the
> > plugin was showing as I scroll is better than a black square.
> 
> FYI, ads and video are the same to us, we don't differentiate. 
...

Understood, and I don't think we need to. I think the biggest differences for the user is that typically ads play automatically, don't produce noise and are rarely in the middle of the page. Videos typically require user interaction to get going and do make noise.

> > * for video, it's intuitively more jarring for users to have the image
> > frozen as they scroll. I think the jarring-ness depends heavily on how
> > recent the image is that we captured, and this problem seems rife with
> > complication:
> > 
> >   - few videos will actually look the same when the user scrolls than they
> > did when the plugin loaded
> 
> Yes this is complete random. You never know what you'll get in the capture.

Given that what I care about most is a good experience watching a video, this is not great. It's not clear wich is worse: flashing to black which seems to be the current behaviour or flashing to some video frame that is randomly out of sync.

> >   - you can easily imagine perf problems arising if we sample the output of
> > the plugin every frame or even every second
> 
> Currently I do not try to do this specifically for this reason. Measuring
> what the impact would be might be tough but we could try to put something
> together.

I think I agree with your original decision - the effort spent measuring it isn't worth it.

...
> > Also, to the question of offscreen plugins not being able to be sampled, I
> > think for a user this is not terrible, the scenario is instead that the user
> > has a video playing but they can't see it - if they scroll it into view they
> > will initially see say a black, blank rectangle but then when they stop
> > scrolling the video will appear - correct? In my mind this is, at least for
> > the time being, acceptable.
> 
> Initially they should see a gray background (or whatever color we want to
> use). Once they stop scrolling the window becomes visible and the video
> displays, and we try to grab a capture right then for the next scroll.

I think if we're going to pick a colour I'd stick with gray as it's the least distance jump visually from any other colour.
> Given that what I care about most is a good experience watching a video,
> this is not great. It's not clear which is worse: flashing to black which
> seems to be the current behaviour or flashing to some video frame that is
> randomly out of sync.

Hmm, it shouldn't be black. blassey mentioned he was seeing the same on dailymotion in Windows 10. I'll take a look at that.
Attached patch wip (obsolete) — Splinter Review
Attachment #8718948 - Attachment is obsolete: true
Attached patch wipSplinter Review
try builds
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbfc578879e8

These builds should have better results on sites like dailymotion. I switched to using the PinrtWindow api in the hope that it would do a better job of capturing off screen windows, but in the end it didn't help. The api seems to have some reliability issues in actually capturing bits so I've switched back to using BitBlt, which has less overhead and should be more reliable.

I also tweaked some of the timing to try and improve capture success rates.
Attachment #8720076 - Attachment is obsolete: true
new builds:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d79bc93f467

This build fixes a problem on windows 10. It doesn't fix the sandbox problem though, I need to investigate that tomorrow. To turn off the sandbox set the 'security.sandbox.content.level' pref to -1 and restart.
(In reply to Jim Mathies [:jimm] from comment #20)
> new builds:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d79bc93f467
> 
> This build fixes a problem on windows 10. It doesn't fix the sandbox problem
> though, I need to investigate that tomorrow. To turn off the sandbox set the
> 'security.sandbox.content.level' pref to -1 and restart.

Jim and I chatted on IRC - given some light testing we're happy with the current state of this. I think this should be flagged with qa-wanted after landing.
Flags: needinfo?(jgriffiths)
This adds support on Windows, I'm working on a follow up patch to add gtk support.
It seems to me that the Windows implementation here will avoid capturing any plugin that's partially or wholly out of view, right? Isn't that a pretty severe limitation? It means plugins scrolling into view will never benefit from this.
Flags: needinfo?(jmathies)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> It seems to me that the Windows implementation here will avoid capturing any
> plugin that's partially or wholly out of view, right? Isn't that a pretty
> severe limitation? It means plugins scrolling into view will never benefit
> from this.

Yes that's the case. I've experimented with a few other methods to try and work around that but nothing produced full capture for windows that were clipped or hidden. Some of the other methods are in this wip patch [1].  Jeff Griffiths tested a test build and felt this was worth landing for the improvement it provides.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8720076&action=diff (PluginInstanceChild UpdateScrollCapture code)
Flags: needinfo?(jmathies)
Comment on attachment 8723531 [details]
MozReview Request: Bug 1232181 - Plugin module plumbing for retrieving scroll captures and updating plugin instance content scroll state. r=roc

https://reviewboard.mozilla.org/r/36595/#review33337
Attachment #8723531 - Flags: review?(roc) → review+
Comment on attachment 8723532 [details]
MozReview Request: Bug 1232181 - Notify plugins about scroll state. r=roc

https://reviewboard.mozilla.org/r/36597/#review33339
Attachment #8723532 - Flags: review?(roc) → review+
Comment on attachment 8723533 [details]
MozReview Request: Bug 1232181 - Add an image layer for plugin frames that represent windowed plugins on platforms that support scroll capture. r=roc

https://reviewboard.mozilla.org/r/36599/#review33341

::: layout/generic/nsPluginFrame.cpp:420
(Diff revision 1)
> -    configuration->mVisible = mWidget->IsVisible();
> +    configuration->mVisible = mIsHiddenDueToScroll ? false : mWidget->IsVisible();

!mIsHiddenDueToScroll && mWidget->IsVisible()
Comment on attachment 8723531 [details]
MozReview Request: Bug 1232181 - Plugin module plumbing for retrieving scroll captures and updating plugin instance content scroll state. r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36595/diff/1-2/
Attachment #8723531 - Attachment description: MozReview Request: Bug 1232181 - Plugin module plumbing for retrieving scroll captures and updating plugin instance content scroll state. r?roc → MozReview Request: Bug 1232181 - Plugin module plumbing for retrieving scroll captures and updating plugin instance content scroll state. r=roc
Comment on attachment 8723532 [details]
MozReview Request: Bug 1232181 - Notify plugins about scroll state. r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36597/diff/1-2/
Attachment #8723532 - Attachment description: MozReview Request: Bug 1232181 - Notify plugins about scroll state. r?roc → MozReview Request: Bug 1232181 - Notify plugins about scroll state. r=roc
Comment on attachment 8723533 [details]
MozReview Request: Bug 1232181 - Add an image layer for plugin frames that represent windowed plugins on platforms that support scroll capture. r=roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36599/diff/1-2/
Attachment #8723533 - Attachment description: MozReview Request: Bug 1232181 - Add an image layer for plugin frames that represent windowed plugins on platforms that support scroll capture. r?roc → MozReview Request: Bug 1232181 - Add an image layer for plugin frames that represent windowed plugins on platforms that support scroll capture. r=roc
Attachment #8723534 - Attachment description: MozReview Request: Bug 1232181 - Add a few win resource helpers. → MozReview Request: Bug 1232181 - Add a few win resource helpers. r?aklotz
Attachment #8723534 - Flags: review?(aklotz)
Comment on attachment 8723534 [details]
MozReview Request: Bug 1232181 - Add a few win resource helpers. r?aklotz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36601/diff/1-2/
Comment on attachment 8723535 [details]
MozReview Request: Bug 1232181 - Add support for capturing plugin windows on Windows. r?aklotz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36603/diff/1-2/
Attachment #8723535 - Attachment description: MozReview Request: Bug 1232181 - Add support for capturing plugin windows on Windows. → MozReview Request: Bug 1232181 - Add support for capturing plugin windows on Windows. r?aklotz
Attachment #8723535 - Flags: review?(aklotz)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> Comment on attachment 8723533 [details]
> MozReview Request: Bug 1232181 - Add an image layer for plugin frames that
> represent windowed plugins on platforms that support scroll capture. r=roc
> 
> https://reviewboard.mozilla.org/r/36599/#review33341
> 
> ::: layout/generic/nsPluginFrame.cpp:420
> (Diff revision 1)
> > -    configuration->mVisible = mWidget->IsVisible();
> > +    configuration->mVisible = mIsHiddenDueToScroll ? false : mWidget->IsVisible();
> 
> !mIsHiddenDueToScroll && mWidget->IsVisible()

Updated, thanks.
Hey Karl, I'm trying to implement the window capture portion of this work for GTK. Currently experimenting with the XGetImage api [1]. I've run into a problem figuring out how to get the resulting XImage data of this call into a native gfxXlibSurface [2] surface. Can you comment on the general direction I'm attempting here (is XGetImage the right api to use) and I'm wondering if you have any ideas on the best way to get the resulting image into one of our internal gfx objects. I need to copy the result over into a sharable image surface compatible with IPC.

[1] https://tronche.com/gui/x/xlib/graphics/XGetImage.html
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxXlibSurface.h
Flags: needinfo?(karlt)
(In reply to Jim Mathies [:jimm] from comment #39)
> Hey Karl, I'm trying to implement the window capture portion of this work
> for GTK. Currently experimenting with the XGetImage api [1]. I've run into a
> problem figuring out how to get the resulting XImage data of this call into
> a native gfxXlibSurface [2] surface. Can you comment on the general
> direction I'm attempting here (is XGetImage the right api to use) and I'm
> wondering if you have any ideas on the best way to get the resulting image
> into one of our internal gfx objects. I need to copy the result over into a
> sharable image surface compatible with IPC.

This is going to be awkward whatever the approach, and since the landing for
bug 1241832 anything is going to be slow due to the need to read back from the
window.

This caveat makes use of XGetImage with windows difficult:

  "If the drawable is a window, the window must be viewable, and it must be the
  case that if there were no inferiors or overlapping windows, the specified
  rectangle of the window would be fully visible on the screen and wholly
  contained within the outside edges of the window, or a BadMatch error
  results."

Cairo has code to work around much of this issue, though you still won't get
anything useful for plugins out of view.

Creating a cairo xlib surface for the window and drawing that into an image
surface will let cairo do this work for you, but its code is only suitable for
use on the main thread.  I'm not familiar with how this translates to Moz2D
APIs.

Still, I wonder whether it is better to just accept some glitches while
scrolling than to slow down the scrolling with XGetImage.  Perhaps the capture can be done in the plugin process, but you'd need something to draw while waiting for the response.

Another approach that has been used in the past is to render the plugin
offscreen [3].  It would still be slow since the change for bug 1241832, but
you'd have something sane to draw even when the plugin is not visible, and you could always get that in the plugin process whenever the plugin draws, so that you would not need to wait for the plugin when scrolling.
However, event handling then gets complex, and I don't know how well the old
code worked.  I'm doubting this is worth the effort.

[3] MOZ_COMPOSITED_PLUGINS at
    http://hg.mozilla.org/releases/mozilla-beta/rev/48236642cb12
Flags: needinfo?(karlt)
(In reply to Jim Mathies [:jimm] from comment #39)
> Hey Karl, I'm trying to implement the window capture portion of this work
> for GTK. Currently experimenting with the XGetImage api [1]. I've run into a
> problem figuring out how to get the resulting XImage data of this call into
> a native gfxXlibSurface [2] surface. Can you comment on the general
> direction I'm attempting here (is XGetImage the right api to use) and I'm
> wondering if you have any ideas on the best way to get the resulting image
> into one of our internal gfx objects. I need to copy the result over into a
> sharable image surface compatible with IPC.
> 
> [1] https://tronche.com/gui/x/xlib/graphics/XGetImage.html
> [2] http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxXlibSurface.h

I agree with Karl in that it sounds like the easy way out is wrapping the X window in a cairo xlib surface and then drawing to a cairo image surface with it. This is going to be far easier than messing with raw Xlib.

On the other hand the Cairo Xlib code to do it is not terribly bad and you could roughly copy the idea, if you must: https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-xlib-surface.c#875
All it does is create a pixmap, copy to the pixmap, getimage from that, then free the pixmap. It's not that much more involved.

The XImage itself is just a wrapper around raw data in memory. The data pointer pointers to the bytes, bytes_per_line is the stride, width/height are the size, and the SurfaceFormat you should ensure you already have around somewhere, as it is extremely hard to produce otherwise.

But I also agree with Karl that this is going to be slow and janky because we really have no fast way of transferring between the X server and an image surface in the code as it stands. You can do it in principle, but it is just going to be slow, no easy way around it.

There are ways to do it quickly with XShm and XShmGetImage like how nsShmImage in the widget code works, but this is going to be overkill and error prone to code. If XGetImage is not really fast enough to work without massive jank, then it'd be best to just leave the flicker in its stead...
(In reply to Karl Tomlinson (ni?:karlt) from comment #40)
> This is going to be awkward whatever the approach, and since the landing for
> bug 1241832 anything is going to be slow due to the need to read back from
> the window.
> 
> This caveat makes use of XGetImage with windows difficult:
> 
>   "If the drawable is a window, the window must be viewable, and it must be
> the
>   case that if there were no inferiors or overlapping windows, the specified
>   rectangle of the window would be fully visible on the screen and wholly
>   contained within the outside edges of the window, or a BadMatch error
>   results."

This is ok, the same limits apply on the windows side, which we're going to accept. If the capture fails for some reason the underlying layer is painted gray as a work around.

> Cairo has code to work around much of this issue, though you still won't get
> anything useful for plugins out of view.
> 
> Creating a cairo xlib surface for the window and drawing that into an image
> surface will let cairo do this work for you, but its code is only suitable
> for
> use on the main thread.  I'm not familiar with how this translates to Moz2D
> APIs.
> 
> Still, I wonder whether it is better to just accept some glitches while
> scrolling than to slow down the scrolling with XGetImage.  Perhaps the
> capture can be done in the plugin process, but you'd need something to draw
> while waiting for the response.

I don't think the performance here is that important. We're not interested in doing lots of capturing, just a single capture near plugin startup and another at the end of each page scroll. There are plans to try and do capture at the beginning of the scroll but that work will be in a follow up.

> Another approach that has been used in the past is to render the plugin
> offscreen [3].  It would still be slow since the change for bug 1241832, but
> you'd have something sane to draw even when the plugin is not visible, and
> you could always get that in the plugin process whenever the plugin draws,
> so that you would not need to wait for the plugin when scrolling.
> However, event handling then gets complex, and I don't know how well the old
> code worked.  I'm doubting this is worth the effort.

This sounds scary, and I want to avoid invasive changes here. If there's no simply way to grab an image of the window occasionally when the window is in view, then we'll just live with the flicker until npapi goes away.

(In reply to Lee Salzman [:lsalzman] from comment #41)
> On the other hand the Cairo Xlib code to do it is not terribly bad and you
> could roughly copy the idea, if you must:
> https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-
> xlib-surface.c#875
> All it does is create a pixmap, copy to the pixmap, getimage from that, then
> free the pixmap. It's not that much more involved.
> 
> The XImage itself is just a wrapper around raw data in memory. The data
> pointer pointers to the bytes, bytes_per_line is the stride, width/height
> are the size, and the SurfaceFormat you should ensure you already have
> around somewhere, as it is extremely hard to produce otherwise.

I was looking at this code for clues as to how to get the results of an XGetImage call into an gfxXlibSurface. I wanted to avoid manual copying of bits, but maybe that's the only way. Maybe I could expose the Drawable of gfxXlibSurface and pass that to XGetImage here?

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxXlibSurface.h
 
> But I also agree with Karl that this is going to be slow and janky because
> we really have no fast way of transferring between the X server and an image
> surface in the code as it stands. You can do it in principle, but it is just
> going to be slow, no easy way around it.

The plan is to do capture occasionally, with the expectation that the window image will usually be stale.

If you guys have any other thoughts based on this I'd love to hear them. I'd like to work up a test build using XGetImage (or similar api) to see how well it works.
(In reply to Jim Mathies [:jimm] from comment #42)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #40)
> > This is going to be awkward whatever the approach, and since the landing for
> > bug 1241832 anything is going to be slow due to the need to read back from
> > the window.
> > 
> > This caveat makes use of XGetImage with windows difficult:
> > 
> >   "If the drawable is a window, the window must be viewable, and it must be
> > the
> >   case that if there were no inferiors or overlapping windows, the specified
> >   rectangle of the window would be fully visible on the screen and wholly
> >   contained within the outside edges of the window, or a BadMatch error
> >   results."
> 
> This is ok, the same limits apply on the windows side, which we're going to
> accept. If the capture fails for some reason the underlying layer is painted
> gray as a work around.
> 
> > Cairo has code to work around much of this issue, though you still won't get
> > anything useful for plugins out of view.
> > 
> > Creating a cairo xlib surface for the window and drawing that into an image
> > surface will let cairo do this work for you, but its code is only suitable
> > for
> > use on the main thread.  I'm not familiar with how this translates to Moz2D
> > APIs.
> > 
> > Still, I wonder whether it is better to just accept some glitches while
> > scrolling than to slow down the scrolling with XGetImage.  Perhaps the
> > capture can be done in the plugin process, but you'd need something to draw
> > while waiting for the response.
> 
> I don't think the performance here is that important. We're not interested
> in doing lots of capturing, just a single capture near plugin startup and
> another at the end of each page scroll. There are plans to try and do
> capture at the beginning of the scroll but that work will be in a follow up.
> 
> > Another approach that has been used in the past is to render the plugin
> > offscreen [3].  It would still be slow since the change for bug 1241832, but
> > you'd have something sane to draw even when the plugin is not visible, and
> > you could always get that in the plugin process whenever the plugin draws,
> > so that you would not need to wait for the plugin when scrolling.
> > However, event handling then gets complex, and I don't know how well the old
> > code worked.  I'm doubting this is worth the effort.
> 
> This sounds scary, and I want to avoid invasive changes here. If there's no
> simply way to grab an image of the window occasionally when the window is in
> view, then we'll just live with the flicker until npapi goes away.
> 
> (In reply to Lee Salzman [:lsalzman] from comment #41)
> > On the other hand the Cairo Xlib code to do it is not terribly bad and you
> > could roughly copy the idea, if you must:
> > https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-
> > xlib-surface.c#875
> > All it does is create a pixmap, copy to the pixmap, getimage from that, then
> > free the pixmap. It's not that much more involved.
> > 
> > The XImage itself is just a wrapper around raw data in memory. The data
> > pointer pointers to the bytes, bytes_per_line is the stride, width/height
> > are the size, and the SurfaceFormat you should ensure you already have
> > around somewhere, as it is extremely hard to produce otherwise.
> 
> I was looking at this code for clues as to how to get the results of an
> XGetImage call into an gfxXlibSurface. I wanted to avoid manual copying of
> bits, but maybe that's the only way. Maybe I could expose the Drawable of
> gfxXlibSurface and pass that to XGetImage here?
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxXlibSurface.h
>  
> > But I also agree with Karl that this is going to be slow and janky because
> > we really have no fast way of transferring between the X server and an image
> > surface in the code as it stands. You can do it in principle, but it is just
> > going to be slow, no easy way around it.
> 
> The plan is to do capture occasionally, with the expectation that the window
> image will usually be stale.
> 
> If you guys have any other thoughts based on this I'd love to hear them. I'd
> like to work up a test build using XGetImage (or similar api) to see how
> well it works.

XGetImage *is* a server->client copy into raw data in the client process. By the time you did XGetImage, you actually did a readback from the X server into our client process, that has not only copied it, but sent the data over a socket, a very slow socket. If your goal was to avoid that, then the fact that you are staring at an XGetImage means you've taken the wrong fork in the road. :)

It sounds like you might be a bit lost in the maze that is Xlib, and that what you're wanting to do is create a server-side Pixmap into which the data is backed up so that it can be blit out... if your goal is to blit it back to a window. If you actually need to trade it with the compositor or something similar, that is messier and depends on which compositor is used. You'd need to describe in more detail about where this data needs to go and how.

But backing it up into a Pixmap, if that is what you truly need to do, is easy enough... create a new Cairo xlib surface somehow (either via gfxXlibSurface or cairo_xlib_surface_create_*), then wrap your source in a gfxXlibSurface or cairo_xlib_surface similarly, copy one to the other. Now you're done, and have a shiny new Pixmap that is wrapped up for you.

At some point I might be better off just implementing the details for you since this might end up trickier than it seems.
Comment on attachment 8723535 [details]
MozReview Request: Bug 1232181 - Add support for capturing plugin windows on Windows. r?aklotz

https://reviewboard.mozilla.org/r/36603/#review33729

::: dom/plugins/ipc/PluginInstanceParent.cpp:468
(Diff revision 2)
> +        if (windowed) {

Nit: indentation

::: dom/plugins/ipc/PluginInstanceParent.cpp:1380
(Diff revision 2)
> +PluginInstanceParent::GetScrollCaptureContainer(ImageContainer** aContainer)

Nit: Validate aContainer != nullptr
Attachment #8723535 - Flags: review?(aklotz) → review+
Attachment #8723534 - Flags: review?(aklotz) → review+
(In reply to Lee Salzman [:lsalzman] from comment #43)
> XGetImage *is* a server->client copy into raw data in the client process. By
> the time you did XGetImage, you actually did a readback from the X server
> into our client process, that has not only copied it, but sent the data over
> a socket, a very slow socket. If your goal was to avoid that, then the fact
> that you are staring at an XGetImage means you've taken the wrong fork in
> the road. :)
> 
> It sounds like you might be a bit lost in the maze that is Xlib, and that
> what you're wanting to do is create a server-side Pixmap into which the data
> is backed up so that it can be blit out... if your goal is to blit it back
> to a window.
>
> If you actually need to trade it with the compositor or
> something similar, that is messier and depends on which compositor is used.
> You'd need to describe in more detail about where this data needs to go and
> how.

I need to blit a copy of the window to a mozilla layer in our layer system, which is managed by the content process. When the user scrolls scroll the window gets hidden. The layer underneath that window would hold a recent capture of the window. The idea is to minimize the effect of hiding the window. 


> But backing it up into a Pixmap, if that is what you truly need to do, is
> easy enough... create a new Cairo xlib surface somehow (either via
> gfxXlibSurface or cairo_xlib_surface_create_*), then wrap your source in a
> gfxXlibSurface or cairo_xlib_surface similarly, copy one to the other. Now
> you're done, and have a shiny new Pixmap that is wrapped up for you.
> 
> At some point I might be better off just implementing the details for you
> since this might end up trickier than it seems.

Yeah. I think I'll split the GTX/X11 work out and land the Windows bits.
Blocks: 1252874
Blocks: 1252877
Depends on: 1253434
Is this still an m8 blocker? It hasn't landed on 46 aurora.
Flags: needinfo?(jmathies)
I'd like this work to bake a bit. I'm not sure if we'll want to take it on beta.
Flags: needinfo?(jmathies)
Jim, should we uplift this plugin scrolling fix to Beta 46 for our e10s experiment?
Flags: needinfo?(jmathies)
(In reply to Chris Peterson [:cpeterson] from comment #51)
> Jim, should we uplift this plugin scrolling fix to Beta 46 for our e10s
> experiment?

No I don't think so, it's in 47 which is the next potential beta ga. I don't want to push this code out any harder.
Flags: needinfo?(jmathies)
That makes sense. Marking 46 as wontfix.
Blocks: 1257911
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: