Bug 206209

Summary: Expose video 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
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 205123    
Attachments:
Description Flags
Patch
none
Patch none

Eric Carlson
Reported 2020-01-13 18:17:45 PST
Expose video tracks for media files in the GPUProcess
Attachments
Patch (44.61 KB, patch)
2020-01-13 18:34 PST, Eric Carlson 		no flags
DetailsFormatted DiffDiff
Patch (45.99 KB, patch)
2020-01-13 20:47 PST, Eric Carlson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-01-13 18:17:55 PST
<rdar://problem/58553026>
Eric Carlson
Comment 2 2020-01-13 18:34:55 PST
Created attachment 387605 [details] Patch
Jer Noble
Comment 3 2020-01-13 20:35:49 PST
Comment on attachment 387605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387605&action=review r=me with a nit and a couple fixes. > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:423 > + ASSERT(!m_videoTracks.contains(identifier)); > + if (m_videoTracks.contains(identifier)) > + return; > + > + auto track = VideoTrackPrivateRemote::create(*this, identifier, WTFMove(configuration)); > + m_player->addVideoTrack(m_videoTracks.add(identifier, WTFMove(track)).iterator->value); Nit: This could be made ever so slightly more efficient by doing something like: auto addResult = m_videoTracks.ensure(identifier, [&] { return VideoTrackPrivateRemote::create(*this, identifier, WTFMove(configuration)); }); ASSERT(addResult.isNewEntry); if (!addResult.isNewEntry) return; m_player->addVideoTrack(addResult.iterator->value); > Source/WebKit/WebProcess/GPU/media/VideoTrackPrivateRemote.cpp:65 > + client()->idChanged(m_label); labelChanged(m_label)? > Source/WebKit/WebProcess/GPU/media/VideoTrackPrivateRemote.cpp:72 > + client()->idChanged(m_language); languageChanged(m_language)? > Source/WebKit/WebProcess/GPU/media/VideoTrackPrivateRemote.h:67 > + int m_trackIndex { -1 }; Is it too late to use Optional<int> here? Probably, but it'd be a nice clean up in a future patch.
Eric Carlson
Comment 4 2020-01-13 20:47:34 PST
Created attachment 387613 [details] Patch
Eric Carlson
Comment 5 2020-01-13 21:25:29 PST
Comment on attachment 387605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387605&action=review >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:423 >> + m_player->addVideoTrack(m_videoTracks.add(identifier, WTFMove(track)).iterator->value); > > Nit: This could be made ever so slightly more efficient by doing something like: > > auto addResult = m_videoTracks.ensure(identifier, [&] { return VideoTrackPrivateRemote::create(*this, identifier, WTFMove(configuration)); }); > ASSERT(addResult.isNewEntry); > if (!addResult.isNewEntry) > return; > m_player->addVideoTrack(addResult.iterator->value); Changed. >> Source/WebKit/WebProcess/GPU/media/VideoTrackPrivateRemote.cpp:65 >> + client()->idChanged(m_label); > > labelChanged(m_label)? Oops, changed. >> Source/WebKit/WebProcess/GPU/media/VideoTrackPrivateRemote.cpp:72 >> + client()->idChanged(m_language); > > languageChanged(m_language)? Ditto
WebKit Commit Bot
Comment 6 2020-01-14 00:16:05 PST
Comment on attachment 387613 [details] Patch Clearing flags on attachment: 387613 Committed r254499: <https://trac.webkit.org/changeset/254499>
WebKit Commit Bot
Comment 7 2020-01-14 00:16:07 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.