GStreamer advertizes oga files as audio/x-vorbis. Our player should handle this case and replace audio/x-vorbis with audio/ogg where appropriate.
Created attachment 44034 [details] advertize audio/ogg correctly
style-queue ran check-webkit-style on attachment 44034 [details] without any errors.
The patch looks for "video/x-vorbis" instead of autio/x-vorbis. Was that intentional? Is the original bug comment simply wrong? I'm confused.
(In reply to comment #3) > The patch looks for "video/x-vorbis" instead of autio/x-vorbis. Was that > intentional? Is the original bug comment simply wrong? I'm confused. Sorry, indeed the patch is wrong, will attach a new version.
Created attachment 44059 [details] advertize audio/ogg correctly
style-queue ran check-webkit-style on attachment 44059 [details] without any errors.
How do we test this and how did you test this? How do we know that we aren't missing other mappings here? Should we assert in some way that we've mapped all of these capabilities to their web equivalents, or should that not be necessary?
(In reply to comment #7) > How do we test this and how did you test this? How do we know that we aren't > missing other mappings here? Should we assert in some way that we've mapped > all of these capabilities to their web equivalents, or should that not be > necessary? Well I can update media/video-can-play-type.html and add checks for audio/ogg and video/ogg but I'm afraid this would fail on mac for instance. Generally this depends on the platform and if codecs are installed for the given mime-type, it's not a trivial scenario. So I'm not sure what to do exactly :(
Comment on attachment 44059 [details] advertize audio/ogg correctly (In reply to comment #8) > (In reply to comment #7) > Well I can update media/video-can-play-type.html and add checks for audio/ogg > and video/ogg but I'm afraid this would fail on mac for instance. Generally > this depends on the platform and if codecs are installed for the given > mime-type, it's not a trivial scenario. I have a number of recomendations regarding this: First of all, I think we should refactor this code following this recommendation: Nov 05 14:32:29 <slomo> kov: get the GstStructure(s) from them, use the gst_structure_* API :) Perhaps you want to write that patch? As for how to test, I would recommend adding a separate test similar to media/video-can-play-type.html, and add it to the Skipped file for all platforms that are not supposed to pass it right now. I'll say r- for now.
(In reply to comment #9) > Nov 05 14:32:29 <slomo> kov: get the GstStructure(s) from them, use the > gst_structure_* API :) Just to make it clear, this was related to the fact that the current code parses the caps using string manipulation. I didn't know we had a smarter way of doing it back then. Manipulating GstStructure objects should be saner.
Created attachment 44236 [details] advertize audio/ogg correctly and refactored mime-type cache building
Created attachment 44237 [details] new test for ogg containers
style-queue ran check-webkit-style on attachment 44236 [details] without any errors.
Created attachment 44240 [details] advertize audio/ogg correctly and refactored mime-type cache building
style-queue ran check-webkit-style on attachment 44240 [details] without any errors.
Created attachment 44246 [details] advertize audio/ogg correctly and refactored mime-type cache building explicitely add audio/mpeg in the cache as suggested by Gustavo.
style-queue ran check-webkit-style on attachment 44246 [details] without any errors.
Comment on attachment 44246 [details] advertize audio/ogg correctly and refactored mime-type cache building OK, LGTM, thanks!
Comment on attachment 44246 [details] advertize audio/ogg correctly and refactored mime-type cache building Rejecting patch 44246 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11718 test cases. websocket/tests/simple.html -> failed Exiting early after 1 failures. 11713 tests run. 371.98s total testing time 11712 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 6 test cases (<1%) had stderr output
Comment on attachment 44246 [details] advertize audio/ogg correctly and refactored mime-type cache building Failure not related to the patch/test, so I'm setting cq+ again.
Comment on attachment 44246 [details] advertize audio/ogg correctly and refactored mime-type cache building Rejecting patch 44246 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Gustavo Noronha Silva', '--force']" exit_code: 1 Last 500 characters of output: kipped patching file LayoutTests/platform/mac/Skipped patching file LayoutTests/platform/qt-mac/Skipped patching file LayoutTests/platform/qt-win/Skipped patching file LayoutTests/platform/qt/Skipped patching file LayoutTests/platform/win/Skipped Hunk #1 FAILED at 722. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/win/Skipped.rej patching file WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp
I'm concerned about the bogus websocket failure. I'll investigate.
I believe the bogus websocket failure was just bug 30098.
Landed as r51734.
The test wasn't landed in r51734.
(In reply to comment #25) > The test wasn't landed in r51734. Gustavo, can you commit the html test file please? It is in the patch you reviewed, thanks!
Just saw the replies on the bug now. I had forgotten to add the files, but ap pinged me on IRC, so I landed them, sorry for the trouble =(.