Bug 33453 – [Qt] MediaPlayerPrivate: expose supported types to WebCore

• Component: Media
• Status: RESOLVED FIXED

[Jakub Wieczorek]

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).

[Jakub Wieczorek]

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.

[Jakub Wieczorek]

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?

[Antonio Gomes]

(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 ?

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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.

[Jakub Wieczorek]

(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.

[Csaba Osztrogonác]

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

[Simon Hausmann]

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)

[Csaba Osztrogonác]

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.

[Jakub Wieczorek]

(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.

[Jakub Wieczorek]

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?

[Simon Hausmann]

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+? :)

[Jakub Wieczorek]

(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? :)

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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.)

[Eric Seidel (no email)]

Ping?  This was r+'d a week ago.

[Eric Seidel (no email)]

Ping?  This was r+'d over 3 weeks ago.  Is this patch abandoned?

[Jakub Wieczorek]

(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.

[Eric Seidel (no email)]

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.

[Jakub Wieczorek]

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.

[Jakub Wieczorek]

I should have mentioned what are the tests I've unskipped:
media/media-can-play-ogg.html
media/video-can-play-type.html

[Kenneth Rohde Christiansen]

Comment on attachment 51301 [details]
patch

Let me carry on the r+ from Simon. Let's see how this does on the commit-queue.

[Jakub Wieczorek]

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.

[Jakub Wieczorek]

Comment on attachment 51301 [details]
patch

Clearing flags on attachment: 51301

Committed r56349: <http://trac.webkit.org/changeset/56349>

[Jakub Wieczorek]

All reviewed patches have been landed.  Closing bug.