Closed
Bug 1232181
Opened 8 years ago
Closed 8 years ago
Replace plugin windows with static window captures when scrolling
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla47
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
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jmathies
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Comment 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
Latest wip. I'm still not getting good screen captures from flash for instances that are clipped.
Attachment #8713382 -
Attachment is obsolete: true
Assignee | ||
Comment 6•8 years ago
|
||
removed some debug code
Attachment #8717912 -
Attachment is obsolete: true
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8717915 -
Attachment is obsolete: true
Assignee | ||
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
windows builds for testing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7374a2294522
Assignee | ||
Comment 10•8 years ago
|
||
windows builds: http://archive.mozilla.org/pub/firefox/try-builds/jmathies@mozilla.com-7374a2294522745feb44d15a676313739db94ee8/try-win32/
Flags: needinfo?(jgriffiths)
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Comment 12•8 years ago
|
||
Comment 13•8 years ago
|
||
(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)
Assignee | ||
Comment 14•8 years ago
|
||
> * 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)
Comment 15•8 years ago
|
||
(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.
Assignee | ||
Comment 16•8 years ago
|
||
> 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.
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8718948 -
Attachment is obsolete: true
Assignee | ||
Comment 18•8 years ago
|
||
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
Assignee | ||
Comment 19•8 years ago
|
||
refreshed builds: http://archive.mozilla.org/pub/firefox/try-builds/jmathies@mozilla.com-dbfc578879e825ada8a75c863fa2db99559cf177/try-win32/
Flags: needinfo?(jgriffiths)
Assignee | ||
Comment 20•8 years ago
|
||
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.
Comment 21•8 years ago
|
||
(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)
Assignee | ||
Comment 22•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36595/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36595/
Attachment #8723531 -
Flags: review?(roc)
Assignee | ||
Comment 23•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36597/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36597/
Attachment #8723532 -
Flags: review?(roc)
Assignee | ||
Comment 24•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36599/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36599/
Attachment #8723533 -
Flags: review?(roc)
Assignee | ||
Comment 25•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36601/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36601/
Assignee | ||
Comment 26•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36603/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36603/
Assignee | ||
Comment 27•8 years ago
|
||
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)
Assignee | ||
Comment 29•8 years ago
|
||
(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+
Attachment #8723533 -
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()
Assignee | ||
Comment 33•8 years ago
|
||
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
Assignee | ||
Comment 34•8 years ago
|
||
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
Assignee | ||
Comment 35•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
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)
Assignee | ||
Comment 36•8 years ago
|
||
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/
Assignee | ||
Comment 37•8 years ago
|
||
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)
Assignee | ||
Comment 38•8 years ago
|
||
(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.
Assignee | ||
Comment 39•8 years ago
|
||
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)
Comment 40•8 years ago
|
||
(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)
Comment 41•8 years ago
|
||
(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...
Assignee | ||
Comment 42•8 years ago
|
||
(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.
Comment 43•8 years ago
|
||
(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 44•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8723534 -
Flags: review?(aklotz) → review+
Comment 45•8 years ago
|
||
Comment on attachment 8723534 [details] MozReview Request: Bug 1232181 - Add a few win resource helpers. r?aklotz https://reviewboard.mozilla.org/r/36601/#review33733
Assignee | ||
Comment 46•8 years ago
|
||
(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.
Comment 47•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/667a8c2d17d2 https://hg.mozilla.org/integration/mozilla-inbound/rev/ccf462d9fd85 https://hg.mozilla.org/integration/mozilla-inbound/rev/c03393ef4f1a https://hg.mozilla.org/integration/mozilla-inbound/rev/0f396f571f3a https://hg.mozilla.org/integration/mozilla-inbound/rev/8419b0626eb4
Comment 48•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/667a8c2d17d2 https://hg.mozilla.org/mozilla-central/rev/ccf462d9fd85 https://hg.mozilla.org/mozilla-central/rev/c03393ef4f1a https://hg.mozilla.org/mozilla-central/rev/0f396f571f3a https://hg.mozilla.org/mozilla-central/rev/8419b0626eb4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 49•8 years ago
|
||
Is this still an m8 blocker? It hasn't landed on 46 aurora.
Flags: needinfo?(jmathies)
Updated•8 years ago
|
status-firefox46:
--- → affected
Assignee | ||
Comment 50•8 years ago
|
||
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)
Comment 51•8 years ago
|
||
Jim, should we uplift this plugin scrolling fix to Beta 46 for our e10s experiment?
Flags: needinfo?(jmathies)
Assignee | ||
Comment 52•8 years ago
|
||
(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)
Comment 53•8 years ago
|
||
That makes sense. Marking 46 as wontfix.
You need to log in
before you can comment on or make changes to this bug.
Description
•