Bug 82930 - [BlackBerry] Tab awareness for HTML5 concurrent audio
Summary: [BlackBerry] Tab awareness for HTML5 concurrent audio
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-02 13:00 PDT by Max Feil
Modified: 2012-11-02 12:34 PDT (History)
12 users (show)

See Also:


Attachments
Patch (7.22 KB, patch)
2012-04-02 13:26 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Patch (7.33 KB, patch)
2012-04-04 15:24 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Patch (12.25 KB, patch)
2012-04-16 14:24 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (6.16 MB, application/zip)
2012-04-16 15:24 PDT, WebKit Review Bot
no flags Details
Patch (12.25 KB, patch)
2012-04-16 16:36 PDT, Max Feil
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Max Feil 2012-04-02 13:00:00 PDT
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".
Comment 1 Max Feil 2012-04-02 13:02:11 PDT
For more information on the BlackBerry side of things, see PR96004.
Comment 2 Max Feil 2012-04-02 13:26:02 PDT
Created attachment 135166 [details]
Patch
Comment 3 Max Feil 2012-04-02 13:31:30 PDT
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.
Comment 4 Antonio Gomes 2012-04-02 21:40:19 PDT
could not the PAGE_VISIBILITY stuff we just enabled to BlackBerry port help somehow here?
Comment 5 Andrew Lo 2012-04-03 08:24:07 PDT
(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.
Comment 6 Max Feil 2012-04-03 13:20:13 PDT
(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.
Comment 7 Max Feil 2012-04-04 07:41:19 PDT
I will write a manual test in ManualTests/blackberry and I'm hoping that will be enough to give this bug an r+
Comment 8 Antonio Gomes 2012-04-04 08:51:12 PDT
(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?
Comment 9 Andrew Lo 2012-04-04 09:02:31 PDT
(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.
Comment 10 Max Feil 2012-04-04 12:22:06 PDT
(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.
Comment 11 Max Feil 2012-04-04 15:24:24 PDT
Created attachment 135697 [details]
Patch
Comment 12 Max Feil 2012-04-04 15:27:22 PDT
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
Comment 13 Antonio Gomes 2012-04-05 10:34:09 PDT
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?
Comment 14 Max Feil 2012-04-05 10:44:55 PDT
(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
Comment 15 Max Feil 2012-04-16 14:24:23 PDT
Created attachment 137402 [details]
Patch
Comment 16 Simon Fraser (smfr) 2012-04-16 14:56:23 PDT
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/
Comment 17 Eric Carlson 2012-04-16 15:02:31 PDT
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?
Comment 18 WebKit Review Bot 2012-04-16 15:24:23 PDT
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
Comment 19 WebKit Review Bot 2012-04-16 15:24:30 PDT
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
Comment 20 Max Feil 2012-04-16 16:36:55 PDT
Created attachment 137429 [details]
Patch
Comment 21 Max Feil 2012-04-16 17:34:42 PDT
(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.
Comment 22 Eric Carlson 2012-04-16 21:29:42 PDT
(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?
Comment 23 Max Feil 2012-04-16 22:54:27 PDT
(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.
Comment 24 George Staikos 2012-04-18 01:22:24 PDT
(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.
Comment 25 WebKit Review Bot 2012-04-18 01:40:56 PDT
Comment on attachment 137429 [details]
Patch

Clearing flags on attachment: 137429

Committed r114493: <http://trac.webkit.org/changeset/114493>
Comment 26 WebKit Review Bot 2012-04-18 01:41:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 Eric Carlson 2012-04-18 12:49:38 PDT
(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 :-).
Comment 28 Max Feil 2012-04-18 15:52:10 PDT
(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.
Comment 29 Eric Carlson 2012-04-18 17:13:03 PDT
(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.
Comment 30 Max Feil 2012-11-02 12:34:14 PDT
Closing bug for patch that landed a long time ago.