Bug 211583 - Handle remote audio capture IPC messages in a background thread
Summary: Handle remote audio capture IPC messages in a background thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-07 10:34 PDT by youenn fablet
Modified: 2020-05-08 05:43 PDT (History)
13 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2020-05-07 10:34:01 PDT
Handle remote audio capture IPC messages in a background thread
Comment 1 youenn fablet 2020-05-07 10:44:57 PDT
Created attachment 398761 [details]
Patch
Comment 2 Eric Carlson 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.
Comment 3 youenn fablet 2020-05-07 15:36:37 PDT
Created attachment 398805 [details]
Patch
Comment 4 Eric Carlson 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
Comment 5 youenn fablet 2020-05-08 02:29:04 PDT
Created attachment 398844 [details]
Patch for landing
Comment 6 youenn fablet 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
Comment 7 youenn fablet 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.
Comment 8 EWS 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].
Comment 9 Radar WebKit Bug Importer 2020-05-08 05:43:22 PDT
<rdar://problem/63018424>