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
Patch (73.51 KB, patch)
2020-05-07 15:36 PDT, youenn fablet
no flags
Patch for landing (73.50 KB, patch)
2020-05-08 02:29 PDT, youenn fablet
no flags
youenn fablet
Comment 1 2020-05-07 10:44:57 PDT
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
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
Note You need to log in before you can comment on or make changes to this bug.