Bug 58188 - [Qt] MediaPlayerPrivateQt::supportsType does not parse codec parameter
Summary: [Qt] MediaPlayerPrivateQt::supportsType does not parse codec parameter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nancy Piedra
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2011-04-09 04:35 PDT by Nancy Piedra
Modified: 2011-04-14 04:41 PDT (History)
1 user (show)

See Also:


Attachments
Patch to parse codec string (1.99 KB, patch)
2011-04-09 09:24 PDT, Nancy Piedra
benjamin: review-
Details | Formatted Diff | Diff
Modified patch based on Benjamin's comments (1.97 KB, patch)
2011-04-11 10:56 PDT, Nancy Piedra
benjamin: review-
Details | Formatted Diff | Diff
Modified patch based on Benjamin's comments (3.37 KB, patch)
2011-04-12 07:28 PDT, Nancy Piedra
no flags Details | Formatted Diff | Diff
Add expected results (4.28 KB, patch)
2011-04-12 10:59 PDT, Nancy Piedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nancy Piedra 2011-04-09 04:35:54 PDT
The codec parameter in MediaPlayerPrivateQt::supportsType is not parsed before being passed to the QMediaPlayer::hasSupport function which takes a QStringList.  See below:

MediaPlayer::SupportsType MediaPlayerPrivateQt::supportsType(const String& mime, const String& codec)
...
int QtMediaPlayerSupport = QMediaPlayer::hasSupport(mime, QStringList(codec));


I will be uploading a patch soon.
Comment 1 Nancy Piedra 2011-04-09 09:24:02 PDT
Created attachment 88930 [details]
Patch to parse codec string

The codec parameter in MediaPlayerPrivateQt::supportsType was not parsing the 'codec' QString into a QStringList which is required for the QMediaPlayer::hasSupport function.

This patch parses and trims the codec parameter into a QStringList.
Comment 2 Eric Seidel (no email) 2011-04-10 15:56:41 PDT
Comment on attachment 88930 [details]
Patch to parse codec string

Do you need to unskip a test or update results?
Comment 3 Nancy Piedra 2011-04-11 05:48:21 PDT
(In reply to comment #2)
> (From update of attachment 88930 [details])
> Do you need to unskip a test or update results?

Eric,

My goal is to get the video-can-play-type.html test to pass on Qt.  See this bug:
https://bugs.webkit.org/show_bug.cgi?id=42094

But during my investigations I noticed general issues in this area with parsing, etc.  This bug is one of them.  These are the others:
Misuse of &quote: https://bugs.webkit.org/show_bug.cgi?id=57728 (misue of &quot)
Improper parsing of codecs param: https://bugs.webkit.org/show_bug.cgi?id=53275

I'm trying to fix the issues incrementally.  Right now, I can't pass video-can-play-type.html for Qt because there are some other issues in the return type (either in Qt Multimedia or QtWebkit - I'm still investigating)

Maybe I should make this error a dependency on 42094?  I'm open to other suggestions what is the proper thing to do.
Comment 4 Benjamin Poulain 2011-04-11 06:13:41 PDT
Comment on attachment 88930 [details]
Patch to parse codec string

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

Review express ...

> Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:87
> +    // Parse and trim codecs

Missing dot.

> Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:89
> +    QString codecStr = codec;

Why do you need to copy the string?

> Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:90
> +    codecList = codecStr.split(QLatin1String(","), QString::SkipEmptyParts);

That should be a QLatin1Char(',').

Why do you create the empty QStringList just to assign it another value 2 lines later?

> Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:92
> +    foreach (const QString &codecStrNotTrimmed, codecList) {

Coding style.
Comment 5 Nancy Piedra 2011-04-11 10:56:57 PDT
Created attachment 89030 [details]
Modified patch based on Benjamin's comments

I modified the patch based on Benjamin's comments.

I believe I addressed all comments except:

1.  Why do you need to copy the string?  I needed to created a QString from a String so that I could use the "split" function.

2.  Coding style.  I ran the "check-webkit-style" script and saw no style errors.  If you could be more specific I will fix it.
Comment 6 Benjamin Poulain 2011-04-12 04:56:46 PDT
Comment on attachment 89030 [details]
Modified patch based on Benjamin's comments

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

I am ok with the change. It is not very solid but hey, that is the way QMediaPlayer is defined.
Just clean the patch before it can land.

> Source/WebCore/ChangeLog:9
> +        a QStringList.  This change parses and trims the list.

Two spaces after the dot.

> Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:91
> +    foreach (const QString &codecStrNotTrimmed, codecList) {

coding style: 
const QString& codecStrNotTrimmed.

> Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:93
> +        codecListTrimmed.append(codecStrTrimmed);

Couldn't you have an empty string here.
If codecs = "foo,    , bar,    ". The parts containing only spaces will be null string after trimmed() and will be inserted in the list.
Comment 7 Nancy Piedra 2011-04-12 07:28:24 PDT
Created attachment 89200 [details]
Modified patch based on Benjamin's comments

Modified patch based on Benjamin's comments.

Added a test for a null string in the codecs paramter based on Benjamin's comments.
Comment 8 Nancy Piedra 2011-04-12 10:59:41 PDT
Created attachment 89229 [details]
Add expected results
Comment 9 Benjamin Poulain 2011-04-14 03:41:32 PDT
Comment on attachment 89229 [details]
Add expected results

lgtm
Comment 10 WebKit Commit Bot 2011-04-14 04:38:47 PDT
The commit-queue encountered the following flaky tests while processing attachment 89229 [details]:

http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: alice.liu@apple.com)
The commit-queue is continuing to process your patch.
Comment 11 WebKit Commit Bot 2011-04-14 04:41:28 PDT
Comment on attachment 89229 [details]
Add expected results

Clearing flags on attachment: 89229

Committed r83836: <http://trac.webkit.org/changeset/83836>
Comment 12 WebKit Commit Bot 2011-04-14 04:41:32 PDT
All reviewed patches have been landed.  Closing bug.