WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(40.07 KB, patch)
2020-05-27 06:20 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(39.90 KB, patch)
2020-05-28 02:17 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(39.91 KB, patch)
2020-05-29 05:45 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(40.45 KB, patch)
2020-06-08 05:02 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2020-05-27 06:10:48 PDT
Created
attachment 400325
[details]
Patch
youenn fablet
Comment 2
2020-05-27 06:20:28 PDT
Created
attachment 400327
[details]
Patch
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
Created
attachment 400438
[details]
Patch
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
Created
attachment 400575
[details]
Patch
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
<
rdar://problem/64117578
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug