Bug 219227 – [Media In GPU Process][MSE] Add the support to forward initialization segment from the GPU Process to Web processes

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

[Media In GPU Process][MSE] Add the support to forward initialization segment from the GPU Process to Web processes

[Peng Liu]

Created attachment 414772 [details]
Patch

[Peng Liu]

Created attachment 414774 [details]
Fix build failures on the WPE port

[Peng Liu]

Created attachment 414784 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/71768350>

[Eric Carlson]

Comment on attachment 414784 [details]
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::RemoveRemoteAudioTrack(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::RemoveRemoteVideoTrack(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::RemoveRemoteTextTrack(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

[Peng Liu]

Created attachment 415158 [details]
Updated patch based on Eric's comments

[Peng Liu]

Comment on attachment 414784 [details]
Patch

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

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:504
>> +    m_webProcessConnection->send(Messages::MediaPlayerPrivateRemote::RemoveRemoteAudioTrack(audioTrack->identifier()), m_id);
> 
> This will now crash in a release build if the track is not in the map.

Good point! Added a protection here.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:517
>> +    m_webProcessConnection->send(Messages::MediaPlayerPrivateRemote::RemoveRemoteVideoTrack(videoTrack->identifier()), m_id);
> 
> Ditto

Fixed.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:530
>> +    m_webProcessConnection->send(Messages::MediaPlayerPrivateRemote::RemoveRemoteTextTrack(textTrack->identifier()), m_id);
> 
> Ditto

Fixed.

>> Source/WebKit/GPUProcess/media/RemoteSourceBufferProxy.cpp:74
>> +        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&.

Agree. Fixed.

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

Fixed.

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

Fixed.

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

Fixed by adding a static create() function in RemoteMediaDescription.

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

Fixed.

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

Fixed.

[Peng Liu]

(In reply to Eric Carlson from comment #5)
> Comment on attachment 414784 [details]
> 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.
> 
Agree!

[EWS]

Committed r270334: <https://trac.webkit.org/changeset/270334>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415158 [details].