Bug 220504 – Two pages in the same process should not be able to play media stream backed video elements at the same time

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

Two pages in the same process should not be able to play media stream backed video elements at the same time

[youenn fablet]

Created attachment 417379 [details]
Patch

[Sam Weinig]

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.

[youenn fablet]

Created attachment 417449 [details]
Patch

[youenn fablet]

Created attachment 417451 [details]
Patch

[youenn fablet]

Created attachment 417452 [details]
Patch

[Kate Cheney]

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.

[Sam Weinig]

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.

[youenn fablet]

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

[Sam Weinig]

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

[youenn fablet]

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

[Sam Weinig]

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

[youenn fablet]

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

[youenn fablet]

> 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

[Radar WebKit Bug Importer]

<rdar://problem/73318054>

[youenn fablet]

Created attachment 417957 [details]
Rebasing

[youenn fablet]

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

[Eric Carlson]

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.

[youenn fablet]

Created attachment 417988 [details]
Patch for landing

[EWS]

Committed r271670: <https://trac.webkit.org/changeset/271670>

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