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
Updated patch (8.57 KB, patch)
2010-12-08 09:03 PST, Eric Carlson
no flags
Patch that might compile (8.78 KB, patch)
2010-12-09 07:40 PST, Eric Carlson
darin: review+
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
Early Warning System Bot
Comment 4 2010-12-08 12:27:55 PST
WebKit Review Bot
Comment 5 2010-12-08 12:40:29 PST
Build Bot
Comment 6 2010-12-08 13:45:27 PST
Eric Seidel (no email)
Comment 7 2010-12-08 18:11:34 PST
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
Note You need to log in before you can comment on or make changes to this bug.