Bug 232375 – Ensure synchronized rendering of incoming audio tracks

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Ensure synchronized rendering of incoming audio tracks

[youenn fablet]

Created attachment 442682 [details]
Patch

[youenn fablet]

Created attachment 442683 [details]
Patch

[youenn fablet]

Created attachment 442694 [details]
Patch

[youenn fablet]

Created attachment 442703 [details]
Patch

[Eric Carlson]

Comment on attachment 442703 [details]
Patch

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

r=me once the bots are happy

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.cpp:146
> +        if (m_isRenderingIncomingAudio)
> +            m_incomingAudioMediaStreamTrackRendererUnit->newAudioChunkPushed();
> +        m_currentAudioSampleCount += LibWebRTCAudioFormat::chunkSampleCount;

Is it correct to update m_currentAudioSampleCount after calling `newAudioChunkPushed`? In either case, it might be clearer to pass sample count to `newAudioChunkPushed()` rather than having it query the module.

[youenn fablet]

(In reply to Eric Carlson from comment #5)
> Comment on attachment 442703 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442703&action=review
> 
> r=me once the bots are happy

Issue was with resetting the audio model when resetting the factory.

> > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.cpp:146
> > +        if (m_isRenderingIncomingAudio)
> > +            m_incomingAudioMediaStreamTrackRendererUnit->newAudioChunkPushed();
> > +        m_currentAudioSampleCount += LibWebRTCAudioFormat::chunkSampleCount;
> 
> Is it correct to update m_currentAudioSampleCount after calling
> `newAudioChunkPushed`? In either case, it might be clearer to pass sample
> count to `newAudioChunkPushed()` rather than having it query the module.

Right, passing the sample count to newAudioChunkPushed is a good idea.
As of the order, I think it makes sense this way, we start calling newAudioChunkPushed with count = 0.

[youenn fablet]

Created attachment 442810 [details]
Patch for landing

[EWS]

Committed r285027 (243671@main): <https://commits.webkit.org/243671@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442810 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84806583>

[youenn fablet]

(In reply to youenn fablet from comment #6)
> (In reply to Eric Carlson from comment #5)
> > Comment on attachment 442703 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=442703&action=review
> > 
> > r=me once the bots are happy
> 
> Issue was with resetting the audio model when resetting the factory.
> 
> > > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.cpp:146
> > > +        if (m_isRenderingIncomingAudio)
> > > +            m_incomingAudioMediaStreamTrackRendererUnit->newAudioChunkPushed();
> > > +        m_currentAudioSampleCount += LibWebRTCAudioFormat::chunkSampleCount;
> > 
> > Is it correct to update m_currentAudioSampleCount after calling
> > `newAudioChunkPushed`? In either case, it might be clearer to pass sample
> > count to `newAudioChunkPushed()` rather than having it query the module.
> 
> Right, passing the sample count to newAudioChunkPushed is a good idea.
> As of the order, I think it makes sense this way, we start calling
> newAudioChunkPushed with count = 0.

I forgot to update newAudioChunkPushed, will do this as a follow-up