Summary: | [Qt] Can't play HTML5 Video because of the URL is percent encoded twice | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hui Huang <hui_huang> | ||||||||||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ademar, benjamin, commit-queue, eric, hausmann, hui_huang, laszlo.gombos, max.hong.shen, nancy.piedra, nanjundiah.harsha, yael | ||||||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
URL: | http://youtube.com/html5 | ||||||||||||||
Attachments: |
|
Description
Hui Huang
2011-02-07 20:41:06 PST
Created attachment 81577 [details]
Proposed patch
The URL of the HTML5 Video has already been percent encoded by youtube. It is percent encoded again in MediaPlayerPrivateQt::commitLoad by the constructor QUrl::QUrl(QString). According QUrl class reference (http://doc.qt.nokia.com/latest/qurl.html#QUrl-2), this constructor assumes that the URL is not percent encoded. Since the URL is double percent encoded, the HTTP GET Request for the video is rejected by youtube. The bug can be fixed by using QUrl::fromEncoded to construct QUrl with an encoded URL. Attached proposed patch. Comment on attachment 81577 [details]
Proposed patch
Setting review flag.
When you add a patch, you should set the review flag and the commit-queue flag, otherwise, reviewers do not see your patch (because it is not in the review queue). Here is some more info: http://trac.webkit.org/wiki/QtWebKitContrib I think a test is needed for this case. Comment on attachment 81577 [details]
Proposed patch
Needs a regression test.
Sorry, my mistake. Will add a test as recommended by you and Andreas. (In reply to comment #4) > When you add a patch, you should set the review flag and the commit-queue flag, otherwise, reviewers do not see your patch (because it is not in the review queue). Here is some more info: http://trac.webkit.org/wiki/QtWebKitContrib > > I think a test is needed for this case. I think calling ascii() is scary and IMHO we should avoid it. Wouldn't it be safer to convert the String into a KURL first, and then convert that to a QURL? These conversions we know are safe: KUrl kurl(ParsedURLString, url); QUrl qurl = kurl; But maybe I'm just to afraid of these encoding issues :) Thanks a lot for the advice. I tried your solution. It works. I think it is better than my original code. I will use it in the new patch I am working on. (In reply to comment #7) > I think calling ascii() is scary and IMHO we should avoid it. > > Wouldn't it be safer to convert the String into a KURL first, and then convert that to a QURL? These conversions we know are safe: > > KUrl kurl(ParsedURLString, url); > QUrl qurl = kurl; > > But maybe I'm just to afraid of these encoding issues :) Created attachment 82039 [details]
New patch with regression test
I attached a new patch with regression test and Simon's improvement. I spent quite some time trying to find out a way to create a test using HTML and JS only, but I can't find a good solution without the involvement of an external web server. Therefore, I added a test function in tst_qwebpage. Since the HTTP request is sent by QtMobility, QNetworkAccessManager can not be used to check the request URL. I used DumpRenderTreeSupportQt to access media content URL from the Media Player via video element in DOM. I tested the patch and the test function worked. Suggestions on improvements are appreciated. Created attachment 82049 [details]
New patch after resolving conflicts
I was also working on this issue. I had a similar fix for the issue. :-). I have tested the above patch on ubuntu and youtube html5 video playback works well. Note: Similar fix is needed in the phonon path i.e in MediaPlayerPrivatePhonon.cpp Comment on attachment 82049 [details]
New patch after resolving conflicts
I like it, but I think the test will fail on platforms that don't have QtMobility installed.
Thanks for the suggestion. I will make a new patch to check QtMobility availability when building qwebview test. (In reply to comment #13) > (From update of attachment 82049 [details]) > I like it, but I think the test will fail on platforms that don't have QtMobility installed. Thanks for the info. I think a new bug report with test steps is probably needed for Phonon, so that we can reproduce the problem and test the solution. (In reply to comment #12) > I was also working on this issue. I had a similar fix for the issue. :-). > I have tested the above patch on ubuntu and youtube html5 video playback works well. > > Note: Similar fix is needed in the phonon path i.e in MediaPlayerPrivatePhonon.cpp Created attachment 82540 [details]
New patch after fixing build problem without Qt Multimedia. Skipping test if Qt Multimedia is not available.
Created attachment 82575 [details]
New patch to resolve conflicts
Comment on attachment 82575 [details] New patch to resolve conflicts View in context: https://bugs.webkit.org/attachment.cgi?id=82575&action=review > Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:83 > +#if ENABLE(VIDEO) && ENABLE(QT_MULTIMEDIA) Can we just use ENABLE(VIDEO) guard here - that is less code and the function could be perhaps reused for the phonon case as well ? > Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:969 > +#if ENABLE(VIDEO) && ENABLE(QT_MULTIMEDIA) Ditto. Thanks for the advice. The reason to use ENABLE(QT_MULTIMEDIA) is that MediaPlayerPrivateQt.h can only be compiled with Qt Multimedia because of the dependency on QMediaPlayer. If only ENABLE(VIDEO) is used and Qt Multimedia is not available, there may be build problems. (In reply to comment #18) > (From update of attachment 82575 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82575&action=review > > > Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:83 > > +#if ENABLE(VIDEO) && ENABLE(QT_MULTIMEDIA) > > Can we just use ENABLE(VIDEO) guard here - that is less code and the function could be perhaps reused for the phonon case as well ? Comment on attachment 82575 [details]
New patch to resolve conflicts
r=me.
Thanks a lot. (In reply to comment #20) > (From update of attachment 82575 [details]) > r=me. Comment on attachment 82575 [details] New patch to resolve conflicts Rejecting attachment 82575 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 1 Last 500 characters of output: ommit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run step(tool, options).run(state) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 57, in run if self._has_valid_reviewer(changelog_entry): File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 42, in _has_valid_reviewer if changelog_entry.reviewer(): AttributeError: 'NoneType' object has no attribute 'reviewer' Full output: http://queues.webkit.org/results/7927410 (In reply to comment #22) > (From update of attachment 82575 [details]) > Rejecting attachment 82575 [details] from commit-queue. > > Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 1 > > Last 500 characters of output: > ommit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run > step(tool, options).run(state) > File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 57, in run > if self._has_valid_reviewer(changelog_entry): > File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 42, in _has_valid_reviewer > if changelog_entry.reviewer(): > AttributeError: 'NoneType' object has no attribute 'reviewer' > > Full output: http://queues.webkit.org/results/7927410 A bug in commit-queue perhaps? Laszlo: can you please land it manually? Comment on attachment 82575 [details]
New patch to resolve conflicts
Try again.
(In reply to comment #24) > (From update of attachment 82575 [details]) > Try again. happened to me too. It worked on second try :-) Comment on attachment 82575 [details] New patch to resolve conflicts Clearing flags on attachment: 82575 Committed r78817: <http://trac.webkit.org/changeset/78817> All reviewed patches have been landed. Closing bug. Revision r78817 cherry-picked into qtwebkit-2.1.x with commit 10a21c6 <http://gitorious.org/webkit/qtwebkit/commit/10a21c6> (In reply to comment #23) > (In reply to comment #22) > > (From update of attachment 82575 [details] [details]) > > Rejecting attachment 82575 [details] [details] from commit-queue. > > > > Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 1 > > > > Last 500 characters of output: > > ommit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run > > step(tool, options).run(state) > > File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 57, in run > > if self._has_valid_reviewer(changelog_entry): > > File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 42, in _has_valid_reviewer > > if changelog_entry.reviewer(): > > AttributeError: 'NoneType' object has no attribute 'reviewer' > > > > Full output: http://queues.webkit.org/results/7927410 > > A bug in commit-queue perhaps? Laszlo: can you please land it manually? This generally happens when missing a ChangeLog (or ChangeLog entry). Clearly we need to print a better message. |