Bug 147633

Summary: Media Session: push paused state to the media session focus manager instead of polling
Product: WebKit Reporter: Matt Rajca <mrajca>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, conrad_shultz, eric.carlson, mrajca, ossy, peavo, thorton, webkit-bug-importer
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 145411    
Attachments:
Description Flags
Patch
none
Patch
none
Patch simon.fraser: review+

Description Matt Rajca 2015-08-04 10:59:19 PDT 
Follow up from Bug 147588
Comment 1 Matt Rajca 2015-08-05 00:19:59 PDT 
Created attachment 258269 [details]
Patch
Comment 2 Matt Rajca 2015-08-05 09:24:36 PDT 
Created attachment 258277 [details]
Patch
Comment 3 Eric Carlson 2015-08-05 10:17:47 PDT 
Comment on attachment 258277 [details]
Patch

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

This looks good modulo my minor comments, but I am now a WK2 reviewer so someone else will have to give the official r+.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:344
> +#if ENABLE(MEDIA_SESSION)
> +    document()->updateIsPlayingMedia(HTMLMediaElementUnknownID);
> +#else
> +    document()->updateIsPlayingMedia(0);
> +#endif

Nit: if you don't guard HTMLMediaElementUnknownID with "ENABLE(MEDIA_SESSION)" you won't need the #if here.

Also, the name "HTMLMediaElementUnknownID" is not quite correct because it isn't an HTMLMediaElement at all. Maybe HTMLMediaElementInvalidID?

> Source/WebCore/Modules/webaudio/AudioContext.cpp:1085
> +#if ENABLE(MEDIA_SESSION)
> +            strongThis->document()->updateIsPlayingMedia(HTMLMediaElementUnknownID);
> +#else
> +            strongThis->document()->updateIsPlayingMedia(0);
> +#endif

Ditto.

> Source/WebCore/dom/Document.cpp:3505
> +#if ENABLE(MEDIA_SESSION)
> +    updateIsPlayingMedia(HTMLMediaElementUnknownID);
> +#else
> +    updateIsPlayingMedia(0);
> +#endif

Ditto.

> Source/WebCore/dom/Document.cpp:3516
> +#if ENABLE(MEDIA_SESSION)
> +    updateIsPlayingMedia(HTMLMediaElementUnknownID);
> +#else
> +    updateIsPlayingMedia(0);
> +#endif

Ditto.

> Source/WebCore/dom/Document.cpp:3529
> +    if (HTMLMediaElement* sourceElement = HTMLMediaElement::elementWithID(sourceElementID)) {
> +        if (sourceElement->isPlaying())
> +            state |= MediaProducer::IsSourcePlaying;
> +    }

Nit: you can prevent the failed lookup by checking for HTMLMediaElementUnknownID (or HTMLMediaElementInvalidID, or whatever you use).

> Source/WebCore/page/MediaProducer.h:43
> +#if ENABLE(MEDIA_SESSION)
> +        IsSourcePlaying = 1 << 6,
> +#endif

I don't think you need to #if this flag.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:5947
> +    if (focusManager->focusedMediaElementPage() == this && focusManager->focusedMediaElementID() == sourceElementID)
> +        focusManager->setFocusedMediaElementIsPlaying(state & MediaProducer::IsSourcePlaying);

This seems like the wrong place for this logic. Why not pass the element ID into the focus manager so you can keep all of the focus logic there.
Comment 4 Matt Rajca 2015-08-05 11:02:57 PDT 
(In reply to comment #3)
> Comment on attachment 258277 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258277&action=review
> 
> This looks good modulo my minor comments, but I am now a WK2 reviewer so
> someone else will have to give the official r+.
> 
> > Source/WebCore/Modules/webaudio/AudioContext.cpp:344
> > +#if ENABLE(MEDIA_SESSION)
> > +    document()->updateIsPlayingMedia(HTMLMediaElementUnknownID);
> > +#else
> > +    document()->updateIsPlayingMedia(0);
> > +#endif
> 
> Nit: if you don't guard HTMLMediaElementUnknownID with
> "ENABLE(MEDIA_SESSION)" you won't need the #if here.

Removed it to simplify subsequent code (1).

> 
> Also, the name "HTMLMediaElementUnknownID" is not quite correct because it
> isn't an HTMLMediaElement at all. Maybe HTMLMediaElementInvalidID?

Renamed.

> 
> > Source/WebCore/Modules/webaudio/AudioContext.cpp:1085
> > +#if ENABLE(MEDIA_SESSION)
> > +            strongThis->document()->updateIsPlayingMedia(HTMLMediaElementUnknownID);
> > +#else
> > +            strongThis->document()->updateIsPlayingMedia(0);
> > +#endif
> 
> Ditto.

Simplified this after fixing (1).

> 
> > Source/WebCore/dom/Document.cpp:3505
> > +#if ENABLE(MEDIA_SESSION)
> > +    updateIsPlayingMedia(HTMLMediaElementUnknownID);
> > +#else
> > +    updateIsPlayingMedia(0);
> > +#endif
> 
> Ditto.

Simplified this after fixing (1).

> 
> > Source/WebCore/dom/Document.cpp:3516
> > +#if ENABLE(MEDIA_SESSION)
> > +    updateIsPlayingMedia(HTMLMediaElementUnknownID);
> > +#else
> > +    updateIsPlayingMedia(0);
> > +#endif
> 
> Ditto.

Simplified this after fixing (1).

> 
> > Source/WebCore/dom/Document.cpp:3529
> > +    if (HTMLMediaElement* sourceElement = HTMLMediaElement::elementWithID(sourceElementID)) {
> > +        if (sourceElement->isPlaying())
> > +            state |= MediaProducer::IsSourcePlaying;
> > +    }
> 
> Nit: you can prevent the failed lookup by checking for
> HTMLMediaElementUnknownID (or HTMLMediaElementInvalidID, or whatever you
> use).

I prefer just checking if the pointer is null (even if it's logically equivalent to checking for an invalid ID). Feels safer.

> 
> > Source/WebCore/page/MediaProducer.h:43
> > +#if ENABLE(MEDIA_SESSION)
> > +        IsSourcePlaying = 1 << 6,
> > +#endif
> 
> I don't think you need to #if this flag.

Removed.

> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:5947
> > +    if (focusManager->focusedMediaElementPage() == this && focusManager->focusedMediaElementID() == sourceElementID)
> > +        focusManager->setFocusedMediaElementIsPlaying(state & MediaProducer::IsSourcePlaying);
> 
> This seems like the wrong place for this logic. Why not pass the element ID
> into the focus manager so you can keep all of the focus logic there.

Refactored.
Comment 5 Matt Rajca 2015-08-05 12:03:05 PDT 
Created attachment 258291 [details]
Patch
Comment 6 Simon Fraser (smfr) 2015-08-05 16:34:41 PDT 
Comment on attachment 258291 [details]
Patch

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

> Source/WebCore/dom/Document.h:1263
> +    WEBCORE_EXPORT void updateIsPlayingMedia(uint64_t);

How does this work if more than one media element is playing?
Comment 7 Matt Rajca 2015-08-05 16:42:26 PDT 
(In reply to comment #6)
> Comment on attachment 258291 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258291&action=review
> 
> > Source/WebCore/dom/Document.h:1263
> > +    WEBCORE_EXPORT void updateIsPlayingMedia(uint64_t);
> 
> How does this work if more than one media element is playing?

Every time any media element changes its 'playing' state, that information propagates to the UI process. The UI process keeps track of the currently focused media element belonging to a "Content" Media Session (there can only be one at a time). If the element IDs match, we can cache the 'playing' state as the focused media element's playing state.
Comment 8 Simon Fraser (smfr) 2015-08-05 17:07:45 PDT 
Comment on attachment 258291 [details]
Patch

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

>>> Source/WebCore/dom/Document.h:1263
>>> +    WEBCORE_EXPORT void updateIsPlayingMedia(uint64_t);
>> 
>> How does this work if more than one media element is playing?
> 
> Every time any media element changes its 'playing' state, that information propagates to the UI process. The UI process keeps track of the currently focused media element belonging to a "Content" Media Session (there can only be one at a time). If the element IDs match, we can cache the 'playing' state as the focused media element's playing state.

Please make a typedef for this uint64_t (MediaElementID or whatever), and convert the current code to use it. You can do this in a separate patch.

Give this a default argument, so you don't have to specify HTMLMediaElementInvalidID everywhere.

> Source/WebCore/html/HTMLMediaElement.cpp:3177
> +#if ENABLE(MEDIA_SESSION)
> +        document().updateIsPlayingMedia(m_elementID);
> +#else
> +        document().updateIsPlayingMedia(0);
> +#endif

This #ifdef is confusing. Why can't you always pass m_elementID?

> Source/WebCore/html/HTMLMediaElement.cpp:4545
> +#if ENABLE(MEDIA_SESSION)
> +    document().updateIsPlayingMedia(m_elementID);
> +#else
> +    document().updateIsPlayingMedia(0);
> +#endif

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:4810
> +#if ENABLE(MEDIA_SESSION)
> +    document().updateIsPlayingMedia(m_elementID);
> +#else
> +    document().updateIsPlayingMedia(0);
> +#endif

Ditto.
Comment 9 Matt Rajca 2015-08-05 17:57:39 PDT 
(In reply to comment #8)
> Comment on attachment 258291 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258291&action=review
> 
> >>> Source/WebCore/dom/Document.h:1263
> >>> +    WEBCORE_EXPORT void updateIsPlayingMedia(uint64_t);
> >> 
> >> How does this work if more than one media element is playing?
> > 
> > Every time any media element changes its 'playing' state, that information propagates to the UI process. The UI process keeps track of the currently focused media element belonging to a "Content" Media Session (there can only be one at a time). If the element IDs match, we can cache the 'playing' state as the focused media element's playing state.
> 
> Please make a typedef for this uint64_t (MediaElementID or whatever), and
> convert the current code to use it. You can do this in a separate patch.

Filed Bug 147709.

> 
> Give this a default argument, so you don't have to specify
> HTMLMediaElementInvalidID everywhere.

Done.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:3177
> > +#if ENABLE(MEDIA_SESSION)
> > +        document().updateIsPlayingMedia(m_elementID);
> > +#else
> > +        document().updateIsPlayingMedia(0);
> > +#endif
> 
> This #ifdef is confusing. Why can't you always pass m_elementID?

Element IDs are only assigned under the Media Session feature flag. With the default argument added, though, the confusing 0 argument is now gone.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4545
> > +#if ENABLE(MEDIA_SESSION)
> > +    document().updateIsPlayingMedia(m_elementID);
> > +#else
> > +    document().updateIsPlayingMedia(0);
> > +#endif
> 
> Ditto.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4810
> > +#if ENABLE(MEDIA_SESSION)
> > +    document().updateIsPlayingMedia(m_elementID);
> > +#else
> > +    document().updateIsPlayingMedia(0);
> > +#endif
> 
> Ditto.
Comment 10 Matt Rajca 2015-08-06 00:16:30 PDT 
Committed r188030: <http://trac.webkit.org/changeset/188030>
Comment 11 Csaba Osztrogonác 2015-08-06 04:42:34 PDT 
(In reply to comment #10)
> Committed r188030: <http://trac.webkit.org/changeset/188030>

It broke the WinCairo build:

WebCore.lib(DOMAllInOne.cpp.obj) : error LNK2001: unresolved external symbol "unsigned __int64 const WebCore::HTMLMediaElementInvalidID" (?HTMLMediaElementInvalidID@WebCore@@3_KB)
Comment 12 Alex Christensen 2015-08-06 09:37:17 PDT 
HTMLMediaElementInvalidID is defined inside of ENABLE(VIDEO) in HTMLMediaElement.cpp and used in Document.h outside of ENABLE(VIDEO).
Fixed build in http://trac.webkit.org/changeset/188041
Comment 13 Matt Rajca 2015-08-06 09:48:21 PDT 
(In reply to comment #12)
> HTMLMediaElementInvalidID is defined inside of ENABLE(VIDEO) in
> HTMLMediaElement.cpp and used in Document.h outside of ENABLE(VIDEO).
> Fixed build in http://trac.webkit.org/changeset/188041

Thanks, I guess we have VIDEO enabled on all our build bots.