Closed Bug 742832 Opened 8 years ago Closed 6 years ago

Add support for tab sharing for getUserMedia

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jesup, Assigned: blassey)

References

(Blocks 1 open bug)

Details

(Whiteboard: [getUserMedia], [blocking-gum-])

Attachments

(3 files, 4 obsolete files)

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)
Whiteboard: [getUserMedia], [blocking-gum-]
QA Contact: jsmith
Might be worth revisiting some of the reasons we decided not to expose the canvas drawWindow method to content...
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.
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.
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?
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.
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/
Blocks: 906956
Attached patch WIP patch (obsolete) — Splinter Review
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
Attachment #792947 - Flags: feedback?(rjesup)
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 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.)
Attachment #792947 - Flags: feedback?(rjesup) → feedback+
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.
Attached patch tab_streaming.patch (obsolete) — Splinter Review
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.
Assignee: nobody → blassey.bugs
Attachment #792947 - Attachment is obsolete: true
Attachment #813156 - Flags: review?(rjesup)
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.
Attached patch tab_streaming.patch (obsolete) — Splinter Review
noticed that the code to add the tab video source was inside a #ifdef ANDROID block
Attachment #813156 - Attachment is obsolete: true
Attachment #813156 - Flags: review?(rjesup)
Attachment #813164 - Flags: review?(rjesup)
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
Attachment #813164 - Flags: review?(rjesup) → review+
Attached patch tab_streaming.patch (obsolete) — Splinter Review
I think it's worth getting those prefs into the initial patch, so here it is for re-review
Attachment #813164 - Attachment is obsolete: true
Attachment #818094 - Flags: review?(rjesup)
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
Attachment #818094 - Flags: review?(rjesup) → review-
Attachment #818094 - Attachment is obsolete: true
Attachment #818661 - Flags: review?(rjesup)
Blocks: 928096
Changing description to reflect what's being done here.
Summary: Add support for screen/window sharing for getUserMedia → Add support for tab sharing for getUserMedia
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()
Attachment #818661 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/c5391fab9cc1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Brad, new files here didn't include a license header. Can you fix that ASAP? Thanks!
Flags: needinfo?(blassey.bugs)
Attachment #828660 - Flags: review?(Ms2ger)
Flags: needinfo?(blassey.bugs)
(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
Depends on: 952166
Comment on attachment 828660 [details] [diff] [review]
license_header_tab_source.patch

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

Thanks.
Attachment #828660 - Flags: review?(Ms2ger)
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.
There isn't a good API for that yet
(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.
Thanks! So is Mozilla working on this API? Is there a codebase we can help with? https://www.tricksbysmt.com
Flags: needinfo?(blassey.bugs)
Flags: needinfo?(blassey.bugs) → needinfo?(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.
Flags: needinfo?(mreavy)
Flags: needinfo?(mreavy)
Flags: needinfo?(harshalj5)
You need to log in before you can comment on or make changes to this bug.