Bug 235397 – Reconfiguring the CoreAudioSharedUnit should take into account that there is a speaker sample producer

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

Reconfiguring the CoreAudioSharedUnit should take into account that there is a speaker sample producer

[Radar WebKit Bug Importer]

<rdar://problem/87819949>

[youenn fablet]

Created attachment 449561 [details]
Patch

[Eric Carlson]

Comment on attachment 449561 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        When switching of microphone, we might switch of speaker as well which requires reconfiguring the audio unit.

s/of/off/

[youenn fablet]

Created attachment 449586 [details]
Patch for landing

[Geoffrey Garen]

Comment on attachment 449586 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:13
> +        To prevent this, we set the producer temporarily, stop the audio unit, then set back the producer.

Which function are we trying to influence by setting m_speakerSamplesProducer to null?

I wonder if it would improve encapsulation to use a separate boolean "m_isReconfiguringAudioUnit", and have the target function check that boolean explicitly. When we set m_speakerSamplesProducer to null temporarily, we potentially change the behavior of every function in the class. If we used a separate boolean, we would only change the behavior of functions that explicitly adopted it.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:459
> +            speakerSamplesProducer = m_speakerSamplesProducer;
> +            m_speakerSamplesProducer = nullptr;

This can be written as "speakerSamplesProducer = std::exchange(m_speakerSamplesProducer, nullptr)".

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:464
> +            m_speakerSamplesProducer = speakerSamplesProducer;

Might also be nice to std::exchange with nullptr here, to guarantee that code below cannot use speakerSamplesProducer erroneously.

[EWS]

Committed r288302 (246220@main): <https://commits.webkit.org/246220@main>

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

[youenn fablet]

(In reply to Geoffrey Garen from comment #5)
> Comment on attachment 449586 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449586&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        To prevent this, we set the producer temporarily, stop the audio unit, then set back the producer.
> 
> Which function are we trying to influence by setting
> m_speakerSamplesProducer to null?

We are trying to influence CoreAudioSharedUnit::provideSpeakerData.


> I wonder if it would improve encapsulation to use a separate boolean
> "m_isReconfiguringAudioUnit", and have the target function check that
> boolean explicitly. When we set m_speakerSamplesProducer to null
> temporarily, we potentially change the behavior of every function in the
> class. If we used a separate boolean, we would only change the behavior of
> functions that explicitly adopted it.

Right, this should do the trick as well.
I'll have a try.

> > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:459
> > +            speakerSamplesProducer = m_speakerSamplesProducer;
> > +            m_speakerSamplesProducer = nullptr;
> 
> This can be written as "speakerSamplesProducer =
> std::exchange(m_speakerSamplesProducer, nullptr)".
> 
> > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:464
> > +            m_speakerSamplesProducer = speakerSamplesProducer;
> 
> Might also be nice to std::exchange with nullptr here, to guarantee that
> code below cannot use speakerSamplesProducer erroneously.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 449640 [details]
Patch

[youenn fablet]

Comment on attachment 449640 [details]
Patch

bot failure unrelated.

[EWS]

Committed r288439 (246328@main): <https://commits.webkit.org/246328@main>

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