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

RESOLVED FIXED in Firefox 38

Status

()

Core
WebRTC: Audio/Video
P1
normal
Rank:
10
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ehugg, Assigned: jib)

Tracking

unspecified
mozilla39
x86
Mac OS X
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Created attachment 8575373 [details]
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.
(Reporter)

Comment 2

3 years ago
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
Attachment #8575373 - Attachment mime type: text/plain → text/html
Assignee: nobody → jib
Created attachment 8575760 [details] [diff] [review]
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
Attachment #8575760 - Flags: review?(rjesup)
Created attachment 8575762 [details] [diff] [review]
unregress screensharing frameRate by moving hardcoded capabilities down stack (2)

Correct patch this time.
Attachment #8575760 - Attachment is obsolete: true
Attachment #8575760 - Flags: review?(rjesup)
Attachment #8575762 - Flags: review?(rjesup)
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
Attachment #8575762 - Flags: review?(rjesup) → review-
Created attachment 8576732 [details] [diff] [review]
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).
Attachment #8575762 - Attachment is obsolete: true
Attachment #8576732 - Flags: review?(rjesup)

Updated

3 years ago
Attachment #8576732 - Flags: review?(rjesup) → review+
Keywords: checkin-needed
jib -- can you ask for Aurora approval as soon as this merges to central?  Thanks.
Rank: 10
Flags: needinfo?(jib)
Flags: firefox-backlog+
Priority: -- → P1
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
Flags: needinfo?(jib)
Attachment #8576732 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/308282e7946b
https://hg.mozilla.org/mozilla-central/rev/8b0b98f81a14
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
status-firefox38: --- → affected
Attachment #8576732 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.