742832 - Add support for tab sharing for getUserMedia

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

This will enable (with user agreement!) an application like Google Hangouts to share a screen or window as "video".  In the future such streams might also get specialized codecs for better compression/quality/etc, and may well get special codec settings (like not sending no-change packets often, changing encoding parameters, changing iframe generation timing/triggers, etc)

Comment 1

[(not currently active) Ted Mielczarek]

Might be worth revisiting some of the reasons we decided not to expose the canvas drawWindow method to content...

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Now that we've redesigned <input type="file"> to not draw the full path on the screen, things are better. We still have problems with things like drawing cross-origin images and <iframe>s.

Even with user opt-in, I'd worry about situations like "user loads app page A in one tab and app page B in another tab, page B asks for permission to screen-share page A which looks fine, user accepts, then app swaps an <iframe> of FB or gmail or whatever into page A and grabs the contents.

Comment 3

[(not currently active) Ted Mielczarek]

Google Hangouts allows sharing your entire desktop, so it's actually even worse than that, but yeah, granting permission to capture video to a webapp that can influence what shows up on the screen is problematic.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

It seems like this might be the sort of thing could be most easily achieved step-wise.  In particular, I could imagine making the problem(s) more tractable by ordering them thusly:

* sharing a canvas element
* sharing a tab
* sharing a window
* sharing a desktop

Thoughts?

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

I chose these particular steps because I can think of meaningful use cases for each one.  That said, listing use cases and prioiritizing those might suggest a different ordering, while still make allowing us to pick low-hanging fruit quickly.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

It looks like this is how Chrome for Desktop supports mirroring tabs to the new Chromecast devices. Well, not exactly, but Chrome supports a "tabCapture" API that creates a MediaStream of the active tab. That is then sent to the Chromecast device which displays the "video" on your TV.

http://www.androidauthority.com/html5-and-webrtc-the-technology-behind-chromecast-248968/

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP patch

this is a very WIP patch, looking for feedback on the approach.

a couple notes:
* getting the nsIDOMWindow of a tab to share is probably something best left to the browser chrome
 - this currently just grabs the first tab for Fennec
 - it enumerates all global windows on non-android, which doesn't really work
* I'm not getting an moz-after-paint events, despite testing on a page with an animated gif
 - to make sure everything else was working there's also a timer to repaint ever 40ms
* painting to the surface returned by CairoImage.GetAsSurface() doesn't work.
 - this creates a new surface and new CairoImage every time to work around
 - probably want something less thrashy
* I didn't bother with any locking, despite desperately needing to lock the image/surface

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Nice hack. It's not a terrible approach. Try adding your MozAfterPaint listener to mVideoSource->mWindow->GetChromeEventHandler() (we disabled MozAfterPaint on the content window itself).

A slightly better approach would probably be to hook into the OMTC compositor so after it's composited a frame, we read that frame back (asynchronously if possible) and append it to the SourceMediaStream. That would all happen off the main thread and would not require any event listening (and would work with OMT animations/video).

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 792947 [details] [diff] [review]
WIP patch

I tend to agree with roc that a better long-term plan is to hook the compositor, as that's where (my understanding is) the video element and other things all come together in a coherent view of the tab.  My one concern about hooking it there is that non-visible tabs may not get handled by the compositor, so a "shared" tab may need to be special-cased (though I think a mechanism for this already exists per chat with brad).

I'm sure that sharing tabs with video/audio will be requested, especially as things like Chromecast are actively blocking showing arbitrary HTML5 videos using the DIAL/etc API.  (Yes, the quality will be lower and there will be other issues.  But it can work.)

Comment 10

[jonatan.littke]

Thank you so much blassey for beginning work on this bug. It's very much appreciated and we're looking forward to seeing it implemented.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: tab_streaming.patch

This looks for a nsITabSource service, and if it exists provides that video source as an option.

Note, as for the more elegant solution of taking directly from the compositor, I think that would be a great fast path, but as far as I can tell it won't work for background tabs, so we'll need this slower path anyway.

Also note that we could probably save work by only redrawing the dirty region reported by the MozAfterPaint event. I'd like to do that as a follow up though.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: example implementation of of nsITabSource for fennec

presumably we'll want some UI that says "which tab would you like to share" and appropriate privacy warnings. Right now this is just intended for testing and returns the first tab in Fennec.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: tab_streaming.patch

noticed that the code to add the tab video source was inside a #ifdef ANDROID block

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 813164 [details] [diff] [review]
tab_streaming.patch

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

I'll note there's a BUNCH of support for chosen codec settings/etc for screen sharing in the webrtc.org codebase.  We'll want to use those going forward to set the stream up in VideoConduit.cpp

r+, but file followup(s) to parameterize/pref various settings

::: content/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +18,5 @@
> +namespace mozilla {
> +
> +NS_IMPL_ISUPPORTS1(MediaEngineTabVideoSource, MediaEngineVideoSource)
> +
> +MediaEngineTabVideoSource::MediaEngineTabVideoSource() : mBufW(240), mBufH(240), mData((unsigned char*)malloc(mBufW * mBufH * 4)), mName(NS_LITERAL_STRING("share tab")), mUuid(NS_LITERAL_STRING("uuid")) {

line length
Why 240x240??  Should this be a pref at least, or better settable when the share is created?

@@ +30,5 @@
> +  if (privateDOMWindow) {
> +    privateDOMWindow->GetChromeEventHandler()->AddEventListener(NS_LITERAL_STRING("MozAfterPaint"), mVideoSource, false);
> +  } else {
> +    mVideoSource->mTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +    mVideoSource->mTimer->InitWithCallback(mVideoSource, 40, nsITimer:: TYPE_REPEATING_SLACK);

why 40ms?  25fps.... the video codecs for webrtc typically target 30fps.  You can run slower of course.
I'd suggest making this a pref at least; better would be to have it be adjustable

@@ +133,5 @@
> +  TrackTicks delta = target - aLastEndTime;
> +  if (delta > 0) {
> +    // NULL images are allowed
> +    if (image) {
> +      segment.AppendFrame(image.forget(), delta, gfxIntSize(mBufW, mBufH));

Does the image encode within itself the size in a way that can be queried?  If so, better than using the pre-configured target values

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +382,2 @@
>  
> +  

remove this added blank line

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: tab_streaming.patch

I think it's worth getting those prefs into the initial patch, so here it is for re-review

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 818094 [details] [diff] [review]
tab_streaming.patch

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

Sorry I missed the missing lock before....
Add a Mutex lock around mImage and r+ (with nit fixes)

::: content/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +74,5 @@
> +  if (!branch)
> +    return NS_OK;
> +  branch->GetIntPref("media.tabstreaming.width", &mVideoSource->mBufW);
> +  branch->GetIntPref("media.tabstreaming.height", &mVideoSource->mBufH);
> +  branch->GetIntPref("media.tabstreaming.time_per_frame", &mVideoSource->mTimePerFrame);

You really want to observe these for changes, but likely you don't care yet and the prefs will be replaced later anyways

@@ +86,5 @@
> +  if (!win)
> +    return NS_OK;
> +
> +  mVideoSource->mWindow = win;
> +  NS_DispatchToMainThread(new StartRunnable(mVideoSource));

So, we MUST be running on MainThread here (prefs access aren't threadsafe).  So the dispatchtomainthread loops through the event queue, which is ok, but likely not needed (added complexity for no gain).

If there's actually a reason for it, please note it in a comment since it otherwise seems odd/wrong.

@@ +141,5 @@
> +{
> +  VideoSegment segment;
> +
> +  // Note: we're not giving up mImage here
> +  nsRefPtr<layers::CairoImage> image = mImage;

Need to lock around use of mImage (this is on the MSG thread)

@@ +232,5 @@
> +  uint32_t stride = size.width * 4;
> +
> +  nsRefPtr<layers::ImageContainer> container = layers::LayerManager::CreateImageContainer();
> +  nsRefPtr<gfxASurface> surf;
> +  if (false) {

Really?  At least comment what will go there or link to a bug that blocks it

@@ +235,5 @@
> +  nsRefPtr<gfxASurface> surf;
> +  if (false) {
> +    surf = mImage->GetAsSurface();
> +  } else {
> +    surf = new gfxImageSurface(static_cast<unsigned char*>(mData), size, 

trailing space

@@ +247,5 @@
> +  context->Translate(pt);
> +  context->Scale(scale * size.width / srcW, scale * size.height / srcH);
> +  rv = presShell->RenderDocument(r, renderDocFlags, bgColor, context);
> +
> +  NS_ENSURE_SUCCESS(rv, );

NS_ENSURE_SUCCESS_VOID(rv)

@@ +258,5 @@
> +  nsRefPtr<layers::CairoImage> image = new layers::CairoImage();
> +
> +  image->SetData(cairoData);
> +
> +  mImage = image;

Need to lock around replacement of mImage

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: tab_streaming.patch

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Changing description to reflect what's being done here.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 818661 [details] [diff] [review]
tab_streaming.patch

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

::: content/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +147,5 @@
> +  VideoSegment segment;
> +  MonitorAutoLock mon(mMonitor);
> +
> +  // Note: we're not giving up mImage here
> +  nsRefPtr<layers::CairoImage> image = mImage;

I believe the monitor is only needed around the image = mImage, but it's ok as-is and matches getUserMedia()

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c5391fab9cc1

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Brad, new files here didn't include a license header. Can you fix that ASAP? Thanks!

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: license_header_tab_source.patch

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #18)
> Changing description to reflect what's being done here.

just wanted to note that window, app and screen capture is being tracked in bug 923225

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 828660 [details] [diff] [review]
license_header_tab_source.patch

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

Thanks.

Comment 25

[Shun Zhang]

Hello all,

So I have read through this thread. I am still wondering whether there is an API available so that we can record/share a tab. This is a desirable feature for my work - it would be great if I can use such API instead of implementing my own.

I appreciate your advice.

Thanks.

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

There isn't a good API for that yet

Comment 27

[Shun Zhang]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #26)
> There isn't a good API for that yet

Hello Brad,

Thanks! So is Mozilla working on this API? Is there a codebase we can help with?

Best.

Comment 28

[Chiranjit Dey]

Thanks! So is Mozilla working on this API? Is there a codebase we can help with? https://www.tricksbysmt.com

Comment 29

[Maire Reavy [:mreavy]]

The underlying code for this exists and was used in Hello, but a fair bit of additional work must be done in the spec and on the security UX/UI design in order to expose this to the web (non-privileged). And so until those are resolved, there is no implementation work to do. What's more, it is likely extremely difficult to explain to the user the additional risks of tab sharing.  It was quite challenging to explain the risk of simple screensharing to users.

Comment 30

[hjtrick]

http://hjtrick.com/high-pr-directory-submission-site-list-2017/

Comment 31

[hjtrick]

http://hjtrick.com/5-best-photo-editing-apps-for-android/

Comment 32

[hjtrick]

http://hjtrick.com/affiliate-marketing-tips-for-beginners/

Comment 33

[david bell]

Thanks for posting this. I appreciate it.
<a href='http://www.paidapps4free.com/'>My Boy Free </a>

Comment 34

[yurajjha86]

It looks like this is how Chrome for Desktop supports mirroring tabs to the new Chromecast devices. Well, not exactly, but Chrome supports a "tabCapture" API that creates a MediaStream of the active tab. That is then sent to the Chromecast device which displays the "video" on your TV.
https://bhaikiearningtricks.blogspot.com/

Comment 35

[Mark Banner (:standard8)]

Restricting comments as this is becoming a spam bug.