Bug 206209

Summary:

Expose video 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

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

205123

Attachments:

Description	Flags
Patch	none
Patch	none

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.