1144912 - Screenshare framerate should be requestable from GUM constraint.

Status: RESOLVED DUPLICATE of bug 1211658

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

Description

[Ethan Hugg [:ehugg]]

I would expect that setting a GUM constraint on a screen, window or app share could affect the frame rate of the video stream.  For example:
        frameRate: {
           min: '1',
           max: '3'
        }
could create a stream at 3fps.  

There does not seem to be a way to request frame rates from a GUM constraint for screensharing.  It always sends the same regardless of settings.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: WebRTC Screenshare has own default FPS and can be set from constraints.

Comment 2

[Ethan Hugg [:ehugg]]

Comment on attachment 8581825 [details] [diff] [review]
WebRTC Screenshare has own default FPS and can be set from constraints.

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

Here's my suggestion.  Instead of starting all screen/window/app shares with 30FPS, we have a separate default value.  Also we allow the starting FPS to be settable to the requested max value from the constraints.

Requesting feedback instead of review so we can first agree on what should be done.

Comment 3

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

Comment on attachment 8581825 [details] [diff] [review]
WebRTC Screenshare has own default FPS and can be set from constraints.

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

Min and max in constraints are well-defined modifiers of a primary value, and must not be overloaded with meaning by the browser. The primary constraint here is frameRate.

AFAICT, despite its under-delivery-condoning name, capability.maxFPS is the requested frameRate and not really a max value as far as the control surface is concerned, so we should instead investigate why the existing code is not responding to a plain { frameRate: 3 } or { frameRate: { exact: 3 } or { frameRate: { min: 1, max: 3 } constraint.

::: dom/media/webrtc/MediaEngineCameraVideoSource.cpp
@@ +312,5 @@
> +    double maxFPS = GetMaxRequestedFPS(aConstraints);
> +    if (maxFPS > 0) {
> +      mCapability.maxFPS = maxFPS;
> +    }
> +  }

This code is the wrong fix.

::: dom/media/webrtc/MediaEngineCameraVideoSource.h
@@ +85,5 @@
>    static void TrimLessFitCandidates(CapabilitySet& set);
>    virtual size_t NumCapabilities();
>    virtual void GetCapability(size_t aIndex, webrtc::CaptureCapability& aOut);
> +  bool IsScreenCapture(const dom::MediaTrackConstraintSet &aConstraints);
> +  double GetMaxRequestedFPS(const dom::MediaTrackConstraintSet &aConstraints);

In general, no need to expose these since they're private to the cpp file.

Reading info from the generated dom dictionary classes is a pita, but I still prefer inlining such code over API-displacing wrappers.

I find switch-statements helpful here [1]

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp?rev=1b66d8f67cf7&mark=1667-1674#1638

::: media/webrtc/trunk/webrtc/video_engine/vie_capturer.cc
@@ +211,2 @@
>  
>    if (!CaptureCapabilityFixed()) {

This code already seems to agree that capture_capability can be set by the caller. If useful values are not coming in here in this structure then we should investigate why.

@@ +222,5 @@
> +      if (type == Camera) {
> +        frame_rate = kViECaptureDefaultFramerate;
> +      } else {
> +        frame_rate = requested_capability_.maxFPS != 0 ?
> +          requested_capability_.maxFPS : kViEScreenCaptureDefaultFramerate;

Just do: frame_rate = kViEScreenCaptureDefaultFramerate;

::: media/webrtc/trunk/webrtc/video_engine/vie_defines.h
@@ +40,5 @@
>  enum { kViEMaxCaptureDevices = 256 };
>  enum { kViECaptureDefaultWidth = 352 };
>  enum { kViECaptureDefaultHeight = 288 };
>  enum { kViECaptureDefaultFramerate = 30 };
> +enum { kViEScreenCaptureDefaultFramerate = 5 };

Individual defaults is a good idea.

Comment 4

[Ethan Hugg [:ehugg]]

> ::: media/webrtc/trunk/webrtc/video_engine/vie_capturer.cc
> @@ +211,2 @@
> >  
> >    if (!CaptureCapabilityFixed()) {
> 
> This code already seems to agree that capture_capability can be set by the
> caller. If useful values are not coming in here in this structure then we
> should investigate why.

That would require width and height to also be specified which I don't want to require in the screen capture case.

> @@ +222,5 @@
> > +      if (type == Camera) {
> > +        frame_rate = kViECaptureDefaultFramerate;
> > +      } else {
> > +        frame_rate = requested_capability_.maxFPS != 0 ?
> > +          requested_capability_.maxFPS : kViEScreenCaptureDefaultFramerate;
> 
> Just do: frame_rate = kViEScreenCaptureDefaultFramerate;
> 
> ::: media/webrtc/trunk/webrtc/video_engine/vie_defines.h
> @@ +40,5 @@
> >  enum { kViEMaxCaptureDevices = 256 };
> >  enum { kViECaptureDefaultWidth = 352 };
> >  enum { kViECaptureDefaultHeight = 288 };
> >  enum { kViECaptureDefaultFramerate = 30 };
> > +enum { kViEScreenCaptureDefaultFramerate = 5 };
> 
> Individual defaults is a good idea.

I'll take a step back and propose a patch that just does this first then.

Comment 5

[Ethan Hugg [:ehugg]]

Attachment: WebRTC Screenshare has own default FPS

Comment 6

[Ethan Hugg [:ehugg]]

Comment on attachment 8581933 [details] [diff] [review]
WebRTC Screenshare has own default FPS

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

I pulled down the latest from webrtc.org and this looks like it has not already been done there.  This will set the starting FPS for all screen/window/app shares.

Comment 7

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

Comment on attachment 8581933 [details] [diff] [review]
WebRTC Screenshare has own default FPS

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

lgtm.

::: media/webrtc/trunk/webrtc/video_engine/vie_capturer.cc
@@ +219,5 @@
>        height = kViECaptureDefaultHeight;
>      }
>      if (frame_rate == 0) {
> +      if (type == Camera) {
> +        frame_rate = kViECaptureDefaultFramerate;

nit: given the naming of the constants (ScreenCapture vs. Default) I would have expected the type test to lean the other way on unknown values.

Comment 8

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

(In reply to Ethan Hugg [:ehugg] from comment #4)
> > >    if (!CaptureCapabilityFixed()) {
> > 
> > This code already seems to agree that capture_capability can be set by the
> > caller. If useful values are not coming in here in this structure then we
> > should investigate why.
> 
> That would require width and height to also be specified which I don't want
> to require in the screen capture case.

Maybe I'm reading the code wrong, but doesn't it allow for passed-in (non-zero) values even in the "non-fixed" case?

Comment 9

[Ethan Hugg [:ehugg]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #7)
> Comment on attachment 8581933 [details] [diff] [review]
> WebRTC Screenshare has own default FPS
> 
> Review of attachment 8581933 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm.
> 
> ::: media/webrtc/trunk/webrtc/video_engine/vie_capturer.cc
> @@ +219,5 @@
> >        height = kViECaptureDefaultHeight;
> >      }
> >      if (frame_rate == 0) {
> > +      if (type == Camera) {
> > +        frame_rate = kViECaptureDefaultFramerate;
> 
> nit: given the naming of the constants (ScreenCapture vs. Default) I would
> have expected the type test to lean the other way on unknown values.

Sure, I will check for Screen/Window/Application and default the other way and post that patch shortly.

Comment 10

[Ethan Hugg [:ehugg]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #8)
> (In reply to Ethan Hugg [:ehugg] from comment #4)
> > > >    if (!CaptureCapabilityFixed()) {
> > > 
> > > This code already seems to agree that capture_capability can be set by the
> > > caller. If useful values are not coming in here in this structure then we
> > > should investigate why.
> > 
> > That would require width and height to also be specified which I don't want
> > to require in the screen capture case.
> 
> Maybe I'm reading the code wrong, but doesn't it allow for passed-in
> (non-zero) values even in the "non-fixed" case?


It looks to me that it uses values from GetBestFormat which calls GetPreferedFrameSettings on the observers which I believe is the encoder.  Do you see a way for values from the constraints to end up there?  Perhaps I'm looking in the wrong place.

Comment 11

[Ethan Hugg [:ehugg]]

Attachment: WebRTC Screenshare has own default FPS

Comment 12

[Ethan Hugg [:ehugg]]

Comment on attachment 8582119 [details] [diff] [review]
WebRTC Screenshare has own default FPS

Bringing forward r+ from jib.

Comment 13

[Ethan Hugg [:ehugg]]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c52136c7d924

Comment 14

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

(In reply to Ethan Hugg [:ehugg] from comment #10)
> > Maybe I'm reading the code wrong, but doesn't it allow for passed-in
> > (non-zero) values even in the "non-fixed" case?
> 
> It looks to me that it uses values from GetBestFormat which calls
> GetPreferedFrameSettings on the observers which I believe is the encoder. 
> Do you see a way for values from the constraints to end up there?  Perhaps
> I'm looking in the wrong place.

My mistake, you're right. The code ignores requested settings unless one provides all three (width, height and frameRate). But even if it has internal reasons for doing that, that's wrong. It needs to respect and accommodate constraints no matter how many.

If the implementation can handle width, height and frameRate orthogonally, then this should be simple to fix, just take requested properties into account in whatever dimensions provided. If not, then it needs to tie into the ideal algorithm, which I can help with.

Comment 15

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

Here's a shortcut helper class for when constraints are orthogonal: http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineTabVideoSource.cpp?rev=64f3fa7a5a8d&mark=133-141#121

Comment 16

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae7215f501a5

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

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

Attachment: WIP patch to allow specifying framerate and resolution for screencapture

WIP, but seems to work for screencaptures at least on linux.  Also fixes FPS calculation bug

Comment 19

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

Comment on attachment 8591840 [details] [diff] [review]
WIP patch to allow specifying framerate and resolution for screencapture

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

::: dom/media/webrtc/MediaEngineCameraVideoSource.h
@@ +31,5 @@
>      , mTrackID(0)
>      , mFps(-1)
> +    , mBufWidthMax(0)
> +    , mBufHeightMax(0)
> +    , mFrameRate(0)

Unused?

@@ +116,5 @@
>    TrackID mTrackID;
>    int mFps; // Track rate (30 fps by default)
> +  uint32_t mBufWidthMax;
> +  uint32_t mBufHeightMax;
> +  double mFrameRate;

Unused?

::: dom/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +234,5 @@
> +    mBufWidthMax = c.mWidth.Clamp(c.mWidth.mIdeal.WasPassed() ?
> +      c.mWidth.mIdeal.Value() : 0);
> +    mBufHeightMax = c.mHeight.Clamp(c.mHeight.mIdeal.WasPassed() ?
> +      c.mHeight.mIdeal.Value() : 0);
> +    mFrameRate = c.mFrameRate.Clamp(c.mFrameRate.mIdeal.WasPassed() ?

AFAICT you never use mBufWidthMax, mBufHeightMax and mFrameRate for anything so this code seems redundant and can be removed.

::: media/webrtc/trunk/webrtc/video_engine/vie_capturer.cc
@@ +210,5 @@
>    CaptureDeviceType type = config_.Get<CaptureDeviceInfo>().type;
>  
>    if (!CaptureCapabilityFixed()) {
>      // Ask the observers for best size.
>      GetBestFormat(&width, &height, &frame_rate);

Looks good, except why is the GetBestFormat() function allowed to override passed-in values?

I think the order needs to be:

 !!capture_capability > GetBestFormat() > kViECaptureDefault

capture_capability contains the net result of both required and optional constraints, so if this order is not possible then we'd need more info down here.

@@ +232,2 @@
>        } else {
> +        // XXX What about Tabs?  (and Browser?)

Tab sharing code is here [1]

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineTabVideoSource.cpp?rev=b45c9c38c9dc#122

Comment 20

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #19)

> ::: dom/media/webrtc/MediaEngineWebRTCVideo.cpp
> @@ +234,5 @@
> > +    mBufWidthMax = c.mWidth.Clamp(c.mWidth.mIdeal.WasPassed() ?
> > +      c.mWidth.mIdeal.Value() : 0);
> > +    mBufHeightMax = c.mHeight.Clamp(c.mHeight.mIdeal.WasPassed() ?
> > +      c.mHeight.mIdeal.Value() : 0);
> > +    mFrameRate = c.mFrameRate.Clamp(c.mFrameRate.mIdeal.WasPassed() ?
> 
> AFAICT you never use mBufWidthMax, mBufHeightMax and mFrameRate for anything
> so this code seems redundant and can be removed.

Huh?  Check the code in ::Start(); that's the whole point - they modify mCapability

> ::: media/webrtc/trunk/webrtc/video_engine/vie_capturer.cc
> @@ +210,5 @@
> >    CaptureDeviceType type = config_.Get<CaptureDeviceInfo>().type;
> >  
> >    if (!CaptureCapabilityFixed()) {
> >      // Ask the observers for best size.
> >      GetBestFormat(&width, &height, &frame_rate);
> 
> Looks good, except why is the GetBestFormat() function allowed to override
> passed-in values?

Reasonable point (though largely moot as we have no observers for GetBestFormat())

Comment 21

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

Attachment: Allow specifying framerate and resolution for screen/window/app capture

Also corrects a mistake in wait-time calculation for non-windows to hit the
target rate

Comment 22

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

Comment on attachment 8592253 [details] [diff] [review]
Allow specifying framerate and resolution for screen/window/app capture

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

Ah, sorry I misread the code. I see what it's doing now, but it's going about it in a roundabout way. I'd rather move the code a bit.
It's also tripping up camera code, so I would like to see it again.

::: dom/media/webrtc/MediaEngineCameraVideoSource.h
@@ +31,5 @@
>      , mTrackID(0)
>      , mFps(-1)
> +    , mBufWidthMax(0)
> +    , mBufHeightMax(0)
> +    , mFrameRate(0)

This shouldn't be necessary with changes suggested.

@@ +116,5 @@
>    TrackID mTrackID;
>    int mFps; // Track rate (30 fps by default)
> +  uint32_t mBufWidthMax;
> +  uint32_t mBufHeightMax;
> +  double mFrameRate;

Or this.

::: dom/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +335,5 @@
>    }
>  
> +  if (mBufWidthMax > 0 &&
> +      (mCapability.width == 0 || mBufWidthMax < mCapability.width)) {
> +    mCapability.width = mBufWidthMax;

The "|| mBufWidthMax < mCapability.width" part is not ok. This codepath is for both cameras and screensharing, so this overrides what ChooseCapability() just found with its constraints algorithm.

In other words, only touch mCapability.width if it is zero, then this should be fine. Same for height and frameRate.

But mBufWidthMax and friends seem to exist solely to defer mutating mCapability until after AllocateCaptureDevice(). Unless I'm mistaken this shouldn't be necessary, and it might be more correct to set it ahead with the other mCapabilities?

If my assumption is correct then I'd prefer to move this poking of zero mCapability members to the tail of the ChooseCapability() function itself, after the rest of the constraint code, right before we log the capabilities chosen.

Here: http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineCameraVideoSource.cpp?rev=ce72894838de#322

Makes it accessible to B2G as well should it need it.

This should work well for both cameras and screensharing, and I might even take advantage of this code on OSX and replace the hardcoded 30 fps with 0, since OSX seems capable of producing any frameRate asked for.

Comment 23

[Maire Reavy [:mreavy]]

Randell -- Can you finish this off?

Comment 24

[steinbrecher.johann]

https://bugzilla.mozilla.org/show_bug.cgi?id=1144912

Comment 25

[steinbrecher.johann]

https://bugzilla.mozilla.org/show_bug.cgi?id=1211658 has been merged. Good to close?

Comment 26

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

Yes.