Make the mock video devices more like real capture devices by only supporting discrete width/height pairs.
<rdar://problem/43766551>
Created attachment 348195 [details] Patch
Comment on attachment 348195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348195&action=review > Source/WebCore/ChangeLog:10 > + paris, our mock video capture devices supported arbitrary width and height combinations which pairs > Source/WebCore/platform/mediastream/RealtimeMediaSourceSettings.h:33 > +#include <wtf/Optional.h> Do we need this include? > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:16 > + * software without specific prior written permission. I think we now use two clause license. > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:66 > +void RealtimeVideoSource::setSupportedFrameRates(const Vector<double>& rates) Should be a Vector<>&& > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:75 > + capabilities.setFrameRate({ 0, 0 }); This one is not clear to me. Is there a case where this can actually happen. Should we just say that we are not supporting frame rate? > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:79 > + if (!m_supportedCaptureSizes.size()) { Ditto here, it seems to me that there should be one supported frame rate, width and height. > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:109 > +bool RealtimeVideoSource::supportsSizeAndFrameRate(std::optional<int> width, std::optional<int> height, std::optional<double> frameRate) I know this is preexisting code but It is weird to me that we use ints there. unsigned would make sense. The spec is using long which does not make a lot of sense to me. > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:142 > +bool RealtimeVideoSource::applySize(const IntSize& size) applySize is a bool while applySizeAndFrameRate returns a void. I wonder whether we can find better names for applySize, something like canApplySize maybe? > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:180 > + m_observedFrameRate = (m_observedFrameTimeStamps.size() / interval); This observed frame rate is useful for mocks. Will it be useful for real devices as well, are we planning to expose this value? > Source/WebCore/platform/mediastream/RealtimeVideoSource.h:46 > + Line unneeded > Source/WebCore/platform/mediastream/RealtimeVideoSource.h:57 > + bool applyFrameRate(double) override; I wonder whether we should not try to make code being shared in a different manner than override the parent implementation but keep calling the parent implementation in the override method. Maybe videoSampleAvailable should be renamed to dispatchMediaSampleToObservers for instance? > Source/WebCore/platform/mediastream/RealtimeVideoSource.h:60 > + void setSupportedCaptureSizes(const Vector<IntSize>& sizes) { m_supportedCaptureSizes = sizes; } Vector<>&& > Source/WebCore/platform/mock/MockMediaDevice.h:94 > double defaultFrameRate { 30 }; Is defaultFrameRate still in use? > Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:136 > + Unneeded line. > Source/WebCore/platform/mock/MockRealtimeVideoSource.h:47 > +class MockRealtimeVideoSource : public MockRealtimeMediaSource, public RealtimeVideoSource { This looks a bit weird to derive from two realtime source classes. Maybe MockRealtimeMediaSource should be renamed?
Comment on attachment 348195 [details] Patch Attachment 348195 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9000882 New failing tests: webrtc/video-interruption.html webrtc/video.html imported/w3c/web-platform-tests/mediacapture-streams/MediaStreamTrack-getSettings.https.html imported/w3c/web-platform-tests/mediacapture-streams/GUM-trivial-constraint.https.html
Created attachment 348232 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 348195 [details] Patch Attachment 348195 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9005325 New failing tests: webrtc/video-interruption.html webrtc/video.html imported/w3c/web-platform-tests/mediacapture-streams/MediaStreamTrack-getSettings.https.html imported/w3c/web-platform-tests/mediacapture-streams/GUM-trivial-constraint.https.html
Created attachment 348260 [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 348195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348195&action=review >> Source/WebCore/ChangeLog:10 >> + paris, our mock video capture devices supported arbitrary width and height combinations which > > pairs Fixed. >> Source/WebCore/platform/mediastream/RealtimeMediaSourceSettings.h:33 >> +#include <wtf/Optional.h> > > Do we need this include? No, removed. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:16 >> + * software without specific prior written permission. > > I think we now use two clause license. Oops, fixed. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:66 >> +void RealtimeVideoSource::setSupportedFrameRates(const Vector<double>& rates) > > Should be a Vector<>&& Fixed. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:75 >> + capabilities.setFrameRate({ 0, 0 }); > > This one is not clear to me. Is there a case where this can actually happen. > Should we just say that we are not supporting frame rate? Seems unlikely, but added a single default. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:79 >> + if (!m_supportedCaptureSizes.size()) { > > Ditto here, it seems to me that there should be one supported frame rate, width and height. Ditto. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:109 >> +bool RealtimeVideoSource::supportsSizeAndFrameRate(std::optional<int> width, std::optional<int> height, std::optional<double> frameRate) > > I know this is preexisting code but It is weird to me that we use ints there. unsigned would make sense. > The spec is using long which does not make a lot of sense to me. I will fix this in a follow up. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:142 >> +bool RealtimeVideoSource::applySize(const IntSize& size) > > applySize is a bool while applySizeAndFrameRate returns a void. > I wonder whether we can find better names for applySize, something like canApplySize maybe? I agree, we should split out the "can you apply this size" portion into a new method. That will a big change by itself, so I will do that in a follow-up. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:180 >> + m_observedFrameRate = (m_observedFrameTimeStamps.size() / interval); > > This observed frame rate is useful for mocks. > Will it be useful for real devices as well, are we planning to expose this value? I think it will be useful for diagnostics in other video types. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.h:46 >> + > > Line unneeded Removed. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.h:57 >> + bool applyFrameRate(double) override; > > I wonder whether we should not try to make code being shared in a different manner than override the parent implementation but keep calling the parent implementation in the override method. > Maybe videoSampleAvailable should be renamed to dispatchMediaSampleToObservers for instance? OK >> Source/WebCore/platform/mediastream/RealtimeVideoSource.h:60 >> + void setSupportedCaptureSizes(const Vector<IntSize>& sizes) { m_supportedCaptureSizes = sizes; } > > Vector<>&& Fixed. >> Source/WebCore/platform/mock/MockMediaDevice.h:94 >> double defaultFrameRate { 30 }; > > Is defaultFrameRate still in use? Yes. >> Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:136 >> + > > Unneeded line. Removed. >> Source/WebCore/platform/mock/MockRealtimeVideoSource.h:47 >> +class MockRealtimeVideoSource : public MockRealtimeMediaSource, public RealtimeVideoSource { > > This looks a bit weird to derive from two realtime source classes. Maybe MockRealtimeMediaSource should be renamed? I agree. MockRealtimeMediaSource did almost nothing as a base class, so I removed it and moved the device management stuff to MockRealtimeMediaSourceCenter.
Created attachment 348418 [details] Patch
Created attachment 348420 [details] Patch
Comment on attachment 348420 [details] Patch Looks almost good to go. I still have difficulties with having both applySize and applySizeAndFrameRate, each one being able to do the actual work or not, one returning a boolean and not the other. Maybe we could try to simplify things in this patch, or maybe in another patch? View in context: https://bugs.webkit.org/attachment.cgi?id=348420&action=review > Source/WebCore/platform/mediastream/MediaConstraints.h:258 > + value = min; Do we have test coverage here and below? > Source/WebCore/platform/mediastream/RealtimeMediaSource.h:251 > + virtual void videoSampleAvailable(MediaSample&); Does not seem needed anymore to be virtual? > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:2 > + * Copyright (C) 2013-2015 Apple Inc. All rights reserved. 2018 > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:49 > +void RealtimeVideoSource::startProducingData() Do we need this method here? It does not seem to be used by Mock Video Source, although maybe it should? > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:61 > +void RealtimeVideoSource::setSupportedFrameRates(const Vector<double>&& rates) s/const// > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:63 > + m_supportedFrameRates = rates; = WTFMove(rates) > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:88 > + for (auto size : m_supportedCaptureSizes) { const auto&? > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:111 > + return false; Could be written as "if (m_supportedCaptureSizes.isEmpty())" But do we need this check? > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:114 > + if (supportedSize.isEmpty()) one-liner maybe? > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:137 > +bool RealtimeVideoSource::applySize(const IntSize& size) s/applySize/supportSize/? > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:149 > +{ Maybe it would be clearer to replace ASSERT(!supportedSize.isEmpty()); by ASSERT(supportSize(width, heigh)); > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:175 > + m_observedFrameRate = (m_observedFrameTimeStamps.size() / interval); I am not sure whether this will be useful in every case and I wonder whether it might cost some unnecessary CPU cycles. In regular capture, we would probably only use this when activating release logging. For mock video sources, we actually want to use it. Maybe we should make this processing optional by moving this computation to a separate method, that either mock or release logging would call? > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:182 > + return supportsFrameRate(rate); This implementation seems a bit weird here. If we rename applySize to supportSize maybe applyFrameRate here should be supportFrameRate? > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:187 > + double epsilon = 0.001; We have different places where we do this epsilon comparison, maybe it would be good to refactor this, not in this patch probably though. > Source/WebCore/platform/mediastream/RealtimeVideoSource.h:2 > + * Copyright (C) 2013-2015 Apple Inc. All rights reserved. 2018 > Source/WebCore/platform/mediastream/RealtimeVideoSource.h:50 > + bool applyFrameRate(double) override; Some of these can be made final, right? > Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:163 > + return true; I struggle with applySize and supportSize a bit... As I see the model, currently applySizeAndFrameRate is the one doing the actual change, applySize becomes somehow supportSize since we do want to change all three parameters together. If so, MockRealtimeVideoSourceMac::applySize should not be overridden and m_bufferPool should be set to nullptr in applySizeAndFrameRate. > Source/WebCore/platform/mock/MockMediaDevice.h:91 > + return MockCameraProperties { *defaultFrameRate, *facingMode, *frameRates, *frameSizes, Color::black }; WTFMove(*frameRates) and WTFMove(frameSizes). > Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:113 > + m_device = *device; To remove the ASSERT, maybe the constructor should take a MockDevice&& as parameter. In MockRealtimeVideoSource::create, if there is no device, we would then return an error. > Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:141 > + capabilities.addFacingMode(WTF::get<MockCameraProperties>(m_device.properties).facingMode); Shouldn't we set width, height, frame rate capabilities like done for screen? It seems addSupportedCapabilities will do that for us by using some defaults. > Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:146 > + capabilities.setFrameRate(CapabilityValueOrRange(.01, 60.0)); It would be nice to not have these constants in this code. Maybe MockMediaDevice could store these somehow and allows unifying code for mock camera and mock screen? > Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:196 > + startCaptureTimer(); m_haveProducedData will remain true while the source started capturing even after stopping, right? It is unclear to me why we need this mechanism and why we need both startCaptureTimer() and generateFrame() here. > Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:203 > + m_emitFrameTimer.startRepeating(1_ms * lround(1000 / frameRate())); 1_s / frameRate() is not working? > Source/WebCore/platform/mock/MockRealtimeVideoSource.h:47 > +class MockRealtimeVideoSource : public RealtimeVideoSource { final? > LayoutTests/webrtc/video-interruption.html:48 > + return navigator.mediaDevices.getUserMedia({video: {advanced: [{width:{min:640}}, {height:{min:480} } ]}}).then((stream) => { video: true is good enough here, I believe.
Comment on attachment 348420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348420&action=review >> Source/WebCore/platform/mediastream/MediaConstraints.h:258 >> + value = min; > > Do we have test coverage here and below? Yes, I found these because of failures in existing tests. >> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:251 >> + virtual void videoSampleAvailable(MediaSample&); > > Does not seem needed anymore to be virtual? True, fixed. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:49 >> +void RealtimeVideoSource::startProducingData() > > Do we need this method here? > It does not seem to be used by Mock Video Source, although maybe it should? We need it here or this code will be duplicated in all derived classes. It is implemented by the mock - see MockRealtimeVideoSource::startProducingData below. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:61 >> +void RealtimeVideoSource::setSupportedFrameRates(const Vector<double>&& rates) > > s/const// Oops, fixed. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:63 >> + m_supportedFrameRates = rates; > > = WTFMove(rates) Ditto. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:88 >> + for (auto size : m_supportedCaptureSizes) { > > const auto&? Good point, fixed. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:111 >> + return false; > > Could be written as "if (m_supportedCaptureSizes.isEmpty())" > But do we need this check? Sure. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:114 >> + if (supportedSize.isEmpty()) > > one-liner maybe? Sure. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:137 >> +bool RealtimeVideoSource::applySize(const IntSize& size) > > s/applySize/supportSize/? You mentioned this in the previous review and I said: I agree, we should split out the "can you apply this size" portion into a new method. That will a big change by itself, so I will do that in a follow-up. Changing now will require changes in 16 files, so lets do it in a followup. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:149 >> +{ > > Maybe it would be clearer to replace ASSERT(!supportedSize.isEmpty()); by ASSERT(supportSize(width, heigh)); I suppose it is somewhat clearer but it also requires us to walk through the entire vector twice, even though it should never assert. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:182 >> + return supportsFrameRate(rate); > > This implementation seems a bit weird here. > If we rename applySize to supportSize maybe applyFrameRate here should be supportFrameRate? See my comment above. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:187 >> + double epsilon = 0.001; > > We have different places where we do this epsilon comparison, maybe it would be good to refactor this, not in this patch probably though. We do it here and in AVVideoCaptureSource. The later will go away when it uses RealtimeVideoSource as its base class. >> Source/WebCore/platform/mediastream/RealtimeVideoSource.h:2 >> + * Copyright (C) 2013-2015 Apple Inc. All rights reserved. > > 2018 fixed >> Source/WebCore/platform/mediastream/RealtimeVideoSource.h:50 >> + bool applyFrameRate(double) override; > > Some of these can be made final, right? Some can, fixed. >> Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:163 >> + return true; > > I struggle with applySize and supportSize a bit... > As I see the model, currently applySizeAndFrameRate is the one doing the actual change, applySize becomes somehow supportSize since we do want to change all three parameters together. > If so, MockRealtimeVideoSourceMac::applySize should not be overridden and m_bufferPool should be set to nullptr in applySizeAndFrameRate. It is called from RealtimeVideoSource::applySizeAndFrameRate and from RealtimeVideoSource::startProducingData to set the default size. >> Source/WebCore/platform/mock/MockMediaDevice.h:91 >> + return MockCameraProperties { *defaultFrameRate, *facingMode, *frameRates, *frameSizes, Color::black }; > > WTFMove(*frameRates) and WTFMove(frameSizes). Fixed. >> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:113 >> + m_device = *device; > > To remove the ASSERT, maybe the constructor should take a MockDevice&& as parameter. > In MockRealtimeVideoSource::create, if there is no device, we would then return an error. The ASSERT is important because it signals a build error. Changing the constructor to take a mock device requires a change to all four derived constructors and create methods, so let's leave it here. >> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:141 >> + capabilities.addFacingMode(WTF::get<MockCameraProperties>(m_device.properties).facingMode); > > Shouldn't we set width, height, frame rate capabilities like done for screen? > It seems addSupportedCapabilities will do that for us by using some defaults. addSupportedCapabilities sets width, height, and frame rate capabilities based on m_supportedCaptureSizes and m_supportedFrameRates. >> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:146 >> + capabilities.setFrameRate(CapabilityValueOrRange(.01, 60.0)); > > It would be nice to not have these constants in this code. > Maybe MockMediaDevice could store these somehow and allows unifying code for mock camera and mock screen? I guess they could be in MockMediaDevice, but they will still be hard coded constants. Not sure it makes much difference. >> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:196 >> + startCaptureTimer(); > > m_haveProducedData will remain true while the source started capturing even after stopping, right? > It is unclear to me why we need this mechanism and why we need both startCaptureTimer() and generateFrame() here. It shouldn't start the timer. The call to generateFrame is to force an update if the stream isn't running, but it probably isn't necessary. Removed. >> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:203 >> + m_emitFrameTimer.startRepeating(1_ms * lround(1000 / frameRate())); > > 1_s / frameRate() is not working? Sure thing. >> Source/WebCore/platform/mock/MockRealtimeVideoSource.h:47 >> +class MockRealtimeVideoSource : public RealtimeVideoSource { > > final? Won't work, MockRealtimeVideoSourceMac derives from it. >> LayoutTests/webrtc/video-interruption.html:48 >> + return navigator.mediaDevices.getUserMedia({video: {advanced: [{width:{min:640}}, {height:{min:480} } ]}}).then((stream) => { > > video: true is good enough here, I believe. You added min width and height constraints when you wrote it so I just corrected them so the test would work. I don't think they hurts, so let's leave them.
Created attachment 348462 [details] Patch
Comment on attachment 348462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348462&action=review > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:49 > +void RealtimeVideoSource::startProducingData() Maybe we should rename it to something like prepareToProduceData? Subclasses would call that helper method from startProducingData.
Created attachment 348517 [details] Patch for landing
Comment on attachment 348517 [details] Patch for landing Clearing flags on attachment: 348517 Committed r235513: <https://trac.webkit.org/changeset/235513>
All reviewed patches have been landed. Closing bug.