Implement AVSampleBufferAudioRenderer and AVSampleBufferSynchronizer in MediaPlayerPrivateMediaStream.
<rdar://problem/27380390>
Created attachment 285991 [details] WIP
Created attachment 298603 [details] Proposed patch.
Comment on attachment 298603 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=298603&action=review r=me with nits and comments. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:156 > +- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context > +{ > + UNUSED_PARAM(context); > + UNUSED_PARAM(keyPath); > + ASSERT(_parent); > + > + RetainPtr<WebAVSampleBufferStatusChangeListener> protectedSelf = self; > + if ([object isKindOfClass:getAVSampleBufferDisplayLayerClass()]) { The way I like to do these KVO observer methods is like this: static void* DisplayLayerContext = &DisplayLayerContext; static void* AudioRendererContext = &AudioRendererContext; ... if (context == DisplayLayerContext) { ... } else if (context == AudioRendererContext) { ... }. Your way is totally fine too. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:269 > + MediaTime now = streamTime(); > + while (!queue.isEmpty()) { > + if (queue.first()->decodeTime() > now) > + break; > + queue.removeFirst(); > + }; For SourceBuffer, we use a variant of SampleMap here, to ensure the samples are dequeued in decode order. Specifically, we use a DecodeOrderSampleMap::MapType, where MapType is a std::map from a std::pair<> of DecodeTime, PresentationTime -> MediaTime. If you can guarantee decode order insertion into this queue, you don't need it, but if there's a chance that samples will come in out-of-order, you'll eventually need something like DecodeOrderSampleMap (or to sort the queue). With a std::map, this would turn into: queue.erase(queue.start(), queue.upper_bound({now, MediaTime::zeroTime()}); (Erase everything from the beginning up until the first sample with a decode time greater than now.) > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:319 > + static const double audioRendererLatency = 0.2; > + auto audioTrack = m_audioTrackMap.get(track.id()); > + MediaTime timelineOffset = audioTrack->timelineOffset(); > + if (timelineOffset == MediaTime::invalidTime()) { > + timelineOffset = calculateTimelineOffset(sample, audioRendererLatency); > + audioTrack->setTimelineOffset(timelineOffset); This... > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:351 > + auto videoTrack = m_videoTrackMap.get(track.id()); > + MediaTime timelineOffset = videoTrack->timelineOffset(); > + if (timelineOffset == MediaTime::invalidTime()) { > + timelineOffset = calculateTimelineOffset(sample, 0.01); And this. First off, we should probably document the 0.01s value (the same way audioRendererLatency is documented by making it a static const). But shouldn't the offset values be the same between the audio and video tracks? Like, should this be stored on a per MediaPlayer basis rather than a per-track basis, so that the audio and video doesn't get out of sync? > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:455 > + m_pendingAudioSampleQueue.clear(); > + for (auto& renderer : m_audioRenderers.values()) { > + [m_statusChangeListener stopObservingRenderer:renderer.get()]; > + [renderer flush]; > + [renderer stopRequestingMediaData]; > + } > + m_audioRenderers.clear(); We don't remove the renderers from the synchronizer here, but we do above in destroyAudioRenderer(). Could we refactor this a bit to make a destroyAudioRenderer(AVSampleBufferAudioRenderer*), and call that from both destroyAudioRenderers() and destroyAudioRenderer(AtomicString) ? > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:485 > + if (status.integerValue == AVQueuedSampleBufferRenderingStatusRendering) > + m_audioTrackMap.get(trackID)->setTimelineOffset(MediaTime::invalidTime()); I don't know that this is necessary. You'll probably get the KVO that triggers this after you've already started pushing in data; it may cause discontinuities. > Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:44 > +MediaTime MediaSampleAVFObjC::outputPresentationTime() const > +{ > + return toMediaTime(CMSampleBufferGetOutputPresentationTimeStamp(m_sample.get())); > +} > + I don't think Output Presentation Timestamps are necessary here. I doubt we'll ever be dealing with samples that play backwards, or are trimmed.
(In reply to comment #4) > Comment on attachment 298603 [details] > Proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=298603&action=review > > r=me with nits and comments. > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:156 > > +- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context > > +{ > > + UNUSED_PARAM(context); > > + UNUSED_PARAM(keyPath); > > + ASSERT(_parent); > > + > > + RetainPtr<WebAVSampleBufferStatusChangeListener> protectedSelf = self; > > + if ([object isKindOfClass:getAVSampleBufferDisplayLayerClass()]) { > > The way I like to do these KVO observer methods is like this: > > static void* DisplayLayerContext = &DisplayLayerContext; > static void* AudioRendererContext = &AudioRendererContext; > > ... > > if (context == DisplayLayerContext) { ... } else if (context == > AudioRendererContext) { ... }. > > Your way is totally fine too. > I copy/pasted this from WebAVSampleBufferErrorListener in SourceBufferPrivateAVFObjC :-) > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:269 > > + MediaTime now = streamTime(); > > + while (!queue.isEmpty()) { > > + if (queue.first()->decodeTime() > now) > > + break; > > + queue.removeFirst(); > > + }; > > For SourceBuffer, we use a variant of SampleMap here, to ensure the samples > are dequeued in decode order. Specifically, we use a > DecodeOrderSampleMap::MapType, where MapType is a std::map from a > std::pair<> of DecodeTime, PresentationTime -> MediaTime. If you can > guarantee decode order insertion into this queue, you don't need it, but if > there's a chance that samples will come in out-of-order, you'll eventually > need something like DecodeOrderSampleMap (or to sort the queue). > > With a std::map, this would turn into: queue.erase(queue.start(), > queue.upper_bound({now, MediaTime::zeroTime()}); (Erase everything from the > beginning up until the first sample with a decode time greater than now.) > Thanks, we don't need that yet but will soon. I will change in an upcoming patch. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:319 > > + static const double audioRendererLatency = 0.2; > > + auto audioTrack = m_audioTrackMap.get(track.id()); > > + MediaTime timelineOffset = audioTrack->timelineOffset(); > > + if (timelineOffset == MediaTime::invalidTime()) { > > + timelineOffset = calculateTimelineOffset(sample, audioRendererLatency); > > + audioTrack->setTimelineOffset(timelineOffset); > > This... > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:351 > > + auto videoTrack = m_videoTrackMap.get(track.id()); > > + MediaTime timelineOffset = videoTrack->timelineOffset(); > > + if (timelineOffset == MediaTime::invalidTime()) { > > + timelineOffset = calculateTimelineOffset(sample, 0.01); > > And this. First off, we should probably document the 0.01s value (the same > way audioRendererLatency is documented by making it a static const). But > shouldn't the offset values be the same between the audio and video tracks? > Like, should this be stored on a per MediaPlayer basis rather than a > per-track basis, so that the audio and video doesn't get out of sync? > Yes, there should be one value. We need to experiment to figure out what the right value is long term, but fixed for now. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:455 > > + m_pendingAudioSampleQueue.clear(); > > + for (auto& renderer : m_audioRenderers.values()) { > > + [m_statusChangeListener stopObservingRenderer:renderer.get()]; > > + [renderer flush]; > > + [renderer stopRequestingMediaData]; > > + } > > + m_audioRenderers.clear(); > > We don't remove the renderers from the synchronizer here, but we do above in > destroyAudioRenderer(). Could we refactor this a bit to make a > destroyAudioRenderer(AVSampleBufferAudioRenderer*), and call that from both > destroyAudioRenderers() and destroyAudioRenderer(AtomicString) ? > Fixed. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:485 > > + if (status.integerValue == AVQueuedSampleBufferRenderingStatusRendering) > > + m_audioTrackMap.get(trackID)->setTimelineOffset(MediaTime::invalidTime()); > > I don't know that this is necessary. You'll probably get the KVO that > triggers this after you've already started pushing in data; it may cause > discontinuities. > This only forces a recalculation of the offset, I don't think it should cause discontinuities. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:44 > > +MediaTime MediaSampleAVFObjC::outputPresentationTime() const > > +{ > > + return toMediaTime(CMSampleBufferGetOutputPresentationTimeStamp(m_sample.get())); > > +} > > + > > I don't think Output Presentation Timestamps are necessary here. I doubt > we'll ever be dealing with samples that play backwards, or are trimmed. > We can talk about this offline and revisit.
Created attachment 298646 [details] Patch for landing.
Created attachment 298653 [details] Updated patch for landing.
Comment on attachment 298653 [details] Updated patch for landing. Clearing flags on attachment: 298653 Committed r210621: <http://trac.webkit.org/changeset/210621>
All reviewed patches have been landed. Closing bug.
Plus https://trac.webkit.org/r210631 to revert an accidental change.