Bug 50209 – MediaPlayer should try all installed media engines

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

When a file load fails before reaching HAVE_METADATA and there is more than one registered media engine, MediaPlayer.cpp should try loading with the next installed engine.

[Eric Carlson]

Created attachment 75233 [details]
Proposed patch

[Eric Carlson]

Created attachment 75913 [details]
Updated patch

Updated to behave correctly when retrying when there is no MIME type.

[Eric Seidel (no email)]

Attachment 75913 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6754115

[Early Warning System Bot]

Attachment 75913 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6719133

[WebKit Review Bot]

Attachment 75913 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6862077

[Build Bot]

Attachment 75913 [details] did not build on win:
Build output: http://queues.webkit.org/results/6720123

[Eric Seidel (no email)]

Attachment 75913 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6759127

[Eric Carlson]

Created attachment 76064 [details]
Patch that might compile

[Joseph Pecoraro]

Comment on attachment 76064 [details]
Patch that might compile

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

> WebCore/platform/graphics/MediaPlayer.cpp:233
>      unsigned count = engines.size();
>      for (unsigned ndx = 0; ndx < count; ndx++) {

(see comment below)

> WebCore/platform/graphics/MediaPlayer.cpp:257
> +    Vector<MediaPlayerFactory*>& engines = installedMediaEngines();
> +    if (engines.isEmpty())
> +        return 0;
> +
> +    MediaPlayerFactory* engine = 0;
> +    unsigned count = engines.size();
> +    for (unsigned ndx = 0; ndx < count; ndx++) {

I recently learned that Vector::size() is size_t, and on 64bit systems may be 64bits,
but unsigned is always 32bits. Although it probably isn't an issue here (count and
ndx), its something to keep in mind for future code.

[Darin Adler]

Comment on attachment 76064 [details]
Patch that might compile

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

> WebCore/platform/graphics/MediaPlayer.cpp:253
> +    if (engines.isEmpty())
> +        return 0;

No need for a special case. The normal code path handles an empty vector just fine.

>> WebCore/platform/graphics/MediaPlayer.cpp:257
>> +    for (unsigned ndx = 0; ndx < count; ndx++) {
> 
> I recently learned that Vector::size() is size_t, and on 64bit systems may be 64bits,
> but unsigned is always 32bits. Although it probably isn't an issue here (count and
> ndx), its something to keep in mind for future code.

Yes, better style to use size_t here. Also, we normally prefer “i” over “ndx”. And I would call it size rather than count, just to make it clearer that it just reflects the size function result.

> WebCore/platform/graphics/MediaPlayer.cpp:264
> +        if (current) {
> +            if (current == engines[ndx])
> +                current = 0;
> +            continue;
> +        }
> +        engine = engines[ndx];
> +        break;

The logic here is a little convoluted. You could write this more clearly by just saying “if (!current) return first" and for the non-zero case using the find function that vectors have without writing out a loop.

[Eric Carlson]

http://trac.webkit.org/changeset/74118