Bug 235397 - Reconfiguring the CoreAudioSharedUnit should take into account that there is a speaker sample producer
Summary: Reconfiguring the CoreAudioSharedUnit should take into account that there is ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-20 03:03 PST by youenn fablet
Modified: 2022-01-24 07:38 PST (History)
11 users (show)

See Also:


Attachments
Patch (2.41 KB, patch)
2022-01-20 03:06 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (2.42 KB, patch)
2022-01-20 09:03 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (4.13 KB, patch)
2022-01-20 23:15 PST, 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 2022-01-20 03:03:40 PST
Reconfiguring the CoreAudioSharedUnit should take into account that there is a speaker sample producer
Comment 1 Radar WebKit Bug Importer 2022-01-20 03:06:03 PST
<rdar://problem/87819949>
Comment 2 youenn fablet 2022-01-20 03:06:19 PST
Created attachment 449561 [details]
Patch
Comment 3 Eric Carlson 2022-01-20 08:24:35 PST
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/
Comment 4 youenn fablet 2022-01-20 09:03:47 PST
Created attachment 449586 [details]
Patch for landing
Comment 5 Geoffrey Garen 2022-01-20 09:35:27 PST
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.
Comment 6 EWS 2022-01-20 10:08:00 PST
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].
Comment 7 youenn fablet 2022-01-20 23:01:55 PST
(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.
Comment 8 youenn fablet 2022-01-20 23:15:02 PST
Reopening to attach new patch.
Comment 9 youenn fablet 2022-01-20 23:15:06 PST
Created attachment 449640 [details]
Patch
Comment 10 youenn fablet 2022-01-24 07:33:25 PST
Comment on attachment 449640 [details]
Patch

bot failure unrelated.
Comment 11 EWS 2022-01-24 07:38:30 PST
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].