Bug 206152 - Expose audio tracks for media files in the GPUProcess
Summary: Expose audio tracks for media files in the GPUProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-12 10:37 PST by Eric Carlson
Modified: 2020-01-14 08:57 PST (History)
9 users (show)

See Also:


Attachments
Patch (63.27 KB, patch)
2020-01-12 10:52 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (63.84 KB, patch)
2020-01-12 11:43 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing (63.86 KB, patch)
2020-01-13 08:58 PST, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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