[webkit-reviews] review granted: [Bug 206152] Expose audio tracks for media files in the GPUProcess : [Attachment 387484] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 206152: Expose audio tracks for media files in the GPUProcess
https://bugs.webkit.org/show_bug.cgi?id=206152

Attachment 387484: Patch

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




--- Comment #4 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 387484
  --> https://bugs.webkit.org/attachment.cgi?id=387484
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(MediaPlayerPrivateRemoteIdentifie
r 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