1141622 - GUM constraint maxFramerate set below 30 for screenshare fails on FF38+

Status: RESOLVED FIXED

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

Description

[Ethan Hugg [:ehugg]]

On FF37 and older we could set the frameRate on a screenshare like this:
         frameRate: {
           min: '1',
           max: '3'
        }

Now as of FF38 this fails and the max value must be set to 30 or larger in order to succeeed.

This appears to be caused by the first patch from bug 1119335

30 is very fast for screenshare and we should be able to set the rate below that.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: gumconstraints.html

Test page for screenshare with frameRate constraint set.  See line 134.  Works on 37, fails on 38 unless you set the max to >30.

Comment 2

[Ethan Hugg [:ehugg]]

In particular FitnessDistance returns UINT32_MAX here because a screenshare looks like a camera with a single option of an fps of 30.

https://dxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineCameraVideoSource.cpp#76

Comment 3

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

Attachment: unregress screensharing frameRate by moving hardcoded capabilities down stack

Fix regression by no longer failing on frameRate constraint, though I doubt it actually did anything (it looks like screenshare updates more frequently to me).

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cdd2f3dd620

Comment 4

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

Attachment: unregress screensharing frameRate by moving hardcoded capabilities down stack (2)

Correct patch this time.

Comment 5

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

Comment on attachment 8575762 [details] [diff] [review]
unregress screensharing frameRate by moving hardcoded capabilities down stack (2)

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

I'd rather not push our hard-coded capabilities that far down into upstream code.  Instead, I'd rather either revert the change and make screenshare sources a separate class (that overrides with 0's), or have the hardcoding be in MediaEngineWebRTCVideoSource and key off WEBRTC_MAC and check aMediaSource before installing the hardcoded mac values.  Probably the second, as there is little difference, and we already have an mMediaSource (though it's really just used for "am I a camera" tests).  Depends on how O-Oish we want to be.

Rest of the comments are based on keeping the structure you have

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/qtkit/video_capture_qtkit_info.h
@@ +85,5 @@
>      virtual int32_t CreateCapabilityMap(
>          const char* deviceUniqueIdUTF8);
>  
>      VideoCaptureMacQTKitInfoObjC*    _captureInfo;
> +    VideoCaptureCapability _hardcodedCapabilities[25];

#define MAX_HARDCODED_CAPABILITIES 25 (or whatever)

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/qtkit/video_capture_qtkit_info.mm
@@ +39,5 @@
>      _captureInfo = [[VideoCaptureMacQTKitInfoObjC alloc] init];
> +
> +    // Hardcode generic OSX capabilities modeled on OSX cameras.
> +    // Note: Values are empirically picked to be OSX friendly, as on OSX, values
> +    // other than these cause the source to not produce.

#if defined(WEBRTC_MOZILLA_BUILD)

@@ +41,5 @@
> +    // Hardcode generic OSX capabilities modeled on OSX cameras.
> +    // Note: Values are empirically picked to be OSX friendly, as on OSX, values
> +    // other than these cause the source to not produce.
> +
> +    for (int i = 0; i < 9; i++) {

Assert index < MAX_HARDCODED_CAPABILITIES

@@ +99,3 @@
>      // "analog". QTKit will do it's best to convert frames to what ever format
>      // you ask for.
> +    return sizeof(_hardcodedCapabilities) / sizeof(*_hardcodedCapabilities);

#if defined(WEBRTC_MOZILLA_BUILD)
...
#else
...
#endif

@@ +114,5 @@
> +        sizeof(_hardcodedCapabilities) / sizeof(*_hardcodedCapabilities)) {
> +      return -1;
> +    }
> +    capability = _hardcodedCapabilities[deviceCapabilityNumber];
> +    return 0;

ditto

::: media/webrtc/trunk/webrtc/video_engine/vie_capture_impl.cc
@@ -316,5 @@
> -  // Thus this function cannot be supported on the Mac platform.
> -  shared_data_->SetLastError(kViECaptureDeviceMacQtkitNotSupported);
> -  LOG_F(LS_ERROR) << "API not supported on Mac OS X.";
> -  return -1;
> -#endif

This will cause problems with upstream merges.

#if defined(WEBRTC_MAC) && !defined(WEBRTC_MOZILLA_BUILD)

@@ -327,5 @@
>                                           const unsigned int unique_idUTF8Length,
>                                           const unsigned int capability_number,
>                                           CaptureCapability& capability) {
>  
> -#if defined(WEBRTC_MAC)

ditto

Comment 6

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

Attachment: unregress screensharing frameRate by limiting hardcoded capabilities to osx camera

Moved back to MediaEngineWebRTCVideo.cpp keying off mMediaSource and XP_MACOSX (WEBRTC_MAC was not defined here).

Comment 7

[Maire Reavy [:mreavy]]

jib -- can you ask for Aurora approval as soon as this merges to central?  Thanks.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/308282e7946b

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b0b98f81a14

Comment 10

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

Comment on attachment 8576732 [details] [diff] [review]
unregress screensharing frameRate by limiting hardcoded capabilities to osx camera

Approval Request Comment
[Feature/regressing bug #]: Bug 1119335
[User impact if declined]: Requests for screensharing fail outright if request demands frameRates lower than 30.
[Describe test coverage new/current, TreeHerder]:
Fix confirmed locally. Landed on m-c.
[Risks and why]: 
Low risk. The new addition of OSX default capabilities kicked in where it shouldn't, specifically on the sceensharing devices, making them falsely act as if the did not support lower framerates, when they do. This fix limits OSX defaults to OSX and cameras, and gives other devices a friendlier default of one capability that claims to handle all widths/heights/framerates, which is safer in lieu of information to the contrary, is in line with previous behavior, and is a good match for screensharing devices where parameter limitations are mostly orthogonal anyway, and reporting accurate limits matters less since there are no competing devices.

[String/UUID change made/needed]: none

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/308282e7946b
https://hg.mozilla.org/mozilla-central/rev/8b0b98f81a14

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a18090955c7