Bug 189000 – Mock video devices should only support discrete sizes

• Component: WebRTC
• Status: RESOLVED FIXED

[Eric Carlson]

Make the mock video devices more like real capture devices by only supporting discrete width/height pairs.

[Radar WebKit Bug Importer]

<rdar://problem/43766551>

[Eric Carlson]

Created attachment 348195 [details]
Patch

[youenn fablet]

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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 348418 [details]
Patch

[Eric Carlson]

Created attachment 348420 [details]
Patch

[youenn fablet]

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.

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 348462 [details]
Patch

[youenn fablet]

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.

[Eric Carlson]

Created attachment 348517 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 348517 [details]
Patch for landing

Clearing flags on attachment: 348517

Committed r235513: <https://trac.webkit.org/changeset/235513>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.