Bug 137218 – Implement API in WebCore::Page that returns whether there's any audio playing on the page

• Component: Media
• Status: RESOLVED FIXED

[Ada Chan]

This bug focuses on getting the HTMLMediaElement related parts working.  I'll file a separate bug for handling the plug-ins case.

Here's a rough plan on implementing this:

1. Add a method in MediaSessionClient to return which Page it belongs to.
2. Add a method in MediaSessionClient that returns its "media characteristics".  Media characteristics can be a combination of Audible, Visual, and Legible.
3. Maintain a map of Page -> list of MediaSessions in MediaSessionManager.
4. Add a method in MediaSessionManager that returns whether there's audio playing for a particular Page.  That method will iterate through the list of MediaSessions for that page, and return true if any one of them is in the Playing state and has the Audible characteristic.

[Ada Chan]

Steps 3 and 4 have changed.  Instead of trying to maintain a map of Page -> MediaSessions, we'll keep a set of MediaSessions in the Document instead.  The Page can go through its frames' documents to check whether there are any MediaSessions with audio that are playing.

[Ada Chan]

Created attachment 239179 [details]
Patch: Implement WebCore::Page::isPlayingAudio()

[Eric Carlson]

Comment on attachment 239179 [details]
Patch: Implement WebCore::Page::isPlayingAudio()

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

> Source/WebCore/dom/Document.cpp:3293
> +    for (auto mediaSessionsIt = m_mediaSessions.begin(); mediaSessionsIt != m_mediaSessions.end(); ++mediaSessionsIt) {

Can you do this with a modern for loop?   for (auto mediaSession : m_mediaSessions)

> Source/WebCore/html/HTMLMediaElement.cpp:447
> +    document.registerMediaSession(*m_mediaSession);

I think it would make more sense to have the session register/unregister with the document. Maybe something like MediaSession::addedToDocument and MediaSession::removedFromDocument?

> Source/WebCore/html/HTMLMediaElement.cpp:472
> +    document.unregisterMediaSession(*m_mediaSession);

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:4326
> +    document().updateIsPlayingAudio();

The session should update the document. MediaSession::characteristicChanged ?

> Source/WebCore/html/HTMLMediaElement.cpp:6018
> +    document().updateIsPlayingAudio();

Ditto.

> Source/WebCore/html/HTMLMediaElement.h:706
> +    virtual void mediaStateDidChange() override;

This isn't necessary.

[Ada Chan]

Thanks for reviewing!

(In reply to comment #3)
> (From update of attachment 239179 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239179&action=review
> 
> > Source/WebCore/dom/Document.cpp:3293
> > +    for (auto mediaSessionsIt = m_mediaSessions.begin(); mediaSessionsIt != m_mediaSessions.end(); ++mediaSessionsIt) {
> 
> Can you do this with a modern for loop?   for (auto mediaSession : m_mediaSessions)

Yes, I can do that.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:447
> > +    document.registerMediaSession(*m_mediaSession);
> 
> I think it would make more sense to have the session register/unregister with the document. Maybe something like MediaSession::addedToDocument and MediaSession::removedFromDocument?

I actually had that before, but Jer reminded me that things in the platform layer should not know anything about the dom layer.  That's why I did it this way.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:472
> > +    document.unregisterMediaSession(*m_mediaSession);
> 
> Ditto.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4326
> > +    document().updateIsPlayingAudio();
> 
> The session should update the document. MediaSession::characteristicChanged ?

Same reason here.  The MediaSession shouldn't update the Document directly, so I did it through the MediaSessionClient.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:6018
> > +    document().updateIsPlayingAudio();
> 
> Ditto.
> 
> > Source/WebCore/html/HTMLMediaElement.h:706
> > +    virtual void mediaStateDidChange() override;
> 
> This isn't necessary.

[Ada Chan]

Eric, please let me know if you think there's another approach I can take to maintain the relationship between the Document and the MediaSessions that does not involve the MediaSession having direct knowledge of the Document.  Thanks!

[Eric Carlson]

(In reply to comment #5)
> Eric, please let me know if you think there's another approach I can take to maintain the relationship 
> between the Document and the MediaSessions that does not involve the MediaSession having 
> direct knowledge of the Document.  Thanks!

You could put it int HTMLMediaSession, but I fine with you leaving it as-is if you would rather.

[Ada Chan]

I've decided to leave it as is for now.  I still need to handle MediaSessions from AudioDestinations at some point and that doesn't use HTMLMediaSession.

Committed:
https://trac.webkit.org/changeset/174353

[Csaba Osztrogonác]

(In reply to comment #7)
> I've decided to leave it as is for now.  I still need to handle MediaSessions from AudioDestinations at some point and that doesn't use HTMLMediaSession.
> 
> Committed:
> https://trac.webkit.org/changeset/174353

It broke the !ENABLE(VIDEO) build, see bug137655 for details.