Bug 206152

Summary: Expose audio tracks for media files in the GPUProcess
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, glenn, jer.noble, peng.liu6, philipj, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=206237
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Eric Carlson 2020-01-12 10:37:35 PST
Expose audio tracks for media files in the GPUProcess
Comment 1 Radar WebKit Bug Importer 2020-01-12 10:37:44 PST
<rdar://problem/58513227>
Comment 2 Eric Carlson 2020-01-12 10:52:50 PST
Created attachment 387477 [details]
Patch
Comment 3 Eric Carlson 2020-01-12 11:43:04 PST
Created attachment 387484 [details]
Patch
Comment 4 youenn fablet 2020-01-13 01:09:43 PST
Comment on attachment 387484 [details]
Patch

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

> Source/WebKit/GPUProcess/media/RemoteAudioTrackProxy.cpp:36
> +#include <wtf/NeverDestroyed.h>

Last two headers might not be needed.

> Source/WebKit/GPUProcess/media/RemoteAudioTrackProxy.h:34
> +#include <wtf/ThreadSafeRefCounted.h>

Probably not needed since it is included in TrackPrivateBase.h

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:367
> +void RemoteMediaPlayerProxy::mediaPlayerDidAddAudioTrack(WebCore::AudioTrackPrivate& track)

#if ENABLE(VIDEO_TRACK)?

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:375
> +    m_audioTracks.take(&track);

s/take/remove

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:383
> +            break;

return?

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:385
> +        ASSERT_NOT_REACHED();

Moved to the end of the method?

> Source/WebKit/WebProcess/GPU/media/AudioTrackPrivateRemote.h:31
> +#include "TrackPrivateRemoteConfiguration.h"

Can probably be removed.

> Source/WebKit/WebProcess/GPU/media/AudioTrackPrivateRemote.h:38
> +class AudioTrackPrivateRemote : public WebCore::AudioTrackPrivate {

final?

> Source/WebKit/WebProcess/GPU/media/AudioTrackPrivateRemote.h:41
> +    static RefPtr<AudioTrackPrivateRemote> create(MediaPlayerPrivateRemote& player, TrackPrivateRemoteIdentifier idendifier, TrackPrivateRemoteConfiguration&& configuration)

s/RefPtr/Ref

> Source/WebKit/WebProcess/GPU/media/AudioTrackPrivateRemote.h:53
> +    int trackIndex() const final { return m_trackIndex; }

Do we need these to be public or can we make them private?

> Source/WebKit/WebProcess/GPU/media/AudioTrackPrivateRemote.h:55
> +protected:

private?

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:392
> +    m_audioTracks.add(identifier, track);

We should WTFMove(track) to reduce refcount churn.
We can use the iterator value for the addAudioTrack call.

I am wondering whether we should protect from m_audioTracks to already have identifier as a key. If so, m_player will then have a stale pointer.
Maybe we could use ensure.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:401
> +    if (auto track = m_audioTracks.get(id)) {

s/auto/auto&/

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:323
> +    HashMap<TrackPrivateRemoteIdentifier, RefPtr<AudioTrackPrivateRemote>> m_audioTracks;

s/RefPtr/Ref.

> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:306
> +void RemoteMediaPlayerManager::addRemoteAudioTrack(MediaPlayerPrivateRemoteIdentifier playerID, TrackPrivateRemoteIdentifier trackID, TrackPrivateRemoteConfiguration&& configuration)

Given the amount of IPC method dedicated to player, we could make MediaPlayerPrivateRemote a message receiver, the key being its identifier.
Then, all this binding code would be removed.
Best be done as a follow-up though.

> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:308
> +    if (auto player = m_players.get(playerID))

auto is probably refing/unrefing, might be better to use auto&.

> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:34
>  #include "SharedBufferDataReference.h"

Could remove this one since code is using DataReference, not SharedBufferDataReference
Comment 5 Eric Carlson 2020-01-13 08:56:54 PST
Comment on attachment 387484 [details]
Patch

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

>> Source/WebKit/GPUProcess/media/RemoteAudioTrackProxy.cpp:36
>> +#include <wtf/NeverDestroyed.h>
> 
> Last two headers might not be needed.

Removed.

>> Source/WebKit/GPUProcess/media/RemoteAudioTrackProxy.h:34
>> +#include <wtf/ThreadSafeRefCounted.h>
> 
> Probably not needed since it is included in TrackPrivateBase.h

Removed.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:367
>> +void RemoteMediaPlayerProxy::mediaPlayerDidAddAudioTrack(WebCore::AudioTrackPrivate& track)
> 
> #if ENABLE(VIDEO_TRACK)?

I think we're better off without the compile flag because we always support tracks. If another port decides to use the GPU process and doesn't support tracks, it can add it as needed.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:375
>> +    m_audioTracks.take(&track);
> 
> s/take/remove

Fixed.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:383
>> +            break;
> 
> return?

Fixed.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:385
>> +        ASSERT_NOT_REACHED();
> 
> Moved to the end of the method?

Fixed.

>> Source/WebKit/WebProcess/GPU/media/AudioTrackPrivateRemote.h:31
>> +#include "TrackPrivateRemoteConfiguration.h"
> 
> Can probably be removed.

Removed.

>> Source/WebKit/WebProcess/GPU/media/AudioTrackPrivateRemote.h:38
>> +class AudioTrackPrivateRemote : public WebCore::AudioTrackPrivate {
> 
> final?

Added.

>> Source/WebKit/WebProcess/GPU/media/AudioTrackPrivateRemote.h:41
>> +    static RefPtr<AudioTrackPrivateRemote> create(MediaPlayerPrivateRemote& player, TrackPrivateRemoteIdentifier idendifier, TrackPrivateRemoteConfiguration&& configuration)
> 
> s/RefPtr/Ref

Changed.

>> Source/WebKit/WebProcess/GPU/media/AudioTrackPrivateRemote.h:53
>> +    int trackIndex() const final { return m_trackIndex; }
> 
> Do we need these to be public or can we make them private?

Good point, made private.

>> Source/WebKit/WebProcess/GPU/media/AudioTrackPrivateRemote.h:55
>> +protected:
> 
> private?

Changed.

>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:392
>> +    m_audioTracks.add(identifier, track);
> 
> We should WTFMove(track) to reduce refcount churn.
> We can use the iterator value for the addAudioTrack call.
> 
> I am wondering whether we should protect from m_audioTracks to already have identifier as a key. If so, m_player will then have a stale pointer.
> Maybe we could use ensure.

Changed to WTFMove and add from the iterator.

I changed to return without adding the track if the identifier is already in the map (It should only be possible for this to be called with the same identifier if ObjectIdentifier::generate() returns the same ID twice).

>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:323
>> +    HashMap<TrackPrivateRemoteIdentifier, RefPtr<AudioTrackPrivateRemote>> m_audioTracks;
> 
> s/RefPtr/Ref.

Changed.

>> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:306
>> +void RemoteMediaPlayerManager::addRemoteAudioTrack(MediaPlayerPrivateRemoteIdentifier playerID, TrackPrivateRemoteIdentifier trackID, TrackPrivateRemoteConfiguration&& configuration)
> 
> Given the amount of IPC method dedicated to player, we could make MediaPlayerPrivateRemote a message receiver, the key being its identifier.
> Then, all this binding code would be removed.
> Best be done as a follow-up though.

Good idea, I'll do that in another patch.

>> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:308
>> +    if (auto player = m_players.get(playerID))
> 
> auto is probably refing/unrefing, might be better to use auto&.

The map has WeakPtrs.

>> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:34
>>  #include "SharedBufferDataReference.h"
> 
> Could remove this one since code is using DataReference, not SharedBufferDataReference

Removed.
Comment 6 Eric Carlson 2020-01-13 08:58:21 PST
Created attachment 387536 [details]
Patch for landing
Comment 7 youenn fablet 2020-01-13 09:09:12 PST
> > auto is probably refing/unrefing, might be better to use auto&.
> 
> The map has WeakPtrs.

This probably triggers ref/unref of a RefPtr<WeakPtrImpl>.
Comment 8 Eric Carlson 2020-01-13 09:27:18 PST
(In reply to youenn fablet from comment #7)
> > > auto is probably refing/unrefing, might be better to use auto&.
> > 
> > The map has WeakPtrs.
> 
> This probably triggers ref/unref of a RefPtr<WeakPtrImpl>.

Oops, my reply to incomplete. I also meant to say I changed "auto player = ..." to "const auto& player = ..."
Comment 9 WebKit Commit Bot 2020-01-13 14:27:44 PST
Comment on attachment 387536 [details]
Patch for landing

Clearing flags on attachment: 387536

Committed r254454: <https://trac.webkit.org/changeset/254454>
Comment 10 WebKit Commit Bot 2020-01-13 14:27:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Eric Carlson 2020-01-14 08:57:54 PST
Comment on attachment 387484 [details]
Patch

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

>>> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:306
>>> +void RemoteMediaPlayerManager::addRemoteAudioTrack(MediaPlayerPrivateRemoteIdentifier playerID, TrackPrivateRemoteIdentifier trackID, TrackPrivateRemoteConfiguration&& configuration)
>> 
>> Given the amount of IPC method dedicated to player, we could make MediaPlayerPrivateRemote a message receiver, the key being its identifier.
>> Then, all this binding code would be removed.
>> Best be done as a follow-up though.
> 
> Good idea, I'll do that in another patch.

I filed 206237