[webkit-reviews] review canceled: [Bug 189000] Mock video devices should only support discrete sizes : [Attachment 348195] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has canceled Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 189000: Mock video devices should only support discrete sizes
https://bugs.webkit.org/show_bug.cgi?id=189000

Attachment 348195: Patch

https://bugs.webkit.org/attachment.cgi?id=348195&action=review




--- Comment #8 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 348195
  --> https://bugs.webkit.org/attachment.cgi?id=348195
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.