Bug 125905 – [MSE][Mac] Add AVSampleBufferAudioRenderer support.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[MSE][Mac] Add AVSampleBufferAudioRenderer support.

[Jer Noble]

Created attachment 219489 [details]
Patch

Initial version. The ChangeLog is a little sparse, and I intend to flesh it out.

[Jer Noble]

Created attachment 219534 [details]
Patch

[Eric Carlson]

Comment on attachment 219534 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:326
> +        [renderer flush];
> +        [renderer stopRequestingMediaData];

Shouldn't these be called in the opposite order to avoid a potential race?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:380
> +    return info->sourceBuffer->processCodedFrame(info->trackID, sampleBuffer, info->mediaType) ? noErr : paramErr;

paramErr :-O

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:447
> +    for (auto it = m_audioRenderers.begin(), end = m_audioRenderers.end(); it != end; ++it) {
> +        AVSampleBufferAudioRenderer* renderer = it->second.get();
> +        if (m_mediaSource)
> +            m_mediaSource->player()->removeAudioRenderer(renderer);
> +        [renderer flush];
> +        [renderer stopRequestingMediaData];
> +    }
> +
> +    m_audioRenderers.clear();

This looks like the same code you have in the destructor. Can it be in a shared method?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:510
> +        if (m_mediaSource)
> +            m_mediaSource->player()->addDisplayLayer(m_displayLayer.get());

player()->addDisplayLayer() is a noop if it the layer has already been added?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:558
> +    int trackID = trackIDString.toInt();

Might be worth adding an ASSERT(trackID < m_audioRenderers.size())

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:604
> +    int trackID = trackIDString.toInt();

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:624
> +    int trackID = trackIDString.toInt();

Ditto.

[Jer Noble]

(In reply to comment #3)
> (From update of attachment 219534 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219534&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:326
> > +        [renderer flush];
> > +        [renderer stopRequestingMediaData];
> 
> Shouldn't these be called in the opposite order to avoid a potential race?

Since the renderers are requesting media data on the main dispatch queue, there's no opportunity for a race.  But I'll swap them anyways.

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:380
> > +    return info->sourceBuffer->processCodedFrame(info->trackID, sampleBuffer, info->mediaType) ? noErr : paramErr;
> 
> paramErr :-O

I no rite?!  Luckily, this gets removed in a future patch.

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:447
> > +    for (auto it = m_audioRenderers.begin(), end = m_audioRenderers.end(); it != end; ++it) {
> > +        AVSampleBufferAudioRenderer* renderer = it->second.get();
> > +        if (m_mediaSource)
> > +            m_mediaSource->player()->removeAudioRenderer(renderer);
> > +        [renderer flush];
> > +        [renderer stopRequestingMediaData];
> > +    }
> > +
> > +    m_audioRenderers.clear();
> 
> This looks like the same code you have in the destructor. Can it be in a shared method?

I think so.

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:510
> > +        if (m_mediaSource)
> > +            m_mediaSource->player()->addDisplayLayer(m_displayLayer.get());
> 
> player()->addDisplayLayer() is a noop if it the layer has already been added?

No, it swaps out the existing layer for the new one.  The existing one might have been created by ensurePlatformLayer().

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:558
> > +    int trackID = trackIDString.toInt();
> 
> Might be worth adding an ASSERT(trackID < m_audioRenderers.size())

The "if (m_audioRenderers.count(trackID))" check should handle that.  Besides, what if a video has 2 video tracks, then 1 audio track?

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:604
> > +    int trackID = trackIDString.toInt();
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:624
> > +    int trackID = trackIDString.toInt();
> 
> Ditto.

Both these have ASSERT_NOT_REACHED checks.

[Jer Noble]

Committed r160810: <http://trac.webkit.org/changeset/160810>