Bug 206209 - Expose video tracks for media files in the GPUProcess
Summary: Expose video 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: 205123
  Show dependency treegraph
 
Reported: 2020-01-13 18:17 PST by Eric Carlson
Modified: 2020-01-14 00:16 PST (History)
8 users (show)

See Also:


Attachments
Patch (44.61 KB, patch)
2020-01-13 18:34 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (45.99 KB, patch)
2020-01-13 20:47 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-13 18:17:45 PST
Expose video tracks for media files in the GPUProcess
Comment 1 Radar WebKit Bug Importer 2020-01-13 18:17:55 PST
<rdar://problem/58553026>
Comment 2 Eric Carlson 2020-01-13 18:34:55 PST
Created attachment 387605 [details]
Patch
Comment 3 Jer Noble 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.
Comment 4 Eric Carlson 2020-01-13 20:47:34 PST
Created attachment 387613 [details]
Patch
Comment 5 Eric Carlson 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
Comment 6 WebKit Commit Bot 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>
Comment 7 WebKit Commit Bot 2020-01-14 00:16:07 PST
All reviewed patches have been landed.  Closing bug.