Summary: | [BlackBerry] MMRPlayer will hang webkit thread when retrieving media metadata | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Dong <jonathan.dong.webkit> | ||||||||||
Component: | WebKit BlackBerry | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Critical | CC: | bipingm, charles.wei, eric.carlson, feature-media-reviews, leo.yang, mfeil, rwlbuis, staikos, tonikitoo, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Jonathan Dong
2012-03-13 02:22:20 PDT
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. |