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.
Created attachment 131577 [details] Patch
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?
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.
Comment on attachment 131577 [details] Patch Needs investigation whether a test can be made, per Eric's and my comment.
Created attachment 136391 [details] Patch
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.
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.
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.
Created attachment 136617 [details] Patch
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
Comment on attachment 136617 [details] Patch Looks good to me!
(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.
Created attachment 136801 [details] Patch
Comment on attachment 136801 [details] Patch Clearing flags on attachment: 136801 Committed r113937: <http://trac.webkit.org/changeset/113937>
All reviewed patches have been landed. Closing bug.