Bug 144009 – [Mac] Use one playback target for all web processes

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

All web processes should share the same MediaPlaybackTarget.

[Eric Carlson]

Created attachment 251283 [details]
Proposed patch

[WebKit Commit Bot]

Attachment 251283 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/MediaPlaybackTargetClient.h:26:  #ifndef header guard has wrong style, please use: MediaPlaybackTargetClient_h  [build/header_guard] [5]
Total errors found: 1 in 60 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

<rdar://problem/20644744>

[Simon Fraser (smfr)]

Comment on attachment 251283 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=144009

Is there an associated radar?

> Source/WebCore/ChangeLog:15
> +        Instead of having each Page/Document pair manage access to the playback target for the videos
> +        in a web process, put all of the logic into a new class - WebMediaSessionManager. A singleton
> +        instance talks to the target picker and manages video element access for all web processes.
> +        All playback target logic was removed from Document, Page, and MediaSessionManager.

What's the plan for testing this stuff?

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:44
> +    uint64_t m_contextId { 0 };

Can we use a typedef rather than uint64_t?

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:49
> +static bool flagsAreSet(unsigned& value, unsigned flags)

Why is value a reference? Can value be a known type?

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:51
> +    return !!(value & flags);

C++ does the implicit bool conversion for you.

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:55
> +    : m_taskTimer(RunLoop::current(), this, &WebMediaSessionManager::taskTimerFired)

One per line please.

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:61
> +    m_taskTimer.stop();

Timers stop themselves on destruction.

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:63
> +    m_taskQueue.clear();
> +    m_clientState.clear();

Not sure these are required either.

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:126
> +    for (size_t i = 0; i < m_clientState.size(); ++i)

Use a modern loop.

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:153
> +        if (state->m_contextId == contextId && state->m_client == &client)

Give state have an operator ==?

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:181
> +    for (size_t i = 0; i < m_clientState.size(); ++i) {
> +        auto& state = m_clientState[i];

Modern loop.

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:246
> +    for (size_t i = 0; i < m_clientState.size(); ++i) {
> +        if (m_clientState[i]->m_contextId == contextId && m_clientState[i]->m_client == client)
> +            return i;
> +    }

Linear search :(. Is it worth hashing by context ID?

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.h:52
> +    WEBCORE_EXPORT uint64_t addPlaybackTargetPickerClient(WebMediaSessionManagerClient&, uint64_t);
> +    WEBCORE_EXPORT void removePlaybackTargetPickerClient(WebMediaSessionManagerClient&, uint64_t);
> +    WEBCORE_EXPORT void removeAllPlaybackTargetPickerClients(WebMediaSessionManagerClient&);
> +    WEBCORE_EXPORT void showPlaybackTargetPicker(WebMediaSessionManagerClient&, uint64_t, const IntRect&, bool);
> +    WEBCORE_EXPORT void clientStateDidChange(WebMediaSessionManagerClient&, uint64_t, WebCore::MediaProducer::MediaStateFlags);

That uint64_t really needs a typedef.

> Source/WebCore/dom/Document.cpp:6607
> +    if (!m_idToClientMap.contains(clientId))
> +        return;
>  
> -#endif
> +    auto* client = m_idToClientMap.get(clientId);

That's two hash lookups. You could use find().

> Source/WebCore/dom/Document.h:1693
> +    HashMap<uint64_t, WebCore::MediaPlaybackTargetClient*> m_idToClientMap;
> +    HashMap<WebCore::MediaPlaybackTargetClient*, uint64_t> m_clientToIDMap;

It's a bit odd that you need maps in both directions.

> Source/WebCore/platform/audio/MediaSessionManager.h:44
> +

No empty line.

> Source/WebKit2/UIProcess/mac/WebMediaSessionManagerMac.h:35
> +class WebMediaSessionManagerMac : public WebMediaSessionManager {

Odd that this class exists both in WebCore/platform/graphics/avfoundation and here?

> Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:267
> -        function();
> +

What happened here?

[Tim Horton]

Comment on attachment 251283 [details]
Proposed patch

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

Please try to not break *all* the bots :D

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:25
> +#include "config.h"

newline above here

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:36
> +struct ClientState {

I think we use non-m_ prefixed member names for structs? Should this be a class, or should the member names change, or is that just a bad style in general? I don't know.

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:45
> +    WebCore::MediaProducer::MediaStateFlags m_flags { WebCore::MediaProducer::IsNotPlaying};

space before the closing curly brace

> Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:77
> +        TaskCallback callback = std::make_tuple(&client, contextId, [this](ClientState& state) {

Is it safe to capture "this" here? Is it safe because the callback is kept and called from this object as well?

> Source/WebCore/html/HTMLMediaSession.cpp:391
> +#if PLATFORM(IOS)
> +#endif

What.

> Source/WebCore/page/MediaProducer.h:38
> +        RequirePlaybackTargetMonitoring = 1 << 3,

I think the part-of-speech of this one is different than the others but I don't know how English works.

> Source/WebCore/platform/graphics/avfoundation/WebMediaSessionManagerMac.cpp:25
> +#include "config.h"

newline above

> Source/WebKit2/UIProcess/PageClient.h:53
> +class WebMediaSessionManager;

I think these are sorted including the type (so this should be up above Highlight).

> Source/WebKit/mac/WebView/WebView.mm:296
> +#import <WebCore/WebMediaSessionManagerMac.h>
>  #import "WebMediaPlaybackTargetPicker.h"

swap the lines, I think?

[Eric Carlson]

(In reply to comment #4)
> Comment on attachment 251283 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251283&action=review
> 
> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:44
> > +    uint64_t m_contextId { 0 };
> 
> Can we use a typedef rather than uint64_t?
> 

I could, but frequently use uint64_t for context IDs, eg. page ID, frame ID, message ID, navigation ID, back/forward item ID, drag operation, touch tap ID, etc, etc. 

> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:49
> > +static bool flagsAreSet(unsigned& value, unsigned flags)
> 
> Why is value a reference? Can value be a known type?
> 

Shouldn't be a reference, fixed.

> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:51
> > +    return !!(value & flags);
> 
> C++ does the implicit bool conversion for you.
> 

Fixed.

> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:55
> > +    : m_taskTimer(RunLoop::current(), this, &WebMediaSessionManager::taskTimerFired)
> 
> One per line please.
> 

This is one line, the RunLoop::Timer constructor takes three arguments.

> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:61
> > +    m_taskTimer.stop();
> 
> Timers stop themselves on destruction.
> 
> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:63
> > +    m_taskQueue.clear();
> > +    m_clientState.clear();
> 
> Not sure these are required either.
> 

Fixed.

> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:126
> > +    for (size_t i = 0; i < m_clientState.size(); ++i)
> 
> Use a modern loop.
> 

Fixed.

> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:153
> > +        if (state->m_contextId == contextId && state->m_client == &client)
> 
> Give state have an operator ==?
> 

It isn't checking object equality, only that the client and context ID are the same.

> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:181
> > +    for (size_t i = 0; i < m_clientState.size(); ++i) {
> > +        auto& state = m_clientState[i];
> 
> Modern loop.
> 

This can't be a modern loop, we save off an index.

> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:246
> > +    for (size_t i = 0; i < m_clientState.size(); ++i) {
> > +        if (m_clientState[i]->m_contextId == contextId && m_clientState[i]->m_client == client)
> > +            return i;
> > +    }
> 
> Linear search :(. Is it worth hashing by context ID?
> 

Context ID is only unique per web process, so the lookup has to consider both context ID and client.

> > Source/WebCore/dom/Document.cpp:6607
> > +    if (!m_idToClientMap.contains(clientId))
> > +        return;
> >  
> > -#endif
> > +    auto* client = m_idToClientMap.get(clientId);
> 
> That's two hash lookups. You could use find().
> 

Fixed.

> > Source/WebCore/dom/Document.h:1693
> > +    HashMap<uint64_t, WebCore::MediaPlaybackTargetClient*> m_idToClientMap;
> > +    HashMap<WebCore::MediaPlaybackTargetClient*, uint64_t> m_clientToIDMap;
> 
> It's a bit odd that you need maps in both directions.
> 

We have to map from client interface to and from context ID.

> > Source/WebCore/platform/audio/MediaSessionManager.h:44
> > +
> 
> No empty line.
> 

Fixed.

> > Source/WebKit2/UIProcess/mac/WebMediaSessionManagerMac.h:35
> > +class WebMediaSessionManagerMac : public WebMediaSessionManager {
> 
> Odd that this class exists both in WebCore/platform/graphics/avfoundation
> and here?
> 

I moved it to WebCore so it can be used by WK1 and forgot to delete this one. Fixed.

> > Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:267
> > -        function();
> > +
> 
> What happened here?

Badness, fixed!

[Eric Carlson]

(In reply to comment #5)
> Comment on attachment 251283 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251283&action=review
> 
> Please try to not break *all* the bots :D
> 

Oh, you are no fun at all!

> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:25
> > +#include "config.h"
> 
> newline above here
> 

Fixed.

> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:36
> > +struct ClientState {
> 
> I think we use non-m_ prefixed member names for structs? Should this be a
> class, or should the member names change, or is that just a bad style in
> general? I don't know.
> 

We do seem to mostly not use m_, fixed.

> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:45
> > +    WebCore::MediaProducer::MediaStateFlags m_flags { WebCore::MediaProducer::IsNotPlaying};
> 
> space before the closing curly brace
> 
> > Source/WebCore/Modules/mediasession/WebMediaSessionManager.cpp:77
> > +        TaskCallback callback = std::make_tuple(&client, contextId, [this](ClientState& state) {
> 
> Is it safe to capture "this" here? Is it safe because the callback is kept
> and called from this object as well?
> 

That and WebMediaSessionManager is a NeverDestroyed singleton.

> > Source/WebCore/html/HTMLMediaSession.cpp:391
> > +#if PLATFORM(IOS)
> > +#endif
> 
> What.
> 

What indeed. Fixed.

> > Source/WebCore/page/MediaProducer.h:38
> > +        RequirePlaybackTargetMonitoring = 1 << 3,
> 
> I think the part-of-speech of this one is different than the others but I
> don't know how English works.
> 

Fixed.

> > Source/WebCore/platform/graphics/avfoundation/WebMediaSessionManagerMac.cpp:25
> > +#include "config.h"
> 
> newline above
> 
> > Source/WebKit2/UIProcess/PageClient.h:53
> > +class WebMediaSessionManager;
> 
> I think these are sorted including the type (so this should be up above
> Highlight).
> 

Fixed.

> > Source/WebKit/mac/WebView/WebView.mm:296
> > +#import <WebCore/WebMediaSessionManagerMac.h>
> >  #import "WebMediaPlaybackTargetPicker.h"
> 
> swap the lines, I think?

Fixed.

[Eric Carlson]

Created attachment 251297 [details]
Patch for the bots.

[Eric Carlson]

Created attachment 251298 [details]
Patch for the bots.

[WebKit Commit Bot]

Comment on attachment 251298 [details]
Patch for the bots.

Clearing flags on attachment: 251298

Committed r183096: <http://trac.webkit.org/changeset/183096>