Bug 220504

Summary: Two pages in the same process should not be able to play media stream backed video elements at the same time
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, katherine_cheney, philipj, ryuan.choi, sam, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 199661    
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Rebasing
none
Patch for landing none

Description youenn fablet 2021-01-11 03:25:44 PST
Two pages in the same process should not be able to play media stream backed video elements at the same time
Comment 1 youenn fablet 2021-01-11 08:21:42 PST
Created attachment 417379 [details]
Patch
Comment 2 Sam Weinig 2021-01-11 13:55:02 PST
Comment on attachment 417379 [details]
Patch

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

> Source/WebCore/platform/audio/PlatformMediaSession.h:30
> +#include "PageIdentifier.h"

This is a layering violation. Please don't use concepts like PageIdentifier (that live outside of platform)  in the platform/ directory.
Comment 3 youenn fablet 2021-01-12 05:39:44 PST
Created attachment 417449 [details]
Patch
Comment 4 youenn fablet 2021-01-12 06:20:01 PST
Created attachment 417451 [details]
Patch
Comment 5 youenn fablet 2021-01-12 06:29:00 PST
Created attachment 417452 [details]
Patch
Comment 6 Kate Cheney 2021-01-12 12:22:15 PST
Comment on attachment 417452 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:201
> +    Optional<PageIdentifier> hostingPageIdentifier() const final {return document().pageID(); }

nit: missing a space before return.

> Source/WebCore/platform/PageIdentifier.h:2
> + * Copyright (C) 2019 Apple Inc. All rights reserved.

2021?

> Source/WebCore/platform/audio/PlatformMediaSession.cpp:353
> +    return mediaType == MediaElementSession::MediaType::VideoAudio || mediaType == MediaElementSession::MediaType::Audio;

Does this need to consider WebAudio at all?

> Source/WebCore/platform/audio/PlatformMediaSessionManager.cpp:-248
> -                && oneSession.mediaType() == sessionType

I am curious about why this can be removed.
Comment 7 Sam Weinig 2021-01-12 13:58:47 PST
Comment on attachment 417452 [details]
Patch

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

> Source/WebCore/platform/PageIdentifier.h:33
> +enum PageIdentifierType { };
> +using PageIdentifier = ObjectIdentifier<PageIdentifierType>;

This is still a layering violation. Moving the concept of PageIdentifer to platform really doesn't make any sense. Page is a WebCore concept, platforms don't have such a concept.
Comment 8 youenn fablet 2021-01-13 08:01:50 PST
(In reply to Sam Weinig from comment #7)
> Comment on attachment 417452 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417452&action=review
> 
> > Source/WebCore/platform/PageIdentifier.h:33
> > +enum PageIdentifierType { };
> > +using PageIdentifier = ObjectIdentifier<PageIdentifierType>;
> 
> This is still a layering violation. Moving the concept of PageIdentifer to
> platform really doesn't make any sense. Page is a WebCore concept, platforms
> don't have such a concept.

PlatformMediaSession and PlatformMediaSessionManager have been using the concept of a Document since a long time, as a way to group media sessions.
It is unclear why a Document would be fine but not a a Page.
Or maybe PlatformMediaSession/Manager should be refactored to move some code out of platform.

I can continue using DocumentIdentifier as a first step, this should be functionally an improvement.
Comment 9 Sam Weinig 2021-01-13 10:05:07 PST
(In reply to youenn fablet from comment #8)
> (In reply to Sam Weinig from comment #7)
> > Comment on attachment 417452 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=417452&action=review
> > 
> > > Source/WebCore/platform/PageIdentifier.h:33
> > > +enum PageIdentifierType { };
> > > +using PageIdentifier = ObjectIdentifier<PageIdentifierType>;
> > 
> > This is still a layering violation. Moving the concept of PageIdentifer to
> > platform really doesn't make any sense. Page is a WebCore concept, platforms
> > don't have such a concept.
> 
> PlatformMediaSession and PlatformMediaSessionManager have been using the
> concept of a Document since a long time, as a way to group media sessions.
> It is unclear why a Document would be fine but not a a Page.
> Or maybe PlatformMediaSession/Manager should be refactored to move some code
> out of platform.
> 
> I can continue using DocumentIdentifier as a first step, this should be
> functionally an improvement.

DocumentIdentifer is also not ok to use in platform code. That should definitely be resolved as well. Why is a concept like document or page needed? Surely the underlying platform code doesn't know about such things, so what is it being used for?
Comment 10 youenn fablet 2021-01-13 10:51:39 PST
> DocumentIdentifer is also not ok to use in platform code. That should
> definitely be resolved as well. Why is a concept like document or page
> needed? Surely the underlying platform code doesn't know about such things,
> so what is it being used for?

To do processing on the group of sessions owned by a Document.
We can try to remove some of the uses by having the Document owning the group of sessions.
Comment 11 Sam Weinig 2021-01-13 13:39:45 PST
(In reply to youenn fablet from comment #10)
> > DocumentIdentifer is also not ok to use in platform code. That should
> > definitely be resolved as well. Why is a concept like document or page
> > needed? Surely the underlying platform code doesn't know about such things,
> > so what is it being used for?
> 
> To do processing on the group of sessions owned by a Document.
> We can try to remove some of the uses by having the Document owning the
> group of sessions.

Another pattern that can work is to expose a new type, that represents the group and have Document and/or Page implement that type.
Comment 12 youenn fablet 2021-01-14 08:23:34 PST
(In reply to Sam Weinig from comment #11)
> (In reply to youenn fablet from comment #10)
> > > DocumentIdentifer is also not ok to use in platform code. That should
> > > definitely be resolved as well. Why is a concept like document or page
> > > needed? Surely the underlying platform code doesn't know about such things,
> > > so what is it being used for?
> > 
> > To do processing on the group of sessions owned by a Document.
> > We can try to remove some of the uses by having the Document owning the
> > group of sessions.
> 
> Another pattern that can work is to expose a new type, that represents the
> group and have Document and/or Page implement that type.

Looking through the use of DocumentIdentifier, it seems the pattern is mostly (essentially) to go from the Page to all its document and fo each document to all its sessions.
We might be able to do some simplification here.
Comment 13 youenn fablet 2021-01-18 01:55:51 PST
> Looking through the use of DocumentIdentifier, it seems the pattern is
> mostly (essentially) to go from the Page to all its document and fo each
> document to all its sessions.
> We might be able to do some simplification here.

Continuing work at https://bugs.webkit.org/show_bug.cgi?id=220706
Comment 14 Radar WebKit Bug Importer 2021-01-18 03:26:13 PST
<rdar://problem/73318054>
Comment 15 youenn fablet 2021-01-20 02:41:01 PST
Created attachment 417957 [details]
Rebasing
Comment 16 youenn fablet 2021-01-20 02:43:32 PST
> > Source/WebCore/platform/audio/PlatformMediaSession.cpp:353
> > +    return mediaType == MediaElementSession::MediaType::VideoAudio || mediaType == MediaElementSession::MediaType::Audio;
> 
> Does this need to consider WebAudio at all?

This check replaces the oneSession.mediaType() == sessionType.
It is more lax in the sense that a video element that is audio only should probably be able to interrupt a video element that is audio/video.
For track-based media elements, this can change very dynamically.

> > Source/WebCore/platform/audio/PlatformMediaSessionManager.cpp:-248
> > -                && oneSession.mediaType() == sessionType
> 
> I am curious about why this can be removed.

Replaced by the new session type related check in canPlayConcurrently.
Comment 17 Eric Carlson 2021-01-20 09:22:54 PST
Comment on attachment 417957 [details]
Rebasing

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

> Source/WebCore/ChangeLog:9
> +        Add hostingPageIdentifier getter for that purpose.

This comment should be updated.
Comment 18 youenn fablet 2021-01-20 12:47:17 PST
Created attachment 417988 [details]
Patch for landing
Comment 19 EWS 2021-01-20 13:27:12 PST
Committed r271670: <https://trac.webkit.org/changeset/271670>

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