1041700 - add browserArgs to gUM constraints

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

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

Attachment: browserArgs.patch

We want to be able to specify the tab to be captured from chrome code without showing the tab chooser dialog.

Note, I may want to extend this to specify height and width of the area of the page to be captured. Right now the size of the area being captured and the size of the video being encoded are the same, but that's proving to be too big (and unnecessarily big) on some high dpi phones

Comment 1

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

Comment on attachment 8459743 [details] [diff] [review]
browserArgs.patch

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

r? jib for constraints; you'll need r? DOM peer

::: content/media/webrtc/MediaEngineTabVideoSource.h
@@ +63,4 @@
>  private:
>      int mBufW;
>      int mBufH;
> +    long mWindowId;

Is there a reason we're using generic types here? 
"because IDL" I imagine :-/

::: dom/webidl/Constraints.webidl
@@ +31,5 @@
>      unrestricted double min = -Infinity;
>      unrestricted double max = Infinity;
>  };
> +
> +dictionary ConstrainBrowserArgs {

see the other webidl

::: dom/webidl/MediaTrackConstraintSet.webidl
@@ +13,5 @@
>      "width",
>      "height",
>      "frameRate",
> +    "mediaSource",
> +    "browserArgs"

mozBrowserArgs for now I think; not even proposed to the WG - jib?
Even if it's moz internal only, let's prefix it (maybe mozInternal then?)

@@ +28,4 @@
>      ConstrainDoubleRange frameRate;
>      ConstrainVideoFacingMode facingMode;
>      ConstrainMediaSource mediaSource = "camera";
> +    ConstrainBrowserArgs browserArgs;

ditto

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8459743 [details] [diff] [review]
browserArgs.patch

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

r=me for whatever the media guys are cool with

Comment 3

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

bikshedding warning!

browserArgs makes me cringe. Aren't they all "args" of some kind? Also, "browser" is not really true either, and it makes the constraints too specific to the use case. What about just calling it "window" ?

Comment 4

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

(In reply to Mark Finkle (:mfinkle) from comment #3)
> bikshedding warning!
> 
> browserArgs makes me cringe. Aren't they all "args" of some kind? Also,
> "browser" is not really true either, and it makes the constraints too
> specific to the use case. What about just calling it "window" ?

on the browser vs. window, in the context of gUM, these are browsers, where windows are application windows outside of the user agent.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8459743 [details] [diff] [review]
browserArgs.patch

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

browserArgs seems like a grouping-attempt at context. For better or worse, the WG decided long ago to dump all constrainable properties into the same dictionary. e.g. frameRate and sampleRate are siblings even though one is used solely in video: {} and the other in audio: {} http://dev.w3.org/2011/webrtc/editor/getusermedia.html#media-track-constraints

Or maybe browserArgs is our first multi-member datatype like a Vector, but since we didn't even do that for width/height, I don't recommend it, and the definitions are in the wrong place for that (there's the basic datatype and then there's the Constrain* dictionary that works to constrain a range or set of values of that type). Doesn't seem like a fit.

I recommend making two discrete constraints instead, and shoot for generic names unlikely to clash too badly, something like browserTab and scrollWithPage.

::: content/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +47,1 @@
>    return NS_OK;

(I actually like this style but), missing {}

@@ +56,5 @@
>      mVideoSource->mTimer->Cancel();
>      mVideoSource->mTimer = nullptr;
>    }
> +  if (mVideoSource->mTabSource)
> +    mVideoSource->mTabSource->NotifyStreamStop(mVideoSource->mWindow);

{}

::: content/media/webrtc/MediaEngineTabVideoSource.h
@@ +63,4 @@
>  private:
>      int mBufW;
>      int mBufH;
> +    long mWindowId;

We should use int32_t in place of long since that's what the generated c++ bindings use (and int64_t in this case with the other changes suggested).

::: dom/media/MediaManager.cpp
@@ +1477,5 @@
>  #endif
>  
> +  if (c.mVideo.IsMediaTrackConstraints()) {
> +    auto& tc = c.mVideo.GetAsMediaTrackConstraints();
> +    if (tc.mBrowserArgs.mWindowId != -1 && !aPrivileged) {

With the other suggestions you'll have to check for .WasPassed() among other changes.

::: dom/webidl/Constraints.webidl
@@ +32,5 @@
>      unrestricted double max = Infinity;
>  };
> +
> +dictionary ConstrainBrowserArgs {
> +  long windowId = -1;

Isn't windowId 64-bit? And move to MediaTrackConstraintSet.webidl as:

  long long browserTab;

There shouldn't be any changes in this file unless you're adding another enum or datatype. Since long long is a first for constraints, if you want to be thorough you can define ConstrainLongLongRange here though it makes little sense for windowIds I imagine.

@@ +33,5 @@
>  };
> +
> +dictionary ConstrainBrowserArgs {
> +  long windowId = -1;
> +  boolean scrollWithPage = true;

boolean scrollWithPage;

Also move, and I would avoid defaults on constraint members as they have unfortunate side-effects in the constraints algorithm as we discovered with mediaSource (the distinction of whether something was passed or not matters in the algorithm of the advanced array).

There can still be a default in the implementation of course, we just can't specify it in the webidl without side-effects.

::: dom/webidl/MediaTrackConstraintSet.webidl
@@ +13,5 @@
>      "width",
>      "height",
>      "frameRate",
> +    "mediaSource",
> +    "browserArgs"

Randell mentioned prefixing, we have a mix. mediaSource isn't standard either, but we were getting pushback internally on prefixes so my best guess is we go unprefixed.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8459743 [details] [diff] [review]
browserArgs.patch

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

::: content/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +42,5 @@
>    mVideoSource->Draw();
>    mVideoSource->mTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
>    mVideoSource->mTimer->InitWithCallback(mVideoSource, mVideoSource->mTimePerFrame, nsITimer:: TYPE_REPEATING_SLACK);
> +  if (mVideoSource->mTabSource)
> +    mVideoSource->mTabSource->NotifyStreamStart(mVideoSource->mWindow);

(I actually like this style but), missing {}

Comment 7

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

Attachment: browserArgs.patch

Comment 8

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

Attachment: use_browserArgs.patch

Comment 9

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

Attachment: browserArgs.patch

removed the ws change to Constraintes.webidl

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8464820 [details] [diff] [review]
browserArgs.patch

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

Sorry, I found more stuff I missed the first time. r- as I would like to see it one more time.

::: content/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +129,5 @@
>      for (uint32_t i = 0; i < advanced.Length(); i++) {
>        if (cWidth.mMax >= advanced[i].mWidth.mMin && cWidth.mMin <= advanced[i].mWidth.mMax &&
> +         cHeight.mMax >= advanced[i].mHeight.mMin && cHeight.mMin <= advanced[i].mHeight.mMax) {
> +        cWidth.mMin = std::max(cWidth.mMin, advanced[i].mWidth.mMin);
> +        cHeight.mMin = std::max(cHeight.mMin, advanced[i].mHeight.mMin);

ws change?

@@ +151,4 @@
>    }
>  
>    mTimePerFrame = aPrefs.mFPS ? 1000 / aPrefs.mFPS : aPrefs.mFPS;
> +  mWindowId = aConstraints.mBrowserWindow.WasPassed() ? aConstraints.mBrowserWindow.Value() : -1;

browserWindow may also appear in the advanced array. The advanced algorithm - http://dev.w3.org/2011/webrtc/editor/getusermedia.html#methods-5 - is quite complicated and does not appear to be implemented in this file. That's OK only as long as the constraints supported are few and non-conflicting, which seems to be the case here.

But we should at least look for the first occurrence of browserWindow in the advanced array IF it was not passed at the top level. See code for width and height above.

@@ +151,5 @@
>    }
>  
>    mTimePerFrame = aPrefs.mFPS ? 1000 / aPrefs.mFPS : aPrefs.mFPS;
> +  mWindowId = aConstraints.mBrowserWindow.WasPassed() ? aConstraints.mBrowserWindow.Value() : -1;
> +  mScrollWithPage = aConstraints.mScrollWithPage.WasPassed() ? aConstraints.mScrollWithPage.Value() : true;

Same for scrollWithPage.

::: dom/media/MediaManager.cpp
@@ +1477,5 @@
>  #endif
>  
> +  if (c.mVideo.IsMediaTrackConstraints()) {
> +    auto& tc = c.mVideo.GetAsMediaTrackConstraints();
> +    if (tc.mBrowserWindow.WasPassed() && !aPrivileged) {

This test is no longer sufficient once you add code to look for browserWindow in the advanced array. Either hunt those down as well or find a way to push this test down past the constraint stuff.

@@ +1479,5 @@
> +  if (c.mVideo.IsMediaTrackConstraints()) {
> +    auto& tc = c.mVideo.GetAsMediaTrackConstraints();
> +    if (tc.mBrowserWindow.WasPassed() && !aPrivileged) {
> +      // only allow privileged content to set the window id
> +      tc.mBrowserWindow.Construct(-1);

tc.mBrowserWindow = -1; (never needs Construct since we know it was passed)

::: dom/webidl/MediaTrackConstraintSet.webidl
@@ +42,4 @@
>  
>  // TODO: Bug 767924 sequences in unions
>  //typedef (VideoFacingModeEnum or sequence<VideoFacingModeEnum>) ConstrainVideoFacingMode;
> +//typedef (MediaSourceEnum or sequence<MediaSourceEnum>) ConstrainMediaSource;

no-op change?

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Also please add the following lines here http://mxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaTrackConstraints.h?mark=103-103#99 :

> Triage(Kind::BrowserWindow).mBrowserWindow = mBrowserWindow;
> Triage(Kind::ScrollWithPage).mScrollWithPage = mScrollWithPage;

The need for this will go away once we fix Bug 1033885.

Comment 12

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #11)
> Also please add the following lines here
> http://mxr.mozilla.org/mozilla-central/source/content/media/webrtc/
> MediaTrackConstraints.h?mark=103-103#99 :
> 
> > Triage(Kind::BrowserWindow).mBrowserWindow = mBrowserWindow;
> > Triage(Kind::ScrollWithPage).mScrollWithPage = mScrollWithPage;
> 
> The need for this will go away once we fix Bug 1033885.

when I add these I get the following errors:

In file included from /Users/blassey/src/fx-team/content/media/webrtc/MediaEngineWebRTC.cpp:35:0:
/Users/blassey/src/fx-team/content/media/webrtc/MediaTrackConstraints.h: In constructor 'mozilla::VideoTrackConstraintsN::VideoTrackConstraintsN(const mozilla::dom::MediaTrackConstraints&)':
/Users/blassey/src/fx-team/content/media/webrtc/MediaTrackConstraints.h:103:50: error: use of deleted function 'mozilla::dom::Optional<long long int>& mozilla::dom::Optional<long long int>::operator=(const mozilla::dom::Optional<long long int>&)'
In file included from ../../../dist/include/mozilla/dom/StorageTypeBinding.h:9:0,
                 from ../../../dist/include/mozilla/dom/quota/PersistenceType.h:12,
                 from ../../../dist/include/mozilla/dom/indexedDB/FileManager.h:15,
                 from ../../../dist/include/mozilla/dom/indexedDB/FileInfo.h:12,
                 from ../../../dist/include/nsDOMFile.h:31,
                 from /Users/blassey/src/fx-team/content/media/webrtc/MediaEngineWebRTC.h:16,
                 from /Users/blassey/src/fx-team/content/media/webrtc/MediaEngineWebRTC.cpp:30:
../../../dist/include/mozilla/dom/BindingDeclarations.h:173:7: note: 'mozilla::dom::Optional<long long int>& mozilla::dom::Optional<long long int>::operator=(const mozilla::dom::Optional<long long int>&)' is implicitly deleted because the default definition would be ill-formed:
../../../dist/include/mozilla/dom/BindingDeclarations.h:166:24: error: 'const mozilla::dom::Optional_base<T, InternalType>& mozilla::dom::Optional_base<T, InternalType>::operator=(const mozilla::dom::Optional_base<T, InternalType>&) [with T = long long int; InternalType = long long int; mozilla::dom::Optional_base<T, InternalType> = mozilla::dom::Optional_base<long long int, long long int>]' is private
../../../dist/include/mozilla/dom/BindingDeclarations.h:173:7: error: within this context
In file included from /Users/blassey/src/fx-team/content/media/webrtc/MediaEngineWebRTC.cpp:35:0:
/Users/blassey/src/fx-team/content/media/webrtc/MediaTrackConstraints.h:104:52: error: use of deleted function 'mozilla::dom::Optional<bool>& mozilla::dom::Optional<bool>::operator=(const mozilla::dom::Optional<bool>&)'
In file included from ../../../dist/include/mozilla/dom/StorageTypeBinding.h:9:0,
                 from ../../../dist/include/mozilla/dom/quota/PersistenceType.h:12,
                 from ../../../dist/include/mozilla/dom/indexedDB/FileManager.h:15,
                 from ../../../dist/include/mozilla/dom/indexedDB/FileInfo.h:12,
                 from ../../../dist/include/nsDOMFile.h:31,
                 from /Users/blassey/src/fx-team/content/media/webrtc/MediaEngineWebRTC.h:16,
                 from /Users/blassey/src/fx-team/content/media/webrtc/MediaEngineWebRTC.cpp:30:
../../../dist/include/mozilla/dom/BindingDeclarations.h:173:7: note: 'mozilla::dom::Optional<bool>& mozilla::dom::Optional<bool>::operator=(const mozilla::dom::Optional<bool>&)' is implicitly deleted because the default definition would be ill-formed:
../../../dist/include/mozilla/dom/BindingDeclarations.h:166:24: error: 'const mozilla::dom::Optional_base<T, InternalType>& mozilla::dom::Optional_base<T, InternalType>::operator=(const mozilla::dom::Optional_base<T, InternalType>&) [with T = bool; InternalType = bool; mozilla::dom::Optional_base<T, InternalType> = mozilla::dom::Optional_base<bool, bool>]' is private
../../../dist/include/mozilla/dom/BindingDeclarations.h:173:7: error: within this context


and that C++ voodoo makes my brain hurt

Comment 13

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

Attachment: browserArgs.patch

(In reply to Jan-Ivar Bruaroey [:jib] from comment #10)
> Comment on attachment 8464820 [details] [diff] [review]
> browserArgs.patch
> 
> Review of attachment 8464820 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I found more stuff I missed the first time. r- as I would like to see
> it one more time.
> 
> ::: content/media/webrtc/MediaEngineTabVideoSource.cpp
> @@ +129,5 @@
> >      for (uint32_t i = 0; i < advanced.Length(); i++) {
> >        if (cWidth.mMax >= advanced[i].mWidth.mMin && cWidth.mMin <= advanced[i].mWidth.mMax &&
> > +         cHeight.mMax >= advanced[i].mHeight.mMin && cHeight.mMin <= advanced[i].mHeight.mMax) {
> > +        cWidth.mMin = std::max(cWidth.mMin, advanced[i].mWidth.mMin);
> > +        cHeight.mMin = std::max(cHeight.mMin, advanced[i].mHeight.mMin);
> 
> ws change?
Yes, I'm removing some tab indents here


> 
> ::: dom/media/MediaManager.cpp
> @@ +1477,5 @@
> >  #endif
> >  
> > +  if (c.mVideo.IsMediaTrackConstraints()) {
> > +    auto& tc = c.mVideo.GetAsMediaTrackConstraints();
> > +    if (tc.mBrowserWindow.WasPassed() && !aPrivileged) {
> 
> This test is no longer sufficient once you add code to look for
> browserWindow in the advanced array. Either hunt those down as well or find
> a way to push this test down past the constraint stuff.

The solution I went with was to set the browserWindow to -1 whenever the caller is unprivileged. This will override and advanced constraints that are passed.

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #12)
> 50: error: use of deleted function 'mozilla::dom::Optional<long long int>&

A limitation in Optional. This seems to work:

>   if (mBrowserWindow.WasPassed()) {
>     Triage(Kind::BrowserWindow).mBrowserWindow.Construct(mBrowserWindow.Value());
>   }
>   if (mScrollWithPage.WasPassed()) {
>     Triage(Kind::ScrollWithPage).mScrollWithPage.Construct(mScrollWithPage.Value());
>   }

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8465831 [details] [diff] [review]
browserArgs.patch

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

r=me with comments here and comment 14 addressed.

::: content/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +136,5 @@
> +        cWidth.mMin = std::max(cWidth.mMin, advanced[i].mWidth.mMin);
> +        cHeight.mMin = std::max(cHeight.mMin, advanced[i].mHeight.mMin);
> +      }
> +
> +      if (mWindowId == -1 && advanced[i].mBrowserWindow.WasPassed()) {

This tramples passed-in -1 which you've overloaded elsewhere to mean disallow.

@@ +141,5 @@
> +        mWindowId = advanced[i].mBrowserWindow.Value();
> +      }
> +
> +      if (!haveScrollWithPage && advanced[i].mScrollWithPage.WasPassed()) {
> +        mScrollWithPage = advanced[i].mScrollWithPage.Value();

need to update haveScrollWithPage = true here.

@@ +163,5 @@
>    }
>  
>    mTimePerFrame = aPrefs.mFPS ? 1000 / aPrefs.mFPS : aPrefs.mFPS;
> +  mWindowId = aConstraints.mBrowserWindow.WasPassed() ? aConstraints.mBrowserWindow.Value() : -1;
> +  mScrollWithPage = aConstraints.mScrollWithPage.WasPassed() ? aConstraints.mScrollWithPage.Value() : true;

Remove now redundant code.

::: content/media/webrtc/MediaEngineTabVideoSource.h
@@ +63,4 @@
>  private:
>      int mBufW;
>      int mBufH;
> +    long mWindowId;

int64_t

::: dom/media/MediaManager.cpp
@@ +1478,5 @@
>  
> +  if (c.mVideo.IsMediaTrackConstraints() && !aPrivileged) {
> +    auto& tc = c.mVideo.GetAsMediaTrackConstraints();
> +    // only allow privileged content to set the window id
> +    tc.mBrowserWindow.Construct(-1);

You're overloading passed-in -1 to mean disallow rather than not-passed, which could work except you need to update your other code to respect this and not trample it.

Comment 16

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #15)
> Comment on attachment 8465831 [details] [diff] [review]
> browserArgs.patch
> 
> Review of attachment 8465831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaManager.cpp
> @@ +1478,5 @@
> >  
> > +  if (c.mVideo.IsMediaTrackConstraints() && !aPrivileged) {
> > +    auto& tc = c.mVideo.GetAsMediaTrackConstraints();
> > +    // only allow privileged content to set the window id
> > +    tc.mBrowserWindow.Construct(-1);
> 
> You're overloading passed-in -1 to mean disallow rather than not-passed,
> which could work except you need to update your other code to respect this
> and not trample it.

ugh... logic is hard. I think this works:

  if (c.mVideo.IsMediaTrackConstraints() && !aPrivileged) {
    auto& tc = c.mVideo.GetAsMediaTrackConstraints();
    // only allow privileged content to set the window id
    tc.mBrowserWindow.Construct(-1);
    if (tc.mAdvanced.WasPassed()) {
      const auto& advanced = tc.mAdvanced.Value();
      for (uint32_t i = 0; i < advanced.Length(); i++) {
        if (advanced[i].mBrowserWindow.WasPassed()) {
          advanced[i].mBrowserWindow.Construct(-1);
        }
      }
  }
and leaving the treatment of advanced alone in MediaEngineTabVideoSource.cpp

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

That should work. Two nits: The const auto reference seems inappropriate, and again you don't need Construct when wasPassed is already true.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/f8704e9dd82c
https://hg.mozilla.org/mozilla-central/rev/8d11683f71b3