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.
<rdar://problem/46665734>
Created attachment 357150 [details] Patch
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
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
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
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
Created attachment 357174 [details] Patch
Comment on attachment 357174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357174&action=review > Source/WebKit/ChangeLog:17 > +2018-12-12 Eric Carlson <eric.carlson@apple.com> 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
Comment on attachment 357174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357174&action=review >> Source/WebKit/ChangeLog:17 >> +2018-12-12 Eric Carlson <eric.carlson@apple.com> > > 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.
Created attachment 357228 [details] Patch for landing
Comment on attachment 357228 [details] Patch for landing Clearing flags on attachment: 357228 Committed r239163: <https://trac.webkit.org/changeset/239163>
All reviewed patches have been landed. Closing bug.
Reopening to attach new patch.
Created attachment 357234 [details] Follow up fix
Comment on attachment 357234 [details] Follow up fix Clearing flags on attachment: 357234 Committed r239169: <https://trac.webkit.org/changeset/239169>