Two pages in the same process should not be able to play media stream backed video elements at the same time
Created attachment 417379 [details] Patch
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.
Created attachment 417449 [details] Patch
Created attachment 417451 [details] Patch
Created attachment 417452 [details] Patch
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 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.
(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.
(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?
> 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.
(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.
(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.
> 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
<rdar://problem/73318054>
Created attachment 417957 [details] Rebasing
> > 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 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.
Created attachment 417988 [details] Patch for landing
Committed r271670: <https://trac.webkit.org/changeset/271670> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417988 [details].