Reconfiguring the CoreAudioSharedUnit should take into account that there is a speaker sample producer
<rdar://problem/87819949>
Created attachment 449561 [details] Patch
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/
Created attachment 449586 [details] Patch for landing
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.
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].
(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.
Reopening to attach new patch.
Created attachment 449640 [details] Patch
Comment on attachment 449640 [details] Patch bot failure unrelated.
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].