Bug 31990 - [GTK] Recognize oga as audio/ogg
Summary: [GTK] Recognize oga as audio/ogg
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-30 11:54 PST by Philippe Normand
Modified: 2009-12-09 05:54 PST (History)
5 users (show)

See Also:


Attachments
advertize audio/ogg correctly (1.69 KB, patch)
2009-11-30 11:58 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
advertize audio/ogg correctly (1.69 KB, patch)
2009-12-01 00:25 PST, Philippe Normand
gns: review-
Details | Formatted Diff | Diff
advertize audio/ogg correctly and refactored mime-type cache building (6.95 KB, patch)
2009-12-03 06:26 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
new test for ogg containers (6.32 KB, patch)
2009-12-03 06:26 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
advertize audio/ogg correctly and refactored mime-type cache building (13.27 KB, patch)
2009-12-03 07:11 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
advertize audio/ogg correctly and refactored mime-type cache building (13.35 KB, patch)
2009-12-03 08:57 PST, Philippe Normand
gns: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2009-11-30 11:54:42 PST
GStreamer advertizes oga files as audio/x-vorbis. Our player should handle this case and replace audio/x-vorbis with audio/ogg where appropriate.
Comment 1 Philippe Normand 2009-11-30 11:58:05 PST
Created attachment 44034 [details]
advertize audio/ogg correctly
Comment 2 Adam Barth 2009-11-30 12:53:02 PST
style-queue ran check-webkit-style on attachment 44034 [details] without any errors.
Comment 3 Eric Seidel (no email) 2009-11-30 21:45:14 PST
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.
Comment 4 Philippe Normand 2009-12-01 00:00:15 PST
(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.
Comment 5 Philippe Normand 2009-12-01 00:25:06 PST
Created attachment 44059 [details]
advertize audio/ogg correctly
Comment 6 WebKit Review Bot 2009-12-01 00:28:19 PST
style-queue ran check-webkit-style on attachment 44059 [details] without any errors.
Comment 7 Eric Seidel (no email) 2009-12-01 07:10:47 PST
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?
Comment 8 Philippe Normand 2009-12-01 10:18:44 PST
(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 9 Gustavo Noronha (kov) 2009-12-02 10:15:06 PST
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.
Comment 10 Gustavo Noronha (kov) 2009-12-02 10:16:21 PST
(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.
Comment 11 Philippe Normand 2009-12-03 06:26:36 PST
Created attachment 44236 [details]
advertize audio/ogg correctly and refactored mime-type cache building
Comment 12 Philippe Normand 2009-12-03 06:26:50 PST
Created attachment 44237 [details]
new test for ogg containers
Comment 13 WebKit Review Bot 2009-12-03 06:28:10 PST
style-queue ran check-webkit-style on attachment 44236 [details] without any errors.
Comment 14 Philippe Normand 2009-12-03 07:11:18 PST
Created attachment 44240 [details]
advertize audio/ogg correctly and refactored mime-type cache building
Comment 15 WebKit Review Bot 2009-12-03 07:14:55 PST
style-queue ran check-webkit-style on attachment 44240 [details] without any errors.
Comment 16 Philippe Normand 2009-12-03 08:57:49 PST
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.
Comment 17 WebKit Review Bot 2009-12-03 08:58:16 PST
style-queue ran check-webkit-style on attachment 44246 [details] without any errors.
Comment 18 Gustavo Noronha (kov) 2009-12-03 09:03:42 PST
Comment on attachment 44246 [details]
advertize audio/ogg correctly and refactored mime-type cache building

OK, LGTM, thanks!
Comment 19 WebKit Commit Bot 2009-12-03 13:30:18 PST
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 20 Gustavo Noronha (kov) 2009-12-03 13:35:27 PST
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 21 WebKit Commit Bot 2009-12-03 13:37:59 PST
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
Comment 22 Eric Seidel (no email) 2009-12-03 13:45:10 PST
I'm concerned about the bogus websocket failure.  I'll investigate.
Comment 23 Eric Seidel (no email) 2009-12-03 15:09:42 PST
I believe the bogus websocket failure was just bug 30098.
Comment 24 Gustavo Noronha (kov) 2009-12-05 17:50:43 PST
Landed as r51734.
Comment 25 Alexey Proskuryakov 2009-12-07 11:00:17 PST
The test wasn't landed in r51734.
Comment 26 Philippe Normand 2009-12-08 07:15:17 PST
(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!
Comment 27 Gustavo Noronha (kov) 2009-12-09 05:54:06 PST
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 =(.