Bug 58188 – [Qt] MediaPlayerPrivateQt::supportsType does not parse codec parameter

• Component: Media
• Status: RESOLVED FIXED

[Nancy Piedra]

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.

[Nancy Piedra]

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.

[Eric Seidel (no email)]

Comment on attachment 88930 [details]
Patch to parse codec string

Do you need to unskip a test or update results?

[Nancy Piedra]

(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.

[Benjamin Poulain]

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.

[Nancy Piedra]

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.

[Benjamin Poulain]

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.

[Nancy Piedra]

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.

[Nancy Piedra]

Created attachment 89229 [details]
Add expected results

[Benjamin Poulain]

Comment on attachment 89229 [details]
Add expected results

lgtm

[WebKit Commit Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 89229 [details]
Add expected results

Clearing flags on attachment: 89229

Committed r83836: <http://trac.webkit.org/changeset/83836>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.