Use one audio unit for all tracks of a given process
Created attachment 400325 [details] Patch
Created attachment 400327 [details] Patch
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.
(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
Created attachment 400438 [details] Patch
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.
Created attachment 400575 [details] Patch
ping review
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.
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.
> > 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.
Created attachment 401325 [details] Patch for landing
Committed r262710: <https://trac.webkit.org/changeset/262710> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401325 [details].
<rdar://problem/64117578>