RESOLVED FIXED 212406
Use one audio unit for all MediaStreamTracks of a given process
https://bugs.webkit.org/show_bug.cgi?id=212406
Summary Use one audio unit for all MediaStreamTracks of a given process
youenn fablet
Reported 2020-05-27 05:27:05 PDT
Use one audio unit for all tracks of a given process
Attachments
Patch (39.01 KB, patch)
2020-05-27 06:10 PDT, youenn fablet
no flags
Patch (40.07 KB, patch)
2020-05-27 06:20 PDT, youenn fablet
no flags
Patch (39.90 KB, patch)
2020-05-28 02:17 PDT, youenn fablet
no flags
Patch (39.91 KB, patch)
2020-05-29 05:45 PDT, youenn fablet
no flags
Patch for landing (40.45 KB, patch)
2020-06-08 05:02 PDT, youenn fablet
no flags
youenn fablet
Comment 1 2020-05-27 06:10:48 PDT
youenn fablet
Comment 2 2020-05-27 06:20:28 PDT
Eric Carlson
Comment 3 2020-05-27 10:13:33 PDT
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
Comment 4 2020-05-27 13:32:51 PDT
(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
Comment 5 2020-05-28 02:17:37 PDT
youenn fablet
Comment 6 2020-05-29 05:06:32 PDT
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
Comment 7 2020-05-29 05:45:16 PDT
youenn fablet
Comment 8 2020-06-05 07:56:45 PDT
ping review
Eric Carlson
Comment 9 2020-06-05 08:43:31 PDT
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 10 2020-06-05 09:37:15 PDT
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
Comment 11 2020-06-08 04:45:30 PDT
> > 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
Comment 12 2020-06-08 05:02:07 PDT
Created attachment 401325 [details] Patch for landing
EWS
Comment 13 2020-06-08 07:01:02 PDT
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
Comment 14 2020-06-08 07:02:18 PDT
Note You need to log in before you can comment on or make changes to this bug.