Bug 225603 - Implement a remote Internal Unit in GPUProcess for audio MediaStreamTrack rendering
Summary: Implement a remote Internal Unit in GPUProcess for audio MediaStreamTrack ren...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-10 11:35 PDT by youenn fablet
Modified: 2021-05-21 11:34 PDT (History)
14 users (show)

See Also:

Attachments
Patch (57.58 KB, patch)
2021-05-10 11:41 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch (56.09 KB, patch)
2021-05-12 03:36 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch (57.54 KB, patch)
2021-05-12 05:13 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch (58.07 KB, patch)
2021-05-18 03:03 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch for landing (58.52 KB, patch)
2021-05-18 04:45 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch for landing (58.53 KB, patch)
2021-05-20 06:45 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Fix small regression compared to previous approach (62.96 KB, patch)
2021-05-21 02:01 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch for landing (62.96 KB, patch)
2021-05-21 02:04 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Show Obsolete (7) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2021-05-10 11:35:00 PDT 
Implement a remote Internal Unit in GPUProcess for audio MediaStreamTrack rendering
Comment 1 youenn fablet 2021-05-10 11:41:06 PDT 
Created attachment 428188 [details]
Patch
Comment 2 youenn fablet 2021-05-12 03:36:14 PDT 
Created attachment 428367 [details]
Patch
Comment 3 youenn fablet 2021-05-12 05:13:22 PDT 
Created attachment 428369 [details]
Patch
Comment 4 youenn fablet 2021-05-12 06:31:01 PDT 
A follow-up patch will remove the code sending individual audio track content to GPUProcess.
Comment 5 Eric Carlson 2021-05-12 09:31:22 PDT 
Comment on attachment 428369 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428369&action=review

> Source/WebKit/WebProcess/GPU/webrtc/AudioMediaStreamTrackRendererInternalUnitManager.cpp:107
> +    for (auto proxy : m_proxies.values())
> +        proxy->restartIfNeeded();

Will we want to always relaunch the GPU process as soon as it exits, e.g. if under memory pressure?
Comment 6 Radar WebKit Bug Importer 2021-05-17 11:35:19 PDT 
<rdar://problem/78114391>
Comment 7 youenn fablet 2021-05-18 03:03:56 PDT 
Created attachment 428924 [details]
Patch
Comment 8 youenn fablet 2021-05-18 04:45:10 PDT 
Created attachment 428929 [details]
Patch for landing
Comment 9 youenn fablet 2021-05-18 04:45:57 PDT 
(In reply to Eric Carlson from comment #5)
> Comment on attachment 428369 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428369&action=review
> 
> > Source/WebKit/WebProcess/GPU/webrtc/AudioMediaStreamTrackRendererInternalUnitManager.cpp:107
> > +    for (auto proxy : m_proxies.values())
> > +        proxy->restartIfNeeded();
> 
> Will we want to always relaunch the GPU process as soon as it exits, e.g. if
> under memory pressure?

Good point, GPU Process will only restart if playing audio.
Otherwise, it will wait for the unit to start to restart GPU Process.
Comment 10 youenn fablet 2021-05-20 00:58:08 PDT 
Comment on attachment 428929 [details]
Patch for landing

iOS API test failure unrelated
Comment 11 EWS 2021-05-20 01:53:03 PDT 
Patch 428929 does not build
Comment 12 youenn fablet 2021-05-20 06:45:02 PDT 
Created attachment 429164 [details]
Patch for landing
Comment 13 youenn fablet 2021-05-21 02:01:27 PDT 
Created attachment 429273 [details]
Fix small regression compared to previous approach
Comment 14 youenn fablet 2021-05-21 02:02:00 PDT 
(In reply to youenn fablet from comment #13)
> Created attachment 429273 [details]
> Fix small regression compared to previous approach

Regression was introduced by start being called for every source added.
Comment 15 EWS 2021-05-21 02:02:45 PDT 
ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.
Comment 16 youenn fablet 2021-05-21 02:04:02 PDT 
Created attachment 429274 [details]
Patch for landing
Comment 17 EWS 2021-05-21 02:53:45 PDT 
Committed r277852 (237994@main): <https://commits.webkit.org/237994@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429274 [details].
Comment 18 Chris Dumez 2021-05-21 07:37:23 PDT 
Comment on attachment 429274 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=429274&action=review

> Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererUnit.cpp:111
>          auto locker = holdLock(m_sourcesLock);

Is m_sourcesLock only protecting m_sourcesCopy and not m_sources? If so, can it be renamed?
Comment 19 youenn fablet 2021-05-21 10:06:17 PDT 
Yes, it should be renamed.
Comment 20 Chris Dumez 2021-05-21 11:31:11 PDT 
(In reply to youenn fablet from comment #19)
> Yes, it should be renamed.

I am doing it now and adopting CheckedLock at the same time. Will cc you.
Comment 21 Chris Dumez 2021-05-21 11:34:08 PDT 
(In reply to Chris Dumez from comment #20)
> (In reply to youenn fablet from comment #19)
> > Yes, it should be renamed.
> 
> I am doing it now and adopting CheckedLock at the same time. Will cc you.

-> https://bugs.webkit.org/show_bug.cgi?id=226095