Bug 225603 – Implement a remote Internal Unit in GPUProcess for audio MediaStreamTrack rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Implement a remote Internal Unit in GPUProcess for audio MediaStreamTrack rendering

[youenn fablet]

Created attachment 428188 [details]
Patch

[youenn fablet]

Created attachment 428367 [details]
Patch

[youenn fablet]

Created attachment 428369 [details]
Patch

[youenn fablet]

A follow-up patch will remove the code sending individual audio track content to GPUProcess.

[Eric Carlson]

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?

[Radar WebKit Bug Importer]

<rdar://problem/78114391>

[youenn fablet]

Created attachment 428924 [details]
Patch

[youenn fablet]

Created attachment 428929 [details]
Patch for landing

[youenn fablet]

(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.

[youenn fablet]

Comment on attachment 428929 [details]
Patch for landing

iOS API test failure unrelated

[EWS]

Patch 428929 does not build

[youenn fablet]

Created attachment 429164 [details]
Patch for landing

[youenn fablet]

Created attachment 429273 [details]
Fix small regression compared to previous approach

[youenn fablet]

(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.

[EWS]

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

[youenn fablet]

Created attachment 429274 [details]
Patch for landing

[EWS]

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].

[Chris Dumez]

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?

[youenn fablet]

Yes, it should be renamed.

[Chris Dumez]

(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.

[Chris Dumez]

(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