Bug 137218 - Implement API in WebCore::Page that returns whether there's any audio playing on the page
Summary: Implement API in WebCore::Page that returns whether there's any audio playing...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ada Chan
URL:
Keywords:
Depends on: 137655
Blocks: 137048
  Show dependency treegraph
 
Reported: 2014-09-29 11:19 PDT by Ada Chan
Modified: 2014-10-13 03:42 PDT (History)
11 users (show)

See Also:


Attachments
Patch: Implement WebCore::Page::isPlayingAudio() (18.39 KB, patch)
2014-10-02 22:47 PDT, Ada Chan
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ada Chan 2014-09-29 11:19:25 PDT
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.
Comment 1 Ada Chan 2014-10-02 20:05:34 PDT
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.
Comment 2 Ada Chan 2014-10-02 22:47:58 PDT
Created attachment 239179 [details]
Patch: Implement WebCore::Page::isPlayingAudio()
Comment 3 Eric Carlson 2014-10-03 07:42:39 PDT
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.
Comment 4 Ada Chan 2014-10-03 08:14:10 PDT
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.
Comment 5 Ada Chan 2014-10-03 08:28:39 PDT
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!
Comment 6 Eric Carlson 2014-10-03 18:12:25 PDT
(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.
Comment 7 Ada Chan 2014-10-06 11:34:26 PDT
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
Comment 8 Csaba Osztrogonác 2014-10-13 03:42:39 PDT
(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.