Summary: | Expose audio tracks for media files in the GPUProcess | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||
Component: | Media | Assignee: | 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
Eric Carlson
2020-01-12 10:37:35 PST
Created attachment 387477 [details]
Patch
Created attachment 387484 [details]
Patch
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 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. Created attachment 387536 [details]
Patch for landing
> > auto is probably refing/unrefing, might be better to use auto&.
>
> The map has WeakPtrs.
This probably triggers ref/unref of a RefPtr<WeakPtrImpl>.
(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 on attachment 387536 [details] Patch for landing Clearing flags on attachment: 387536 Committed r254454: <https://trac.webkit.org/changeset/254454> All reviewed patches have been landed. Closing bug. 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 |