[webkit-reviews] review granted: [Bug 123378] [MSE][Mac] Add a new MSE-compatible MediaPlayerPrivate implementation, MediaPlayerPrivateMediaSourceAVFObjC : [Attachment 218240] 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 123378: [MSE][Mac] Add a new MSE-compatible MediaPlayerPrivate
implementation, MediaPlayerPrivateMediaSourceAVFObjC
https://bugs.webkit.org/show_bug.cgi?id=123378

Attachment 218240: Patch
https://bugs.webkit.org/attachment.cgi?id=218240&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218240&action=review


>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:28
> +

#if ENABLE(MEDIA_SOURCE) && USE(AVFOUNDATION)

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:137
> +void MediaPlayerPrivateMediaSourceAVFObjC::load(const String&)
> +{
> +    ASSERT_NOT_REACHED();
> +}
> +
> +void MediaPlayerPrivateMediaSourceAVFObjC::load(const String& url,
PassRefPtr<HTMLMediaSource> source)

Nit: can you change load() take a class/union rather than adding another
version (we will need another variant for MediaStream soon enough)?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:149
> +

Nit: unneeded blank line.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:155
> +

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:208
> +IntSize MediaPlayerPrivateMediaSourceAVFObjC::naturalSize() const
> +{
> +    return IntSize();
> +}

Is this correct - shouldn't it return the intrinsic size of the track(s)?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:309
> +    // No-op

Nit: missing period.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:314
> +    // FIXME: paint()

Nit: bug number?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:319
> +    // FIXME: implement paintCurrentFrameInContext()

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:335
> +    if (supportsAcceleratedRendering() &&
m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player))

Nit: is there really any reason to call supportsAcceleratedRendering()?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:349
> +    // No-op

Nit: missing period.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:360
> +    // FIXME: implement languageOfPrimaryAudioTrack()

Nit: bug number?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:401
> +    MediaPlayer::NetworkState oldNetworkState = m_networkState;
> +    MediaPlayer::ReadyState oldReadyState = m_readyState;
> +
> +    UNUSED_PARAM(oldNetworkState);
> +    UNUSED_PARAM(oldReadyState);

???

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:429
> +    m_sampleBufferDisplayLayer = displayLayer;
> +    [m_sampleBufferDisplayLayer setControlTimebase:m_clock->timebase()];
> +   
m_player->mediaPlayerClient()->mediaPlayerRenderingModeChanged(m_player);

Is it legal to pass a NULL displayLayer? If not, it is probably worth adding an
ASSERT.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:432
> +    // FIXME: move this somewhere appropriate:
> +    m_player->firstVideoFrameAvailable();

"appropriate:" ?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.h:
49
> +class MediaSourcePrivateAVFObjC : public MediaSourcePrivate {

Can this be marked FINAL?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.h:
51
> +    static RefPtr<MediaSourcePrivateAVFObjC>
create(MediaPlayerPrivateMediaSourceAVFObjC*);

Can this return a std::unique_ptr<>?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.mm
:104
> +    // FIXME: implement markEndOfStream()

Nit: bug number?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.mm
:110
> +    // FIXME: implement unmarkEndOfStream()

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h
:57
> +class SourceBufferPrivateAVFObjC : public SourceBufferPrivate {

Can this be marked FINAL?

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h
:107
> +    bool m_parsingSucceeded;
> +    int m_enabledVideoTrackID;
> +
> +    Vector<RefPtr<VideoTrackPrivate>> m_videoTracks;
> +    Vector<RefPtr<AudioTrackPrivate>> m_audioTracks;

Nit: ordering these by size may make the object slightly smaller.

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:246
> +class MediaDescriptionAVFObjC : public MediaDescription {

FINAL?

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:265
> +	       FourCharCode codec =
CMFormatDescriptionGetMediaSubType(description);

Wow, FourCharCode!

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:266
> +	       m_codec = AtomicString(reinterpret_cast<LChar*>(&codec), 4);

What will happen on a Big Endian machine ;-O

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:301
> +SourceBufferPrivateAVFObjC::~SourceBufferPrivateAVFObjC()
> +{
> +    if (m_displayLayer) {
> +	   m_parent->player()->removeDisplayLayer(m_displayLayer.get());
> +	   [m_displayLayer flushAndRemoveImage];
> +	   [m_displayLayer stopRequestingMediaData];
> +	   m_displayLayer = nullptr;
> +    }
> +}

Is there any chance the parser delegate will be called (eg. on another
queue/thread) after this? IOW, should WebAVStreamDataParserListener an explicit
"disconnect"?

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:306
> +

Nit: you can skip a lot of unnecessary work by returning immediately if
m_client is NULL.

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:307
> +    m_asset = asset;

Nit: why retain the asset?

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:325
> +	   // FIXME: Add TextTrack support

Nit: bug number?

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:350
> +    ProcessCodedFrameInfo* info = (ProcessCodedFrameInfo*)refcon;

Nit: C-style cast?

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:374
> +    notImplemented();

Nit: bug number?

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:397
> +    notImplemented();

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:424
> +    notImplemented();

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:429
> +    notImplemented();

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:483
> +    for (CFIndex i = 0; i < CFArrayGetCount(attachmentsArray); ++i) {

Nit: the attachments array won't change size, so you can set a local variable
in the init-expression.

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:505
> +	   PlatformSample platformSample = mediaSample->platformSample();
> +	   if (platformSample.type != PlatformSample::CMSampleBufferType)
> +	       return;

Can the vector have samples of different types? IOW, should this be "continue"?