Bug 212406 – Use one audio unit for all MediaStreamTracks of a given process

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

Use one audio unit for all tracks of a given process

[youenn fablet]

Created attachment 400325 [details]
Patch

[youenn fablet]

Created attachment 400327 [details]
Patch

[Eric Carlson]

Comment on attachment 400327 [details]
Patch

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

> Source/WebCore/platform/audio/mac/AudioSampleBufferList.cpp:-117
> -    if (source.streamDescription() != streamDescription())
> -        return kAudio_ParamError;
> -
> -    if (frameCount > source.sampleCount())
> -        frameCount = source.sampleCount();
> -
> -    if (frameCount > m_sampleCapacity)
> -        return kAudio_ParamError;

Maybe add ASSERTS for these?

> Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererUnit.cpp:113
> +    if (m_isStarted)
> +        AudioOutputUnitStop(m_remoteIOUnit);
> +
> +    AudioComponentInstanceDispose(m_remoteIOUnit);
> +    m_remoteIOUnit = nullptr;

Should you set m_isStarted to false?

> Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererUnit.cpp:203
> +    // Mix all sources.

Nit: this comment belongs with the next block.

[youenn fablet]

(In reply to Eric Carlson from comment #3)
> Comment on attachment 400327 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400327&action=review
> 
> > Source/WebCore/platform/audio/mac/AudioSampleBufferList.cpp:-117
> > -    if (source.streamDescription() != streamDescription())
> > -        return kAudio_ParamError;
> > -
> > -    if (frameCount > source.sampleCount())
> > -        frameCount = source.sampleCount();
> > -
> > -    if (frameCount > m_sampleCapacity)
> > -        return kAudio_ParamError;
> 
> Maybe add ASSERTS for these?

Will do.

> > Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererUnit.cpp:113
> > +    if (m_isStarted)
> > +        AudioOutputUnitStop(m_remoteIOUnit);
> > +
> > +    AudioComponentInstanceDispose(m_remoteIOUnit);
> > +    m_remoteIOUnit = nullptr;
> 
> Should you set m_isStarted to false?

Right, will fix it.

> > Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererUnit.cpp:203
> > +    // Mix all sources.
> 
> Nit: this comment belongs with the next block.

OK

[youenn fablet]

Created attachment 400438 [details]
Patch

[youenn fablet]

I tested using the core audio shared unit on MacOS in WebProcess and used it to play the track samples and this works. I haven't tried yet on iOS.

[youenn fablet]

Created attachment 400575 [details]
Patch

[youenn fablet]

ping review

[Eric Carlson]

Comment on attachment 400575 [details]
Patch

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

> Source/WebCore/platform/audio/mac/AudioSampleBufferList.cpp:-108
> +static void mixBuffers(WebAudioBufferList& destinationBuffer, const AudioBufferList& sourceBuffer, AudioStreamDescription::PCMFormat format, size_t frameCount)
>  {
> -    ASSERT(source.streamDescription() == streamDescription());

ASSERT(frameCount <= sourceBuffer.sampleCount()) ?

> Source/WebCore/platform/audio/mac/AudioSampleDataSource.mm:294
> +        if (m_volume < .95)

Not new to this patch but the magic number should be a named constant, especially since it is used in more than one place now.

[Jer Noble]

Comment on attachment 400575 [details]
Patch

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

The AudioMediaStreamTrackRendererUnit class here duplicates code in AudioDestinationCocoa. Could we refactor that class and use it here instead? The AudioDestinationCocoa class handles cases where the output buffer size changes, for example.  It also clamps output to [-1, 1] to avoid clipping.

> Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererUnit.cpp:202
> +        m_renderSources = copyToVector(m_sources);

This will malloc on the audio thread, which is a blocking operation, and could cause audible glitches.

[youenn fablet]

> > Source/WebCore/platform/audio/mac/AudioSampleBufferList.cpp:-108
> > +static void mixBuffers(WebAudioBufferList& destinationBuffer, const AudioBufferList& sourceBuffer, AudioStreamDescription::PCMFormat format, size_t frameCount)
> >  {
> > -    ASSERT(source.streamDescription() == streamDescription());
> 
> ASSERT(frameCount <= sourceBuffer.sampleCount()) ?

OK

> > Source/WebCore/platform/audio/mac/AudioSampleDataSource.mm:294
> > +        if (m_volume < .95)
> 
> Not new to this patch but the magic number should be a named constant,
> especially since it is used in more than one place now.

I added a constexpr.

> The AudioMediaStreamTrackRendererUnit class here duplicates code in
> AudioDestinationCocoa. Could we refactor that class and use it here instead?
> The AudioDestinationCocoa class handles cases where the output buffer size
> changes, for example.  It also clamps output to [-1, 1] to avoid clipping.

Yes, I saw the duplication. I'll refactor code as a follow-up.

> > Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererUnit.cpp:202
> > +        m_renderSources = copyToVector(m_sources);
> 
> This will malloc on the audio thread, which is a blocking operation, and
> could cause audible glitches.

True, I will add another Vector as a member variable.

[youenn fablet]

Created attachment 401325 [details]
Patch for landing

[EWS]

Committed r262710: <https://trac.webkit.org/changeset/262710>

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

[Radar WebKit Bug Importer]

<rdar://problem/64117578>