WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
50209
MediaPlayer should try all installed media engines
https://bugs.webkit.org/show_bug.cgi?id=50209
Summary
MediaPlayer should try all installed media engines
Eric Carlson
Reported
2010-11-29 22:35:58 PST
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.
Attachments
Proposed patch
(8.21 KB, patch)
2010-11-30 17:05 PST
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Updated patch
(8.57 KB, patch)
2010-12-08 09:03 PST
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Patch that might compile
(8.78 KB, patch)
2010-12-09 07:40 PST
,
Eric Carlson
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Eric Carlson
Comment 1
2010-11-30 17:05:58 PST
Created
attachment 75233
[details]
Proposed patch
Eric Carlson
Comment 2
2010-12-08 09:03:15 PST
Created
attachment 75913
[details]
Updated patch Updated to behave correctly when retrying when there is no MIME type.
Eric Seidel (no email)
Comment 3
2010-12-08 12:24:43 PST
Attachment 75913
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/6754115
Early Warning System Bot
Comment 4
2010-12-08 12:27:55 PST
Attachment 75913
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/6719133
WebKit Review Bot
Comment 5
2010-12-08 12:40:29 PST
Attachment 75913
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/6862077
Build Bot
Comment 6
2010-12-08 13:45:27 PST
Attachment 75913
[details]
did not build on win: Build output:
http://queues.webkit.org/results/6720123
Eric Seidel (no email)
Comment 7
2010-12-08 18:11:34 PST
Attachment 75913
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/6759127
Eric Carlson
Comment 8
2010-12-09 07:40:01 PST
Created
attachment 76064
[details]
Patch that might compile
Joseph Pecoraro
Comment 9
2010-12-09 18:50:32 PST
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 10
2010-12-14 18:30:52 PST
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
Comment 11
2010-12-15 09:28:39 PST
http://trac.webkit.org/changeset/74118
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug