Bug 198840 – Changing settings of a MediaStreamTrack clone should not alter the settings of the original track

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

Especially video resolution.

[youenn fablet]

Created attachment 372437 [details]
Patch

[EWS Watchlist]

Attachment 372437 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:99:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 372460 [details]
Patch

[EWS Watchlist]

Attachment 372460 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:99:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 372460 [details]
Patch

Attachment 372460 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12520994

New failing tests:
platform/ios/mediastream/getUserMedia-single-capture.html
platform/ios/mediastream/audio-muted-in-background-tab.html
platform/ios/mediastream/video-muted-in-background-tab.html

[EWS Watchlist]

Created attachment 372466 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5

[youenn fablet]

Created attachment 372479 [details]
Patch

[EWS Watchlist]

Attachment 372479 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:99:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 372479 [details]
Patch

Patch ready to review.
The idea behind this patch would be to have RealtimeVideoSource be fully responsible for frame rate/size adaptation.
This would remove the need for RealtimeVideoCaptureSource to be a RealtimeMediaSource , which might simplify things. It could instead become a RealtimeCamera or something like that.

[Eric Carlson]

Comment on attachment 372479 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372479&action=review

> Source/WebCore/platform/mediastream/RealtimeVideoCaptureSource.cpp:185
> +        for (auto& size : standardVideoSizes()) {
> +            if (size.width() < minimumWidth || size.height() < minimumHeight)
> +                minimumAspectRatio = std::min(minimumAspectRatio, static_cast<double>(size.width()) / size.height());
> +        }

This isn't new, but I don't think this look is necessary because minimumWidth and  minimumHeight are both 1.

> Source/WebCore/platform/mediastream/RealtimeVideoCaptureSource.cpp:205
> +    const double epsilon = 0.001;
> +    return frameRate + epsilon >= range.minimum && frameRate - epsilon <= range.maximum;

Also not new, but I wonder if we could use WTF::areEssentiallyEqual here?

> Source/WebCore/platform/mediastream/RealtimeVideoCaptureSource.cpp:444
> +    if (!match)
> +        return;

Logging an error here might be helpful.

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:73
> +    if (!width)
> +        width = sourceSize.width() * height.value() / sourceSize.height();
> +    m_currentSettings.setWidth(*width);

Maybe we should ASSERT(sourceSize.width() && sourceSize.height()) since we will get a divide-by-zero crash if either is zero.

[youenn fablet]

Thanks for the review.

(In reply to Eric Carlson from comment #10)
> Comment on attachment 372479 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372479&action=review
> 
> > Source/WebCore/platform/mediastream/RealtimeVideoCaptureSource.cpp:185
> > +        for (auto& size : standardVideoSizes()) {
> > +            if (size.width() < minimumWidth || size.height() < minimumHeight)
> > +                minimumAspectRatio = std::min(minimumAspectRatio, static_cast<double>(size.width()) / size.height());
> > +        }
> 
> This isn't new, but I don't think this look is necessary because
> minimumWidth and  minimumHeight are both 1.

OK, I'll remove this one.

> 
> > Source/WebCore/platform/mediastream/RealtimeVideoCaptureSource.cpp:205
> > +    const double epsilon = 0.001;
> > +    return frameRate + epsilon >= range.minimum && frameRate - epsilon <= range.maximum;
> 
> Also not new, but I wonder if we could use WTF::areEssentiallyEqual here?

Probably, I'll do the change in a follow-up.

> > Source/WebCore/platform/mediastream/RealtimeVideoCaptureSource.cpp:444
> > +    if (!match)
> > +        return;
> 
> Logging an error here might be helpful.

OK

> > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:73
> > +    if (!width)
> > +        width = sourceSize.width() * height.value() / sourceSize.height();
> > +    m_currentSettings.setWidth(*width);
> 
> Maybe we should ASSERT(sourceSize.width() && sourceSize.height()) since we
> will get a divide-by-zero crash if either is zero.

OK

[youenn fablet]

Created attachment 372561 [details]
Patch for landing

[EWS Watchlist]

Attachment 372561 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.h:99:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 372561 [details]
Patch for landing

Clearing flags on attachment: 372561

Committed r246644: <https://trac.webkit.org/changeset/246644>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51953374>