Bug 58188

Summary: [Qt] MediaPlayerPrivateQt::supportsType does not parse codec parameter
Product: WebKit Reporter: Nancy Piedra <nancy.piedra>
Component: MediaAssignee: Nancy Piedra <nancy.piedra>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue
Priority: P3 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch to parse codec string
benjamin: review-
Modified patch based on Benjamin's comments
benjamin: review-
Modified patch based on Benjamin's comments
none
Add expected results none

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.