[webkit-reviews] review granted: [Bug 230841] Add an experimental VideoTrackConfiguration class and accessor on VideoTrack : [Attachment 444746] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 230841: Add an experimental VideoTrackConfiguration class and accessor on
VideoTrack
https://bugs.webkit.org/show_bug.cgi?id=230841

Attachment 444746: Patch

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




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

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

> Source/WebCore/Modules/webcodecs/VideoColorSpace.h:64
> +    VideoColorSpaceInit m_init { };

Nit: 'm_init' is a strange name since it is mutated by the setters. Maybe
'm_state'?

> Source/WebCore/html/track/AudioTrackConfiguration.h:66
> +    AudioTrackConfigurationInit m_init;

Ditto

> Source/WebCore/platform/graphics/VideoTrackPrivate.h:75
> +    virtual void setColorSpace(PlatformVideoColorSpace colorSpace) {
m_colorSpace = colorSpace; }

setColorSpace(PlatformVideoColorSpace&& colorSpace) { m_colorSpace =
WTFMove(colorSpace); }

>
Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:260
> +    if (!assetTrack || !assetTrack.formatDescriptions.count)
> +	   return nullptr;

Why not assert if there is no assetTrack like in the other methods?

>
Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.mm:299
> +    auto assetTrack = assetTrackFor(*this);
> +    if (!assetTrack) {
> +	   ASSERT_NOT_REACHED();
> +	   return 0;
> +    }
> +    return assetTrack.nominalFrameRate;

Nit: This style should match the other methods:

if (auto assetTrack = assetTrackFor(*this))
    return assetTrack.nominalFrameRate;
ASSERT_NOT_REACHED();
return 0;

> Source/WebKit/GPUProcess/media/RemoteVideoTrackProxy.cpp:70
> +	   .colorSpace = m_trackPrivate->colorSpace(),

can you WTFMove() this?

> Source/WebKit/GPUProcess/media/TrackPrivateRemoteConfiguration.h:178
> +	       *codec,

WTFMove

> Source/WebKit/GPUProcess/media/TrackPrivateRemoteConfiguration.h:181
> +	       *colorSpace,

Ditto