Bug 211287 - AudioMediaStreamTrackRendererCocoa should create/start/stop its remote unit on the main thread
Summary: AudioMediaStreamTrackRendererCocoa should create/start/stop its remote unit o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
: 208134 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-05-01 07:12 PDT by youenn fablet
Modified: 2020-06-08 11:02 PDT (History)
11 users (show)

See Also:


Attachments
Patch (10.38 KB, patch)
2020-05-01 07:20 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (10.67 KB, patch)
2020-05-01 08:58 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (11.47 KB, patch)
2020-05-01 09:35 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (13.20 KB, patch)
2020-05-01 09:54 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2020-05-01 07:12:11 PDT
AudioMediaStreamTrackRendererCocoa should create/start/stop its remote unit on the main thread
Comment 1 youenn fablet 2020-05-01 07:20:42 PDT
Created attachment 398180 [details]
Patch
Comment 2 youenn fablet 2020-05-01 08:58:57 PDT
Created attachment 398187 [details]
Patch
Comment 3 youenn fablet 2020-05-01 09:24:22 PDT
As discussed with Eric offline, I'll add a comment stating that the design is that pushSamples can only be called between start and stop which is enforced by AudioTrackPrivateMediaStream::startRenderer/stopRenderer.
Comment 4 youenn fablet 2020-05-01 09:35:08 PDT
Created attachment 398191 [details]
Patch for landing
Comment 5 Eric Carlson 2020-05-01 09:42:12 PDT
Comment on attachment 398180 [details]
Patch

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

> Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.cpp:79
>      if (m_remoteIOUnit) {
>          AudioOutputUnitStop(m_remoteIOUnit);
>          AudioComponentInstanceDispose(m_remoteIOUnit);

stop() does the same thing, you could call it here.

> Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.cpp:159
> +    if (!m_dataSource || !m_dataSource->inputDescription() || toCAAudioStreamDescription(*m_dataSource->inputDescription()) != description) {

You can use CAAudioStreamDescription's `operator==(const AudioStreamDescription&)` instead of calling toCAAudioStreamDescription
Comment 6 youenn fablet 2020-05-01 09:54:12 PDT
Created attachment 398193 [details]
Patch for landing
Comment 7 youenn fablet 2020-05-01 09:54:43 PDT
(In reply to Eric Carlson from comment #5)
> Comment on attachment 398180 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398180&action=review
> 
> > Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.cpp:79
> >      if (m_remoteIOUnit) {
> >          AudioOutputUnitStop(m_remoteIOUnit);
> >          AudioComponentInstanceDispose(m_remoteIOUnit);
> 
> stop() does the same thing, you could call it here.

Will do.

> 
> > Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.cpp:159
> > +    if (!m_dataSource || !m_dataSource->inputDescription() || toCAAudioStreamDescription(*m_dataSource->inputDescription()) != description) {
> 
> You can use CAAudioStreamDescription's `operator==(const
> AudioStreamDescription&)` instead of calling toCAAudioStreamDescription

Tried that but it failed so went this way.
I updated CAAudioStreamDescription to make it work, much nicer.
Comment 8 EWS 2020-05-03 11:16:36 PDT
Committed r261063: <https://trac.webkit.org/changeset/261063>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398193 [details].
Comment 9 Radar WebKit Bug Importer 2020-05-03 11:17:13 PDT
<rdar://problem/62811472>
Comment 10 youenn fablet 2020-06-08 11:02:07 PDT
*** Bug 208134 has been marked as a duplicate of this bug. ***