Summary: | Reconfiguring the CoreAudioSharedUnit should take into account that there is a speaker sample producer | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric.carlson, ews-watchlist, ggaren, glenn, hta, jer.noble, philipj, sergio, tommyw, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
youenn fablet
2022-01-20 03:03:40 PST
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]. |