Bug 146908 – Implement VideoTrackPrivateAVFObjC for MediaStreamPrivate

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[Matthew Daiter]

Make sure we can implement platform-specific portions of the video streaming API for the browser media streams.

[Radar WebKit Bug Importer]

<rdar://problem/21796914>

[Matthew Daiter]

Created attachment 256717 [details]
Patch

[Brent Fulgham]

Comment on attachment 256717 [details]
Patch

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

Looks good, but I don't like the potential null access. Let's make it a reference if we know it's always present; otherwise null check.

> Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateMediaStreamAVFObjC.mm:80
> +    m_parent->activeStatusChanged();

Can m_parent be null? If it shouldn't be, we should take it as a reference in the constructor. Otherwise, we need to null-check here.

[Eric Carlson]

Comment on attachment 256717 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateMediaStreamAVFObjC.mm:66
> +void VideoTrackPrivateMediaStreamAVFObjC::setAssetTrack(AVAssetTrack *track)
> +{
> +    m_impl = std::make_unique<AVTrackPrivateAVFObjCImpl>(track);
> +    resetPropertiesFromTrack();
> +}
> +
> +AVAssetTrack* VideoTrackPrivateMediaStreamAVFObjC::assetTrack() const
> +{
> +    return m_impl->assetTrack();
> +}

Is it possible to have an AVAssetTrack now, or is this for the future? If the later, remove it for now and we can add it when it is needed.

> Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateMediaStreamAVFObjC.mm:86
> +FloatSize VideoTrackPrivateMediaStreamAVFObjC::naturalSize() const
> +{
> +    return FloatSize([assetTrack() naturalSize]);
> +}

What about the nature size of a media stream track that does not have an AVAssetTrack?