Bug 33453 - [Qt] MediaPlayerPrivate: expose supported types to WebCore
Summary: [Qt] MediaPlayerPrivate: expose supported types to WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P3 Normal
Assignee: QtWebKit Unassigned
URL:
Keywords: Qt
Depends on: 33842
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-10 15:22 PST by Jakub Wieczorek
Modified: 2010-03-22 11:42 PDT (History)
9 users (show)

See Also:


Attachments
patch (5.65 KB, patch)
2010-01-10 15:29 PST, Jakub Wieczorek
hausmann: review-
Details | Formatted Diff | Diff
patch (9.37 KB, patch)
2010-01-21 08:23 PST, Jakub Wieczorek
eric: review-
Details | Formatted Diff | Diff
patch (9.97 KB, patch)
2010-03-22 10:25 PDT, Jakub Wieczorek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Wieczorek 2010-01-10 15:22:29 PST
MediaPlayerPrivate should expose the MIME types that are supported by the underlying backend in Phonon.

MediaPlayerPrivate::supportsType() and ::getSupportedTypes() need to be implemented. It will get HTMLMediaElement::canPlay() to work as expected in JS and, since this is extensively used in media layout tests, some of them could be finally enabled (all of them are skipped now).
Comment 1 Jakub Wieczorek 2010-01-10 15:29:58 PST
Created attachment 46246 [details]
patch

This patch uses Phonon::BackendCapabilities to get the supported media types.

Note: to work-around the fact that GStreamer does not list MIME types such as video/ogg, audio/ogg, video/mp4 even if it supports them, the code will check for other types equivalent to them. The Gtk port does the same thing. Perhaps this kind of work-around should go into Phonon itself.
Comment 2 Jakub Wieczorek 2010-01-10 15:36:20 PST
Regarding the layout tests: I am getting strange crashes when running the /media tests but only sequentially (running each test separately is fine) and only in DRT, I can't reproduce the crashes in QtLauncher (with -r as well). Can someone confirm that or would it even be possible to check them on the build bot?
Comment 3 Antonio Gomes 2010-01-11 05:31:19 PST
(In reply to comment #2)
> Regarding the layout tests: I am getting strange crashes when running the
> /media tests but only sequentially (running each test separately is fine) and
> only in DRT, I can't reproduce the crashes in QtLauncher (with -r as well). Can
> someone confirm that or would it even be possible to check them on the build
> bot?

ossy, could you please help jakub on this ?
Comment 4 Csaba Osztrogonác 2010-01-11 08:32:54 PST
(In reply to comment #2)
> Regarding the layout tests: I am getting strange crashes when running the
> /media tests but only sequentially (running each test separately is fine) and
> only in DRT, I can't reproduce the crashes in QtLauncher (with -r as well). Can
> someone confirm that or would it even be possible to check them on the build
> bot?

I will test it on buildbot.
Comment 5 Csaba Osztrogonác 2010-01-11 10:04:41 PST
I tested media tests on official QtBuildBot (Debian Lenny).

results of "run-webkit-tests media --skipped=ignore" :
9 test cases (8%) succeeded
38 test cases (37%) had incorrect layout
10 test cases (9%) were new
45 test cases (44%) crashed
46 test cases (45%) had stderr output

results of "run-webkit-tests media --skipped=ignore --singly" :
16 test cases (15%) succeeded
69 test cases (67%) had incorrect layout
17 test cases (16%) were new
84 test cases (82%) had stderr output


Additionally I tried media tests with your patch and the number of passing/failing test wasn't changed.
Comment 6 Csaba Osztrogonác 2010-01-11 10:10:15 PST
I have no idea what caused the 45 strange crashes. (7 new, 7 passed, 31 failed with --singly option) It can be a DRT bug needs more investigation.
Comment 7 Jakub Wieczorek 2010-01-11 11:04:01 PST
(In reply to comment #5)
> Additionally I tried media tests with your patch and the number of
> passing/failing test wasn't changed.

I am getting different results here with and without the patch:

Before:
18 test cases (17%) succeeded
66 test cases (64%) had incorrect layout
17 test cases (16%) were new
1 test case (<1%) timed out
61 test cases (59%) had stderr output

After:
50 test cases (49%) succeeded
35 test cases (34%) had incorrect layout
17 test cases (16%) were new
26 test cases (25%) had stderr output

I suppose this is dependant on the available gstreamer plugins.
Comment 8 Csaba Osztrogonác 2010-01-13 07:52:59 PST
I installed the missing packages, and I got similar results as you.

results of "run-webkit-tests media --skipped=only --singly":
52 test cases (50%) succeeded
34 test cases (33%) had incorrect layout
17 test cases (16%) were new
18 test cases (17%) had stderr output
Comment 9 Simon Hausmann 2010-01-16 01:04:35 PST
Comment on attachment 46246 [details]
patch


In general this patch looks good to me! I'm only nitpicking a bit :)

> +static HashSet<String>* supportedTypes = 0;

Shouldn't this be a global static object, so that the has is freed?

> +
>  MediaPlayerPrivate::MediaPlayerPrivate(MediaPlayer* player)
>      : m_player(player)
>      , m_networkState(MediaPlayer::Empty)
> @@ -145,15 +149,64 @@ MediaPlayerPrivate::~MediaPlayerPrivate()
>      m_mediaObject = 0;
>  }
>  
> -void MediaPlayerPrivate::getSupportedTypes(HashSet<String>&)
> +void MediaPlayerPrivate::rebuildSupportedTypesCache()
>  {
> -    notImplemented();
> +    ASSERT(!supportedTypes);
> +    supportedTypes = new HashSet<String>;

Why is the function called _rebuild_SupportedTypesCache() when it can only
be called once? :) (Assert in debug build, leak in release build)
Comment 10 Csaba Osztrogonác 2010-01-18 00:28:32 PST
The patch would be good, but don't forget about crashes. If we run tests in the same DRT, we get 38 crashed test. If we run them separately (with --singly option), 15 of them pass, 15 of them failed, 8 of them are new.

I think we should find the reason of crashes and fix it before landing.
Comment 11 Jakub Wieczorek 2010-01-19 07:51:45 PST
(In reply to comment #10)
> The patch would be good, but don't forget about crashes. If we run tests in the
> same DRT, we get 38 crashed test. If we run them separately (with --singly
> option), 15 of them pass, 15 of them failed, 8 of them are new.
> 
> I think we should find the reason of crashes and fix it before landing.

Definitely!

I filed https://bugs.webkit.org/show_bug.cgi?id=33842 with a backtrace.
Comment 12 Jakub Wieczorek 2010-01-21 08:23:15 PST
Created attachment 47121 [details]
patch

Updated the patch according to the suggestions from Simon on IRC.

This one includes the Skipped list update. I went through the tests and unskipped those that are stable enough so that they hopefully won't cause problems on the bot.
Ossy, could you please check this one on the bot and see if the tests are reliable?
Comment 13 Simon Hausmann 2010-01-23 01:47:33 PST
Comment on attachment 47121 [details]
patch

r=me. Do you want to try it out on the bot on monday or otherwise early next week, or do you want to try now and we set cq+? :)
Comment 14 Jakub Wieczorek 2010-01-23 01:55:48 PST
(In reply to comment #13)
> (From update of attachment 47121 [details])
> r=me. Do you want to try it out on the bot on monday or otherwise early next
> week, or do you want to try now and we set cq+? :)

Thanks. OK, let's try it now. If anything goes wrong, will you be able to roll it out? :)
Comment 15 Csaba Osztrogonác 2010-01-23 04:41:53 PST
Comment on attachment 47121 [details]
patch

Unfortunately I have bad news. I executed a test on bot, but 3 tests failed and 2 crashed.

failed:
media/audio-mpeg-supported.html
media/audio-play-event.html
media/video-pause-immediately.html

crashed:
media/video-source-add-src.html
media/video-volume.html

I skipped them, but another one test failed and another 2 crashed. :S

I think there are some flakey tesst enabled brake the others, so I "cq-"-ed it,
and later (after 21-22 pm GMT+1) I will find what caused this strange thing.
Comment 16 Csaba Osztrogonác 2010-01-24 14:35:37 PST
I played a lot with the skiplist, but I can't make a safe skiplist. :( I have 55 stable passing tests if I execute run-webkit-test with --singly option. But without --singly DRT get crazy. :S If I skipped a failing or crashing test, another one failed. I tried to find which test cause which test, but I can't find crasher/crashee pairs :( And then I tried with --valgrind option, but all tests crashed. :o Now I have no idea have can we kill these flakeyness ... I can play with it at monday and tuesday night. (I won't be online in the daytime.)
Comment 17 Eric Seidel (no email) 2010-02-02 14:19:43 PST
Ping?  This was r+'d a week ago.
Comment 18 Eric Seidel (no email) 2010-02-17 14:26:16 PST
Ping?  This was r+'d over 3 weeks ago.  Is this patch abandoned?
Comment 19 Jakub Wieczorek 2010-02-17 14:37:47 PST
(In reply to comment #18)
> Ping?  This was r+'d over 3 weeks ago.  Is this patch abandoned?

No, it's not. This patch can't be landed as-is, because there are still some random crashes in the tests that are to be unskipped (unrelated to the patch itself) - see comment #15. I can't investigate them in the next days unfortunately, so either the patch can be committed without the Skipped list update or will need to wait another couple of days.
Comment 20 Eric Seidel (no email) 2010-03-05 14:39:45 PST
Comment on attachment 47121 [details]
patch

Marking r-.  No update in over 2 weeks.  This can be re-reviewed when it's ready to land.
Comment 21 Jakub Wieczorek 2010-03-22 10:25:15 PDT
Created attachment 51301 [details]
patch

OK, one more try with this patch.

It's the same as the last version except that it doesn't unskip all the media tests: just two that only cover what we are fixing here and are stable enough. The rest is just too flakey when running in a group, they either time out or crash and I couldn't track it down. Possibly still many of the tests can be safely unskipped, but I'll just leave it for another patch.
Hopefully the tests can perform better with the new media backend.

Simon, can you please have a look once again? Thanks.
Comment 22 Jakub Wieczorek 2010-03-22 10:27:24 PDT
I should have mentioned what are the tests I've unskipped:
media/media-can-play-ogg.html
media/video-can-play-type.html
Comment 23 Kenneth Rohde Christiansen 2010-03-22 10:57:26 PDT
Comment on attachment 51301 [details]
patch

Let me carry on the r+ from Simon. Let's see how this does on the commit-queue.
Comment 24 Jakub Wieczorek 2010-03-22 11:34:04 PDT
Comment on attachment 51301 [details]
patch

(In reply to comment #23)
> (From update of attachment 51301 [details])
> Let me carry on the r+ from Simon. Let's see how this does on the commit-queue.

Thanks, I'll land it myself.
Comment 25 Jakub Wieczorek 2010-03-22 11:41:48 PDT
Comment on attachment 51301 [details]
patch

Clearing flags on attachment: 51301

Committed r56349: <http://trac.webkit.org/changeset/56349>
Comment 26 Jakub Wieczorek 2010-03-22 11:42:00 PDT
All reviewed patches have been landed.  Closing bug.