WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
211583
Handle remote audio capture IPC messages in a background thread
https://bugs.webkit.org/show_bug.cgi?id=211583
Summary
Handle remote audio capture IPC messages in a background thread
youenn fablet
Reported
2020-05-07 10:34:01 PDT
Handle remote audio capture IPC messages in a background thread
Attachments
Patch
(68.97 KB, patch)
2020-05-07 10:44 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(73.51 KB, patch)
2020-05-07 15:36 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(73.50 KB, patch)
2020-05-08 02:29 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2020-05-07 10:44:57 PDT
Created
attachment 398761
[details]
Patch
Eric Carlson
Comment 2
2020-05-07 13:33:05 PDT
Comment on
attachment 398761
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=398761&action=review
> Source/WebKit/ChangeLog:10 > + Move UserMediaCaptureManager::Source in its own file and rename it to RemoveRealtimeMediaSource.
s/RemoveRealtimeMediaSource/RemoteRealtimeMediaSource/
> Source/WebKit/WebProcess/cocoa/AudioCaptureSampleManager.cpp:63 > + dispatchToThread([this, source = WTFMove(source)]() mutable {
Should we assert that this is called on the main thread?
> Source/WebKit/WebProcess/cocoa/AudioCaptureSampleManager.cpp:74 > + dispatchToThread([this, identifier] {
Ditto
> Source/WebKit/WebProcess/cocoa/AudioCaptureSampleManager.cpp:127 > + WebAudioBufferList audioData(source.description, numberOfFrames);
It would be more efficient to to have this in an instance variable, which gets reset when necessary
> Source/WebKit/WebProcess/cocoa/RemoteRealtimeMediaSource.cpp:29 > +#if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM)
We're going to have to split this into a base class and a Cocoa-specific derived class at some point.
youenn fablet
Comment 3
2020-05-07 15:36:37 PDT
Created
attachment 398805
[details]
Patch
Eric Carlson
Comment 4
2020-05-07 15:55:18 PDT
Comment on
attachment 398805
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=398805&action=review
r=me once the bots are happy.
> Source/WebKit/WebProcess/cocoa/RemoteCaptureSampleManager.h:2 > + * Copyright (C) 2017 Apple Inc. All rights reserved.
2020
> Source/WebKit/WebProcess/cocoa/RemoteCaptureSampleManager.messages.in:1 > +# Copyright (C) 2017 Apple Inc. All rights reserved.
2020
youenn fablet
Comment 5
2020-05-08 02:29:04 PDT
Created
attachment 398844
[details]
Patch for landing
youenn fablet
Comment 6
2020-05-08 02:29:08 PDT
Need to land
https://bugs.webkit.org/show_bug.cgi?id=211509
to fix iOS compilation errors first
youenn fablet
Comment 7
2020-05-08 02:32:35 PDT
Thanks for the review. (In reply to Eric Carlson from
comment #2
)
> Comment on
attachment 398761
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=398761&action=review
> > > Source/WebKit/ChangeLog:10 > > + Move UserMediaCaptureManager::Source in its own file and rename it to RemoveRealtimeMediaSource. > > s/RemoveRealtimeMediaSource/RemoteRealtimeMediaSource/
Done
> > Source/WebKit/WebProcess/cocoa/AudioCaptureSampleManager.cpp:63 > > + dispatchToThread([this, source = WTFMove(source)]() mutable { > > Should we assert that this is called on the main thread?
This is not strictly needed but does not hurt since that is the spirit of the model. Added.
> > Source/WebKit/WebProcess/cocoa/AudioCaptureSampleManager.cpp:74 > > + dispatchToThread([this, identifier] { > > Ditto
Ditto.
> > Source/WebKit/WebProcess/cocoa/AudioCaptureSampleManager.cpp:127
I renamed AudioCaptureSampleManager to RemoteCaptureSampleManager since we will want to do the same thing for video content.
> > + WebAudioBufferList audioData(source.description, numberOfFrames); > > It would be more efficient to to have this in an instance variable, which > gets reset when necessary
Right! Added a FIXME.
> > Source/WebKit/WebProcess/cocoa/RemoteRealtimeMediaSource.cpp:29 > > +#if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM) > > We're going to have to split this into a base class and a Cocoa-specific > derived class at some point.
Right, I think splitting the class helps, UserMediaCaptureManager should no longer be cocoa specific. As of RemoteRealtimeMediaSource, we might be able to keep it platform agnostic but make its RemoteAudio subclass platform specific in the future.
EWS
Comment 8
2020-05-08 05:42:07 PDT
Committed
r261375
: <
https://trac.webkit.org/changeset/261375
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 398844
[details]
.
Radar WebKit Bug Importer
Comment 9
2020-05-08 05:43:22 PDT
<
rdar://problem/63018424
>
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