Bug 123378 – [MSE][Mac] Add a new MSE-compatible MediaPlayerPrivate implementation, MediaPlayerPrivateMediaSourceAVFObjC

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[MSE][Mac] Add a new MSE-compatible MediaPlayerPrivate implementation, MediaPlayerPrivateMediaSourceAVFObjC

[Jer Noble]

Created attachment 217110 [details]
Patch

[Build Bot]

Comment on attachment 217110 [details]
Patch

Attachment 217110 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/24738002

[Build Bot]

Comment on attachment 217110 [details]
Patch

Attachment 217110 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/23628262

[Build Bot]

Comment on attachment 217110 [details]
Patch

Attachment 217110 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/24728032

[Jer Noble]

The compilation errors are due to the dependency on bug #124422.

[Jer Noble]

Created attachment 218225 [details]
Patch

[Build Bot]

Comment on attachment 218225 [details]
Patch

Attachment 218225 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/42178065

[Build Bot]

Comment on attachment 218225 [details]
Patch

Attachment 218225 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/39688042

[Jer Noble]

Created attachment 218240 [details]
Patch

Fix release build.

[Eric Carlson]

Comment on attachment 218240 [details]
Patch

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

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

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.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/MediaPlayerPrivateMediaSourceAVFObjC.mm:149
> +

Nit: unneeded blank line.

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

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.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/MediaPlayerPrivateMediaSourceAVFObjC.mm:309
> +    // No-op

Nit: missing period.

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

Nit: bug number?

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

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.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/MediaPlayerPrivateMediaSourceAVFObjC.mm:349
> +    // No-op

Nit: missing period.

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

Nit: bug number?

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

???

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.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/MediaPlayerPrivateMediaSourceAVFObjC.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.mm:246
> +class MediaDescriptionAVFObjC : public MediaDescription {

FINAL?

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

Wow, FourCharCode!

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm: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.mm: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.mm: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.mm:307
> +    m_asset = asset;

Nit: why retain the asset?

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

Nit: bug number?

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

Nit: C-style cast?

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

Nit: bug number?

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

Ditto.

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

Ditto.

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

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm: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.mm:505
> +        PlatformSample platformSample = mediaSample->platformSample();
> +        if (platformSample.type != PlatformSample::CMSampleBufferType)
> +            return;

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

[EFL EWS Bot]

Comment on attachment 218240 [details]
Patch

Attachment 218240 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/42828083

[Jer Noble]

Comment on attachment 218240 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:28
>> +
> 
> #if ENABLE(MEDIA_SOURCE) && USE(AVFOUNDATION)

Added.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:137
>> +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)?

Sure, but I'll do that in a separate bug. <http://webkit.org/b/125154>

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:149
>> +
> 
> Nit: unneeded blank line.

Deleted.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:155
>> +
> 
> Ditto.

Deleted.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:208
>> +}
> 
> Is this correct - shouldn't it return the intrinsic size of the track(s)?

It probably should.  I'll hook this up in a future bug. <http://webkit.org/b/125156>

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:314
>> +    // FIXME: paint()
> 
> Nit: bug number?

Added. <http://webkit.org/b/125157>

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

Ditto.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:335
>> +    if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player))
> 
> Nit: is there really any reason to call supportsAcceleratedRendering()?

Not really. :)

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:349
>> +    // No-op
> 
> Nit: missing period.

Added.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:360
>> +    // FIXME: implement languageOfPrimaryAudioTrack()
> 
> Nit: bug number?

Added. <http://webkit.org/b/125158>

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:401
>> +    UNUSED_PARAM(oldReadyState);
> 
> ???

This is crazy, and just dead code; I'll remove it.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:429
>> +    m_player->mediaPlayerClient()->mediaPlayerRenderingModeChanged(m_player);
> 
> Is it legal to pass a NULL displayLayer? If not, it is probably worth adding an ASSERT.

Added.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:432
>> +    m_player->firstVideoFrameAvailable();
> 
> "appropriate:" ?

Yeah, just because a layer was added, that doesn't mean a decoded video frame is actually available.  Unfortunately, we don't have any insight into when AVSampleBufferDisplayLayer actually has decoded a frame of video.  Hence, "Move this somewhere (more) appropriate."

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.h:49
>> +class MediaSourcePrivateAVFObjC : public MediaSourcePrivate {
> 
> Can this be marked FINAL?

Sure.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.h:51
>> +    static RefPtr<MediaSourcePrivateAVFObjC> create(MediaPlayerPrivateMediaSourceAVFObjC*);
> 
> Can this return a std::unique_ptr<>?

No, this is a refcounted class.

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

Added. <http://webkit.org/b/125159>

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

Ditto.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h:57
>> +class SourceBufferPrivateAVFObjC : public SourceBufferPrivate {
> 
> Can this be marked FINAL?

Sure.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h:107
>> +    Vector<RefPtr<AudioTrackPrivate>> m_audioTracks;
> 
> Nit: ordering these by size may make the object slightly smaller.

Ordered.

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

Sure.

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

I no rite!?

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:266
>> +            m_codec = AtomicString(reinterpret_cast<LChar*>(&codec), 4);
> 
> What will happen on a Big Endian machine ;-O

It'll get the 4CC backward. But it will at least be /consistently/ wrong. :)

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:301
>> +}
> 
> Is there any chance the parser delegate will be called (eg. on another queue/thread) after this? IOW, should WebAVStreamDataParserListener an explicit "disconnect"?

No, the parser is not threaded.  It's delegate methods will explicitly only be called from within appendStreamData:.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:306
>> +
> 
> Nit: you can skip a lot of unnecessary work by returning immediately if m_client is NULL.

Good point.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:307
>> +    m_asset = asset;
> 
> Nit: why retain the asset?

I'll use it later. :)

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:325
>> +        // FIXME: Add TextTrack support
> 
> Nit: bug number?

Added. <http://webkit.org/b/125161>

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:350
>> +    ProcessCodedFrameInfo* info = (ProcessCodedFrameInfo*)refcon;
> 
> Nit: C-style cast?

I'll turn this into a static_cast.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:374
>> +    notImplemented();
> 
> Nit: bug number?

There may be nothing to do here. The "End of stream" algorithm is only for decode errors, not for when the end of a stream (or track) is reached.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm: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.

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:505
>> +            return;
> 
> Can the vector have samples of different types? IOW, should this be "continue"?

No, in fact this may even be an ASSERT situation.

[Jer Noble]

(In reply to comment #11)
> (From update of attachment 218240 [details])
> Attachment 218240 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/42828083

The EFL error:

Last 500 characters of output:
Core/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/JSHTMLElementWrapperFactory.cpp.o
c++: internal compiler error: Killed (program cc1plus)

I don't think this build error has anything to do with this patch.

[Jer Noble]

Committed r160251: <http://trac.webkit.org/changeset/160251>