Bug 206152 – Expose audio tracks for media files in the GPUProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Expose audio tracks for media files in the GPUProcess

[Radar WebKit Bug Importer]

<rdar://problem/58513227>

[Eric Carlson]

Created attachment 387477 [details]
Patch

[Eric Carlson]

Created attachment 387484 [details]
Patch

[youenn fablet]

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

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 387536 [details]
Patch for landing

[youenn fablet]

> > auto is probably refing/unrefing, might be better to use auto&.
> 
> The map has WeakPtrs.

This probably triggers ref/unref of a RefPtr<WeakPtrImpl>.

[Eric Carlson]

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

[WebKit Commit Bot]

Comment on attachment 387536 [details]
Patch for landing

Clearing flags on attachment: 387536

Committed r254454: <https://trac.webkit.org/changeset/254454>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

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