[BlackBerry] Tab awareness for HTML5 concurrent audio

• Component: WebKit BlackBerry
• Status: CLOSED FIXED

[Max Feil]

This bug is for support of concurrent HTML5 audio improvements being made in the platform library, which require awareness of tabs and tab visibility. Each MediaPlayerPrivate object will be passed a unique tab identifier (based on the current platformPageClient) when it gets created. Each MediaPlayerPrivate object will also be given the ability to query whether the tab it is on is "visible", meaning "currently selected".

[Max Feil]

For more information on the BlackBerry side of things, see PR96004.

[Max Feil]

Created attachment 135166 [details]
Patch

[Max Feil]

I'm wondering if a test is needed for this patch. Since it involves tabs and tab visibility, I'm assuming that any test would need to be a BlackBerry-only manual test.

[Antonio Gomes]

could not the PAGE_VISIBILITY stuff we just enabled to BlackBerry port help somehow here?

[Andrew Lo]

(In reply to comment #4)
> could not the PAGE_VISIBILITY stuff we just enabled to BlackBerry port help somehow here?

Yeah the WebPagePrivate::setVisible property shows whether the tab is the selected one.

[Max Feil]

(In reply to comment #5)
> (In reply to comment #4)
> > could not the PAGE_VISIBILITY stuff we just enabled to BlackBerry port help somehow here?
> 
> Yeah the WebPagePrivate::setVisible property shows whether the tab is the selected one.

Note that, as described in comment #0, the intention of this patch is to let the platform media code know whether the tab it is on is the selected one. So I believe that PAGE_VISIBILITY does not apply here because it also seems to tie into the the activation state. Responding to the App's activation state is already handled for HTML5 media using a separate mechanism, and is not within the scope of this bug.

[Max Feil]

I will write a manual test in ManualTests/blackberry and I'm hoping that will be enough to give this bug an r+

[Antonio Gomes]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > could not the PAGE_VISIBILITY stuff we just enabled to BlackBerry port help somehow here?
> > 
> > Yeah the WebPagePrivate::setVisible property shows whether the tab is the selected one.
> 
> Note that, as described in comment #0, the intention of this patch is to let the platform media code know whether the tab it is on is the selected one. So I believe that PAGE_VISIBILITY does not apply here because it also seems to tie into the the activation state. Responding to the App's activation state is already handled for HTML5 media using a separate mechanism, and is not within the scope of this bug.

So the difference here is that from your "tab awareness" perspective, a tab can has to be the current selected tab, no matter if it is in thumbnail view mode, or the app itself is in background, etc. Whereas from a "page visibility" perspective, the tab (page) has to be the current selected one, and in active state, i.e. has focus and user can interact with it.


Does it sound correct?

[Andrew Lo]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > could not the PAGE_VISIBILITY stuff we just enabled to BlackBerry port help somehow here?
> > 
> > Yeah the WebPagePrivate::setVisible property shows whether the tab is the selected one.
> 
> Note that, as described in comment #0, the intention of this patch is to let the platform media code know whether the tab it is on is the selected one. So I believe that PAGE_VISIBILITY does not apply here because it also seems to tie into the the activation state. Responding to the App's activation state is already handled for HTML5 media using a separate mechanism, and is not within the scope of this bug.

PAGE_VISIBILITY takes both tab selection and application activation state into account.

However WebPage's visible property only takes tab selection into account, not activation state. Therefore it seems like you can use the latter.

[Max Feil]

(In reply to comment #8)
> So the difference here is that from your "tab awareness" perspective, a tab can has to be the current selected tab, no matter if it is in thumbnail view mode, or the app itself is in background, etc. Whereas from a "page visibility" perspective, the tab (page) has to be the current selected one, and in active state, i.e. has focus and user can interact with it.
> 
> 
> Does it sound correct?

Yes, that is what I'm trying to say. Only the tab selection matters.

[Max Feil]

Created attachment 135697 [details]
Patch

[Max Feil]

My new manual test is a modification of an existing one which has not been upstreamed yet, but you can see it here:

r? webkit/001b6c8

[Antonio Gomes]

Comment on attachment 135697 [details]
Patch

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

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:118
> +    void* tabId = 0;
> +    if (frameView() && frameView()->hostWindow())
> +        tabId = frameView()->hostWindow()->platformPageClient();
> +#if USE(ACCELERATED_COMPOSITING)
> +    m_platformPlayer = new MMRPlayer(this, tabId, true);
> +#else
> +    m_platformPlayer = new MMRPlayer(this, tabId, false);
> +#endif

could you help me to understand how the tabId is used here?

[Max Feil]

(In reply to comment #13)
> could you help me to understand how the tabId is used here?

The tabId is just a unique identifier that the MMRPlayer BlackBerryPlatform class uses to group media elements (aka players) by tab. It does not ever dereference the pointer. Media elements on the same tab will have the same tabId, media elements on different tabs will have different tabId's. That's all.

For details on how the MMRPlayer class does this grouping see my already reviewed patch: platform/982d560

[Max Feil]

Created attachment 137402 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 137402 [details]
Patch

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

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:728
> +    if (frameView() && frameView()->hostWindow())
> +        return frameView()->hostWindow()->platformPageClient()->isVisible();

It's a layering violation for code in platform/ to know about code in page/

[Eric Carlson]

Comment on attachment 137402 [details]
Patch

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

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:115
> +        tabId = frameView()->hostWindow()->platformPageClient();

This is a layering violation, code in WebCore/platform should know about code in /page.

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:697
> +        rc = frameView()->hostWindow()->platformPageClient()->showAlertDialog(atype);

Ditto.

>> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:728
>> +        return frameView()->hostWindow()->platformPageClient()->isVisible();
> 
> It's a layering violation for code in platform/ to know about code in page/

Ditto.

> LayoutTests/ChangeLog:11
> +        * media/audio-concurrent-supported.html: Added.

This test looks platform specific. Why is it in the common media test directory?

[WebKit Review Bot]

Comment on attachment 137402 [details]
Patch

Attachment 137402 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12417257

New failing tests:
media/audio-concurrent-supported.html

[WebKit Review Bot]

Created attachment 137414 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Max Feil]

Created attachment 137429 [details]
Patch

[Max Feil]

(In reply to comment #17)
> > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:115
> > +        tabId = frameView()->hostWindow()->platformPageClient();
> 
> This is a layering violation, code in WebCore/platform should know about code in /page.

Can you please be more specific about the layering violation? Accessing the platformPageClient() via hostWindow() seems to be fairly common in several of the major ports.


> > LayoutTests/ChangeLog:11
> > +        * media/audio-concurrent-supported.html: Added.
> 
> This test looks platform specific. Why is it in the common media test directory?

Can you please be more specific about which part of the test is platform specific? It passes for all major ports.

[Eric Carlson]

(In reply to comment #21)
> (In reply to comment #17)
> > > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:115
> > > +        tabId = frameView()->hostWindow()->platformPageClient();
> > 
> > This is a layering violation, code in WebCore/platform should know about code in /page.
> 
> Can you please be more specific about the layering violation? 
> 
Files in WebCore/platform should not reference WebCore files outside of platform. You can see a discussion in this email thread from earlier this year: https://lists.webkit.org/pipermail/webkit-dev/2012-January/019033.html

Adam Barth summed it up nicely I think:

    Something should go into Platform if it's an abstraction of a facility
    provided by the underlying platform.  For example, ScrollableArea is
    an abstraction of a concept provided by most platforms.  Something
    should not go in Platform if it refers to web concepts (e.g., a Frame,
    which belongs in WebCore proper) or if it's something that might be
    used by every program (e.g., RefPtr, which belongs in WTF).
[ SNIP ]
    Platform can depend on third-party libraries (e.g., SQLite) or on operating 
    system facilities.  Platform can depend on WTF, but no other code in 
    the WebKit project.


> Accessing the platformPageClient() via hostWindow() seems to be fairly common in several of the major ports.

I see now that it is used by BlackBerry, Cairo, EFL, and Qt:

[dev:WebCore/platform/graphics] eric% find . -name "*.cpp" | xargs grep -r "platformPageClient"
./blackberry/MediaPlayerPrivateBlackBerry.cpp:        rc = topdoc->view()->hostWindow()->platformPageClient()->showAlertDialog(atype);
./blackberry/MediaPlayerPrivateBlackBerry.cpp:        return frameView()->hostWindow()->platformPageClient()->platformWindow();
./cairo/GraphicsContext3DPrivate.cpp:    , m_glContext(GLContext::createOffscreenContext(GLContext::getContextForWidget(m_window->platformPageClient())))
./efl/GraphicsContext3DPrivate.cpp:    PageClientEfl* pageClient = static_cast<PageClientEfl*>(hostWindow->platformPageClient());
./qt/GraphicsContext3DQt.cpp:    QWebPageClient* webPageClient = m_hostWindow->platformPageClient();

This doesn't change the fact that this is a layering violation, which we strive to avoid in WebKit.

> 
> > > LayoutTests/ChangeLog:11
> > > +        * media/audio-concurrent-supported.html: Added.
> > 
> > This test looks platform specific. Why is it in the common media test directory?
> 
> Can you please be more specific about which part of the test is platform specific? It passes for all major ports.

My mistake. I assumed that a test for a BlackBerry specific feature would be platform specific. 

I should have asked - is the test useful for any other port? What does it test in a port that does not restrict the number of audio elements that can play concurrently?

[Max Feil]

(In reply to comment #22)
> Files in WebCore/platform should not reference WebCore files outside of platform. 

That clarifies it partially. But the question remains, which WebCore files am I referencing outside of platform that I'm not supposed to be? The platformPageClient() is in WebKit/blackberry/WebCoreSupport/, not WebCore/.


> I should have asked - is the test useful for any other port? What does it test in a port that does not restrict the number of audio elements that can play concurrently?

That makes sense. I don't think the test is very useful even for Blackberry. However patches are not passing review without associated automated layout tests, so I'm just doing what I can to try and shorten the review process.

[George Staikos]

(In reply to comment #23)
> (In reply to comment #22)
> > Files in WebCore/platform should not reference WebCore files outside of platform. 
> 
> That clarifies it partially. But the question remains, which WebCore files am I referencing outside of platform that I'm not supposed to be? The platformPageClient() is in WebKit/blackberry/WebCoreSupport/, not WebCore/.
> 
> 
> > I should have asked - is the test useful for any other port? What does it test in a port that does not restrict the number of audio elements that can play concurrently?
> 
> That makes sense. I don't think the test is very useful even for Blackberry. However patches are not passing review without associated automated layout tests, so I'm just doing what I can to try and shorten the review process.

I don't think this is any worse than the previous code here which was already r+.  However we probably need to think about how to abstract the platform/ code to not call to WebKit/ here.  All that said, this is pretty app-specific for BlackBerry.

[WebKit Review Bot]

Comment on attachment 137429 [details]
Patch

Clearing flags on attachment: 137429

Committed r114493: <http://trac.webkit.org/changeset/114493>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

(In reply to comment #24)
> 
> I don't think this is any worse than the previous code here which was already r+.  However we probably need to think about how to abstract the platform/ code to not call to WebKit/ here.  All that said, this is pretty app-specific for BlackBerry.

I agree that it would be good to figure out how to change the existing code to not assume/know so much about code outside of WebCore/platform. Particularly unfortunate, IMO, is the code that casts MediaPlayer::mediaPlayerClient() to HTMLMediaElement to make calls on the media element directly. The engine constructor is passed a MediaPlayer* so it has an opaque interface to make calls to WebCore without having to do this. 

The MediaPlayerClient interface can be extended if you need something from the media element or something it knows about. Adding the necessary plumbing isn't a lot of work, and once it is there other ports can use it too. For example, to allow the AVFoundation based media engine to pass the correct UA string (https://bugs.webkit.org/show_bug.cgi?id=46241) we added mediaPlayerUserAgent to MediaPlayerClient so the engine can get the media element to fetch the UA string from the loader. MediaPlayerBlackBerry apparently needed the same functionality as well, but instead casts to an HTMLMediaElement so it can get a pointer to the frame loader to call userAgent() directly. Bug 46241 would have been much easier to fix if the MediaPlayerClient plumbing had already been there :-).

[Max Feil]

(In reply to comment #27)

Thanks for the clear description, Eric. I've created Bug 84291 to track your concerns about HTMLMediaElement being referenced directly. I may need your help on that one because there are certain specific reasons we are using HTMLMediaElement. For example, taking the FrameView object from MediaPlayer is not possible during crucial phases of MediaPlayerPrivate setup/teardown because it is set too late and cleared too early.

[Eric Carlson]

(In reply to comment #28)
> (In reply to comment #27)
> 
> Thanks for the clear description, Eric. I've created Bug 84291 to track your concerns about HTMLMediaElement being referenced directly. I may need your help on that one because there are certain specific reasons we are using HTMLMediaElement. For example, taking the FrameView object from MediaPlayer is not possible during crucial phases of MediaPlayerPrivate setup/teardown because it is set too late and cleared too early.

Thanks for filing the bug! 

I will be happy to help you in any way I can. Feel free to track me down on irc, send me email with questions, or just cc me on bugs.

[Max Feil]

Closing bug for patch that landed a long time ago.