[webkit-reviews] review granted: [Bug 198840] Changing settings of a MediaStreamTrack clone should not alter the settings of the original track : [Attachment 372479] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 198840: Changing settings of a MediaStreamTrack clone should not alter the
settings of the original track
https://bugs.webkit.org/show_bug.cgi?id=198840

Attachment 372479: Patch

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




--- Comment #10 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 372479
  --> https://bugs.webkit.org/attachment.cgi?id=372479
Patch

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

> Source/WebCore/platform/mediastream/RealtimeVideoCaptureSource.cpp:185
> +	   for (auto& size : standardVideoSizes()) {
> +	       if (size.width() < minimumWidth || size.height() <
minimumHeight)
> +		   minimumAspectRatio = std::min(minimumAspectRatio,
static_cast<double>(size.width()) / size.height());
> +	   }

This isn't new, but I don't think this look is necessary because minimumWidth
and  minimumHeight are both 1.

> Source/WebCore/platform/mediastream/RealtimeVideoCaptureSource.cpp:205
> +    const double epsilon = 0.001;
> +    return frameRate + epsilon >= range.minimum && frameRate - epsilon <=
range.maximum;

Also not new, but I wonder if we could use WTF::areEssentiallyEqual here?

> Source/WebCore/platform/mediastream/RealtimeVideoCaptureSource.cpp:444
> +    if (!match)
> +	   return;

Logging an error here might be helpful.

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:73
> +    if (!width)
> +	   width = sourceSize.width() * height.value() / sourceSize.height();
> +    m_currentSettings.setWidth(*width);

Maybe we should ASSERT(sourceSize.width() && sourceSize.height()) since we will
get a divide-by-zero crash if either is zero.