Bug 31990 – [GTK] Recognize oga as audio/ogg

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

GStreamer advertizes oga files as audio/x-vorbis. Our player should handle this case and replace audio/x-vorbis with audio/ogg where appropriate.

[Philippe Normand]

Created attachment 44034 [details]
advertize audio/ogg correctly

[Adam Barth]

style-queue ran check-webkit-style on attachment 44034 [details] without any errors.

[Eric Seidel (no email)]

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.

[Philippe Normand]

(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.

[Philippe Normand]

Created attachment 44059 [details]
advertize audio/ogg correctly

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44059 [details] without any errors.

[Eric Seidel (no email)]

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?

[Philippe Normand]

(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 :(

[Gustavo Noronha (kov)]

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.

[Gustavo Noronha (kov)]

(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.

[Philippe Normand]

Created attachment 44236 [details]
advertize audio/ogg correctly and refactored mime-type cache building

[Philippe Normand]

Created attachment 44237 [details]
new test for ogg containers

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44236 [details] without any errors.

[Philippe Normand]

Created attachment 44240 [details]
advertize audio/ogg correctly and refactored mime-type cache building

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44240 [details] without any errors.

[Philippe Normand]

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.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44246 [details] without any errors.

[Gustavo Noronha (kov)]

Comment on attachment 44246 [details]
advertize audio/ogg correctly and refactored mime-type cache building

OK, LGTM, thanks!

[WebKit Commit Bot]

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

[Gustavo Noronha (kov)]

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.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

I'm concerned about the bogus websocket failure.  I'll investigate.

[Eric Seidel (no email)]

I believe the bogus websocket failure was just bug 30098.

[Gustavo Noronha (kov)]

Landed as r51734.

[Alexey Proskuryakov]

The test wasn't landed in r51734.

[Philippe Normand]

(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!

[Gustavo Noronha (kov)]

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 =(.