[webkit-reviews] review granted: [Bug 219227] [Media In GPU Process][MSE] Add the support to forward initialization segment from the GPU Process to Web processes : [Attachment 414784] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Peng Liu
<peng.liu6 at apple.com>'s request for review:
Bug 219227: [Media In GPU Process][MSE] Add the support to forward
initialization segment from the GPU Process to Web processes
https://bugs.webkit.org/show_bug.cgi?id=219227

Attachment 414784: Patch

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




--- Comment #5 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 414784
  --> https://bugs.webkit.org/attachment.cgi?id=414784
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414784&action=review

This would have been better as two patches, one for the remote track
cleanup/simplification, and another for the new initialization segment work.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:504
> +    ASSERT(m_audioTracks.contains(&track));
> +    auto audioTrack = m_audioTracks.get(&track);
> +   
m_webProcessConnection->send(Messages::MediaPlayerPrivateRemote::RemoveRemoteAu
dioTrack(audioTrack->identifier()), m_id);

This will now crash in a release build if the track is not in the map.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:517
> +    ASSERT(m_videoTracks.contains(&track));
> +    auto videoTrack = m_videoTracks.get(&track);
> +   
m_webProcessConnection->send(Messages::MediaPlayerPrivateRemote::RemoveRemoteVi
deoTrack(videoTrack->identifier()), m_id);

Ditto

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:530
>      ASSERT(m_textTracks.contains(&track));
> +    auto textTrack = m_textTracks.get(&track);
> +   
m_webProcessConnection->send(Messages::MediaPlayerPrivateRemote::RemoveRemoteTe
xtTrack(textTrack->identifier()), m_id);

Ditto

> Source/WebKit/GPUProcess/media/RemoteSourceBufferProxy.cpp:74
> +	   auto description = audioTrackInfo.description;
> +	   segmentInfo.audioTracks.append({ { description->codec(),
description->isVideo(), description->isAudio(), description->isText() },
identifier });

This would be clearer/cleaner if you defined a MediaDescriptionInfo constructor
that took a MediaDescription&.

> Source/WebKit/GPUProcess/media/RemoteSourceBufferProxy.cpp:80
> +	   auto description = videoTrackInfo.description;
> +	   segmentInfo.videoTracks.append({ { description->codec(),
description->isVideo(), description->isAudio(), description->isText() },
identifier });

Ditto

> Source/WebKit/GPUProcess/media/RemoteSourceBufferProxy.cpp:86
> +	   auto description = textTrackInfo.description;
> +	   segmentInfo.textTracks.append({ { description->codec(),
description->isVideo(), description->isAudio(), description->isText() },
identifier });

Ditto

> Source/WebKit/WebProcess/GPU/media/SourceBufferPrivateRemote.cpp:170
> +	   info.description = adoptRef(*new
RemoteMediaDescription(audioTrack.description));

`RemoteMediaDescription::create(track.description)` or
`RemoteMediaDescription::create(track)` would be more idiomatic

> Source/WebKit/WebProcess/GPU/media/SourceBufferPrivateRemote.cpp:177
> +	   info.description = adoptRef(*new
RemoteMediaDescription(videoTrack.description));

Ditto

> Source/WebKit/WebProcess/GPU/media/SourceBufferPrivateRemote.cpp:184
> +	   info.description = adoptRef(*new
RemoteMediaDescription(textTrack.description));

Ditto