Bug 80978 – [BlackBerry] MMRPlayer will hang webkit thread when retrieving media metadata

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Jonathan Dong]

this always happens in bad network environment. When MMRPlayer need a long time
to retrieve metadata, the webkit thread is blocked until user get the metadata
or press cancel.

way to reproduce:
1. load live streaming url: http://multiplatform-f.akamaihd.net/i/multi/seeker/LegendofSeeker_16x9_24fps_H264,_1.4Mbps,_1.8Mbps,_1Mbps,_2.5Mbps,_400K,_650K,.mp4.csmil/master.m3u8
then you can see the chrome stop responding when retrieving metadata before the media can get played.

root cause:
the webkit thread will wait for the metadata retrieving thread in
MMRPlayer::mmrConnect(). we should avoid using pthread_cond_timedwait() here,
instead we can start a timer to check the metadata finished status every time
it get fired.

[Jonathan Dong]

Created attachment 131577 [details]
Patch

[Rob Buis]

Comment on attachment 131577 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        No new tests.

It seems like a test is possible?

[Eric Carlson]

Comment on attachment 131577 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        No new tests.
> 
> It seems like a test is possible?

Yes definitely. LayoutTests/http/tests/media/video-load-and-stall.cgi will load part of a file and stall forever, and LayoutTests/http/tests/media/video-throttled-load.cgi will load a file at a specified bitrate to simulate a slow network. These are used by several media tests in LayoutTests/http/tests/media.

[Rob Buis]

Comment on attachment 131577 [details]
Patch

Needs investigation whether a test can be made, per Eric's and my comment.

[Jonathan Dong]

Created attachment 136391 [details]
Patch

[Rob Buis]

Comment on attachment 136391 [details]
Patch

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

Looks good, nice that a test can be added, please fix the typo's before landing.

> Source/WebCore/ChangeLog:10
> +        which starts a timer to wait for the metadata retriving

Typo retrieving.

> Source/WebCore/ChangeLog:11
> +        finish, and popup a dialog to notify user what to do

"to finish", "the user"

> Source/WebCore/ChangeLog:12
> +        if still haven't get metadata when timer fired. This won't

"if still haven't get metadata" -> "if there still is no metadata"

> Source/WebCore/ChangeLog:13
> +        block the webkit thread as what we used to do in platform repo.

please rephrase the last sentence, it is not completely clear to me what you mean.

> LayoutTests/ChangeLog:11
> +        webkit thread is blocked by media retriving thread.

Typo retrieving.

[Eric Carlson]

Comment on attachment 136391 [details]
Patch

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

Marking r- for the incorrect 'type' param in the cgi url.

> LayoutTests/http/tests/media/video-throttle-load-metadata-worker.js:2
> +postMessage("Message from worker thread");
>  \ No newline at end of file

Nit: please add a blank line at the end of the file.

> LayoutTests/http/tests/media/video-throttle-load-metadata.html:5
> +    <!--
> +    This test case is for https://bugs.webkit.org/show_bug.cgi?id=80978
> +    --!>

Nit: Personally, I think comments like this are very helpful to have in the markup of the test so someone looking at the test results, or running the test itself, can see it without having to look through the source.

> LayoutTests/http/tests/media/video-throttle-load-metadata.html:13
> +        function loadedmetadata(e) {
> +            logResult(true, "loaded metadata of media file");
> +            endTest();
> +        }

WebKit style is to have a function's opening brace on a new line.

> LayoutTests/http/tests/media/video-throttle-load-metadata.html:15
> +        function error(e) {

Ditto.

> LayoutTests/http/tests/media/video-throttle-load-metadata.html:20
> +        function start() {

Ditto.

> LayoutTests/http/tests/media/video-throttle-load-metadata.html:32
> +            video.src = "http://127.0.0.1:8000/media/video-throttled-load.cgi?name=" + movie + "&throttle=400&type=video/mp4";

"type=video/mp4" isn't necessarily correct. findMediaFile() will return a file that the current port can load, which may not be MPEG-4. You should use something like "mimeTypeForExtension(movie.split('.').pop())" instead.

[Jonathan Dong]

Comment on attachment 136391 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        block the webkit thread as what we used to do in platform repo.
> 
> please rephrase the last sentence, it is not completely clear to me what you mean.

Thanks Rob, What I'm trying to say is according to our old implementation in platform repo, we hang the webkit thread to wait for metadata thread to finish, the codes lie inside the class BlackBerry::Platform::MMRPlayer::mmrConnect. This patch is part of the fix for this issue, the other part of fix in platform repo has already get reviewed by Max and will push into the trunk together with this one when done.
As this sentence is kind of confusing, and the former words already described the function of this patch, so I intend to remove it in the next patch.

>> LayoutTests/http/tests/media/video-throttle-load-metadata.html:32
>> +            video.src = "http://127.0.0.1:8000/media/video-throttled-load.cgi?name=" + movie + "&throttle=400&type=video/mp4";
> 
> "type=video/mp4" isn't necessarily correct. findMediaFile() will return a file that the current port can load, which may not be MPEG-4. You should use something like "mimeTypeForExtension(movie.split('.').pop())" instead.

Thanks Eric, will fix it with the next patch.

[Jonathan Dong]

Created attachment 136617 [details]
Patch

[Rob Buis]

Comment on attachment 136617 [details]
Patch

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

LGTM. Let's wait a bit with cq+, maybe Eric has some more comments.

> Source/WebCore/ChangeLog:11
> +        finish, and popup a dialog to notify the user what to do

and popup -> and pops up

> Source/WebCore/ChangeLog:12
> +        if there still is no metadata when the timer fired.

fired -> fires

[Eric Carlson]

Comment on attachment 136617 [details]
Patch

Looks good to me!

[Rob Buis]

(In reply to comment #11)
> (From update of attachment 136617 [details])
> Looks good to me!

Thanks Eric!
Jonathan, please fix the typo's tomorrow and get Charles or Leo to help with landing this :)
Cheers,

Rob.

[Jonathan Dong]

Created attachment 136801 [details]
Patch

[WebKit Review Bot]

Comment on attachment 136801 [details]
Patch

Clearing flags on attachment: 136801

Committed r113937: <http://trac.webkit.org/changeset/113937>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.