Bug 192632 – [MediaStream] Calculate width or height when constraints contain only the other

• Component: WebRTC
• Status: RESOLVED FIXED

[Eric Carlson]

When constraints have only width or height, and a device configuration isn't based on presets (ie. displays and windows), calculate the other dimension based on the source's intrinsic size.

[Radar WebKit Bug Importer]

<rdar://problem/46665734>

[Eric Carlson]

Created attachment 357150 [details]
Patch

[EWS Watchlist]

Comment on attachment 357150 [details]
Patch

Attachment 357150 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10371419

New failing tests:
imported/w3c/web-platform-tests/mediacapture-streams/GUM-optional-constraint.https.html

[EWS Watchlist]

Created attachment 357157 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 357150 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/mediacapture-streams/GUM-optional-constraint.https.html

[EWS Watchlist]

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

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

[Eric Carlson]

Created attachment 357174 [details]
Patch

[youenn fablet]

Comment on attachment 357174 [details]
Patch

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

> Source/WebKit/ChangeLog:17
> [phone]  Eric Carlson  <[e-mail]>

Double entry

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:890
> +    m_intrinsicSize = size;

Should we check whether m_size != this->size() before calling notifySettingsDidChangeObservers?

> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:-123
> -    const IntSize& size() const { return m_size; }

Why stopping inlining?

> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:124
> +    const IntSize intrinsicSize() const;

Could be inlined as well.

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:395
> +        if (!size.isEmpty() && size != expandedIntSize(sample.presentationSize())) {

We probably cannot have size being empty anymore since we should have an intrinsic size.
Maybe we can remove the check, or add an ASSERT() if there is a risk for converting to an empty size.

> Source/WebCore/platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp:152
> +        return intrinsicSize();

Simplify to if (frameSize.isEmpty())

> Source/WebCore/platform/mock/MockRealtimeVideoSource.h:73
> +    void setSizeAndFrameRateWithPreset(IntSize, double, RefPtr<VideoPreset>) final;

RefPtr<VideoPreset>&& would be better.

> Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:104
> +#if HAVE(IOSURFACE)

W could remove "#if HAVE(IOSURFACE)" here and make RemoteVideoSample::create return null if HAVE(IOSURFACE) is not defined

> Source/WebKit/WebProcess/cocoa/UserMediaCaptureManager.cpp:139
> +        if (!videoSampleSize.width() && !videoSampleSize.height())

if (videoSampleSize.isZero())

> Source/WebKit/WebProcess/cocoa/UserMediaCaptureManager.cpp:141
> +        else if (!videoSampleSize.width() || !videoSampleSize.height()) {

else if (!videoSampleSize.width())
    videoSampleSize.setWidth
else if (!videoSampleSize.height())
    videoSampleSize.setHeight

[Eric Carlson]

Comment on attachment 357174 [details]
Patch

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

>> Source/WebKit/ChangeLog:17
>> [phone]  Eric Carlson  <[e-mail]>
> 
> Double entry

Oops, fixed.

>> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:890
>> +    m_intrinsicSize = size;
> 
> Should we check whether m_size != this->size() before calling notifySettingsDidChangeObservers?

Good point, fixed.

>> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:-123
>> -    const IntSize& size() const { return m_size; }
> 
> Why stopping inlining?

Because the style checker complains about inline functions in exported classes.

>> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:124
>> +    const IntSize intrinsicSize() const;
> 
> Could be inlined as well.

Ditto.

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:395
>> +        if (!size.isEmpty() && size != expandedIntSize(sample.presentationSize())) {
> 
> We probably cannot have size being empty anymore since we should have an intrinsic size.
> Maybe we can remove the check, or add an ASSERT() if there is a risk for converting to an empty size.

I'll add an ASSERT for now until we're certain it isn't possible.

>> Source/WebCore/platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp:152
>> +        return intrinsicSize();
> 
> Simplify to if (frameSize.isEmpty())

Fixed.

>> Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:104
>> +#if HAVE(IOSURFACE)
> 
> W could remove "#if HAVE(IOSURFACE)" here and make RemoteVideoSample::create return null if HAVE(IOSURFACE) is not defined

RemoteVideoSample::create is only defined when HAVE(IOSURFACE) is defined and I think it is somewhat better to have an ASSERT if this is ever called when it can't succeed, so lets leave this as is.

>> Source/WebKit/WebProcess/cocoa/UserMediaCaptureManager.cpp:139
>> +        if (!videoSampleSize.width() && !videoSampleSize.height())
> 
> if (videoSampleSize.isZero())

Changed.

>> Source/WebKit/WebProcess/cocoa/UserMediaCaptureManager.cpp:141
>> +        else if (!videoSampleSize.width() || !videoSampleSize.height()) {
> 
> else if (!videoSampleSize.width())
>     videoSampleSize.setWidth
> else if (!videoSampleSize.height())
>     videoSampleSize.setHeight

OK, changed.

[Eric Carlson]

Created attachment 357228 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 357228 [details]
Patch for landing

Clearing flags on attachment: 357228

Committed r239163: <https://trac.webkit.org/changeset/239163>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

Reopening to attach new patch.

[Eric Carlson]

Created attachment 357234 [details]
Follow up fix

[WebKit Commit Bot]

Comment on attachment 357234 [details]
Follow up fix

Clearing flags on attachment: 357234

Committed r239169: <https://trac.webkit.org/changeset/239169>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.