Bug 125905

Created attachment 219489 [details] Patch Initial version. The ChangeLog is a little sparse, and I intend to flesh it out.

Created attachment 219534 [details] Patch

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.

(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.

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