Bug 219227 - [Media In GPU Process][MSE] Add the support to forward initialization segment from the GPU Process to Web processes
Summary: [Media In GPU Process][MSE] Add the support to forward initialization segment...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-20 14:45 PST by Peng Liu
Modified: 2020-12-01 17:45 PST (History)
11 users (show)

See Also:


Attachments
Patch (111.51 KB, patch)
2020-11-21 16:31 PST, Peng Liu
no flags Details | Formatted Diff | Diff
Fix build failures on the WPE port (112.70 KB, patch)
2020-11-21 17:03 PST, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (112.23 KB, patch)
2020-11-21 22:03 PST, Peng Liu
eric.carlson: review+
Details | Formatted Diff | Diff
Updated patch based on Eric's comments (112.69 KB, patch)
2020-12-01 13:50 PST, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2020-11-20 14:45:39 PST
[Media In GPU Process][MSE] Add the support to forward initialization segment from the GPU Process to Web processes
Comment 1 Peng Liu 2020-11-21 16:31:12 PST
Created attachment 414772 [details]
Patch
Comment 2 Peng Liu 2020-11-21 17:03:41 PST
Created attachment 414774 [details]
Fix build failures on the WPE port
Comment 3 Peng Liu 2020-11-21 22:03:40 PST
Created attachment 414784 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2020-11-27 14:46:17 PST
<rdar://problem/71768350>
Comment 5 Eric Carlson 2020-12-01 10:34:28 PST
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
Comment 6 Peng Liu 2020-12-01 13:50:03 PST
Created attachment 415158 [details]
Updated patch based on Eric's comments
Comment 7 Peng Liu 2020-12-01 14:13:51 PST
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.
Comment 8 Peng Liu 2020-12-01 14:16:56 PST
(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!
Comment 9 EWS 2020-12-01 17:45:09 PST
Committed r270334: <https://trac.webkit.org/changeset/270334>

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