Bug 232375

Summary: Ensure synchronized rendering of incoming audio tracks
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, eric.carlson, ews-watchlist, glenn, hta, japhet, jer.noble, philipj, sergio, tommyw, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description youenn fablet 2021-10-27 05:43:48 PDT
Ensure synchronized rendering of incoming audio tracks
Comment 1 youenn fablet 2021-10-28 00:28:44 PDT
Created attachment 442682 [details]
Patch
Comment 2 youenn fablet 2021-10-28 01:28:55 PDT
Created attachment 442683 [details]
Patch
Comment 3 youenn fablet 2021-10-28 05:31:35 PDT
Created attachment 442694 [details]
Patch
Comment 4 youenn fablet 2021-10-28 06:52:58 PDT
Created attachment 442703 [details]
Patch
Comment 5 Eric Carlson 2021-10-28 13:08:19 PDT
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.
Comment 6 youenn fablet 2021-10-29 05:47:02 PDT
(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.
Comment 7 youenn fablet 2021-10-29 05:47:41 PDT
Created attachment 442810 [details]
Patch for landing
Comment 8 EWS 2021-10-29 06:36:12 PDT
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].
Comment 9 Radar WebKit Bug Importer 2021-10-29 06:37:18 PDT
<rdar://problem/84806583>
Comment 10 youenn fablet 2021-11-07 23:45:45 PST
(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