[GStreamer] Add Opus MIME type to the list of supported ones
GStreamer has support for the Opus codec (http://opus-codec.org) by using a plugin present in the “gst-plugins-bad” repository. Opus streams may be embedded in a Ogg container, or standalone. The case of the Ogg container is already covered by the “*/ogg” MIME types declared as supported by the GStreamer media playing code, but for standalone streams to work, “audio/opus“ has to be added.
Created attachment 167758 [details] Patch
Attachment 167758 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1 Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1607: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 167760 [details] Patch
Comment on attachment 167760 [details] Patch Attachment 167760 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14159048 New failing tests: media/media-can-play-ogg.html
(In reply to comment #5) > (From update of attachment 167760 [details]) > Attachment 167760 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/14159048 > > New failing tests: > media/media-can-play-ogg.html Mmhh, This test case is expected to fail in all the ports which do not use GStreamer —at least while they do not add Opus support— so for the moment I am moving it to the gtk-specific tests directory and avoid breaking the bots for the rest of the ports.
I think the correct way to proceed further would be to update the platform baselines of the test for non-gstreamer-based ports (eg, all but gtk, efl, qt-linux).
Comment on attachment 167760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167760&action=review > Source/WebCore/ChangeLog:11 > + of the Ogg container is already covered by the â*/oggâ MIME types Is there some encoding issue there? > Source/WebCore/ChangeLog:13 > + standalone streams to work, âaudio/opusâ has to be added. Ditto!
Comment on attachment 167760 [details] Patch r- for red EWS
(In reply to comment #8) > (From update of attachment 167760 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167760&action=review > > > Source/WebCore/ChangeLog:11 > > + of the Ogg container is already covered by the â*/oggâ MIME types > > Is there some encoding issue there? > > > Source/WebCore/ChangeLog:13 > > + standalone streams to work, âaudio/opusâ has to be added. > > Ditto! Ouch, this is because I am used to write typographic quotes (“”) with the keyboard, I will change them to normal ASCII double quotes.
(In reply to comment #7) > I think the correct way to proceed further would be to update the platform baselines of the test for non-gstreamer-based ports (eg, all but gtk, efl, qt-linux). Okay, will update the patch updating the baselines for the non-gstreamer-based ports as suggested. Thanks for the tip.
Comment on attachment 167760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167760&action=review >>> Source/WebCore/ChangeLog:11 >>> + of the Ogg container is already covered by the â*/oggâ MIME types >> >> Is there some encoding issue there? > > Ouch, this is because I am used to write typographic quotes (“”) with the keyboard, I will change them to normal ASCII double quotes. I think it might actually be an issue with the review tool. It doesn't display contents in UTF-8.
Created attachment 182122 [details] Patch
Patch looks good, before landing it'd be good to check the opus gst plugin is built in the bots, meaning ensuring libopus-dev is installed, and document this in the wiki too :)
Comment on attachment 182122 [details] Patch Attachment 182122 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15766843 New failing tests: media/media-can-play-ogg.html
opus plugin is not built on EFL build bots and libopus-dev is not available on Ubuntu 12.04 (only 12.10). This test should be skipped for EFL until we add libopus to jhbuild I guess.
Comment on attachment 182122 [details] Patch Attachment 182122 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15775772
Comment on attachment 182122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182122&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-1606 > - const char* mimeTypes[] = {"application/ogg", > - "application/vnd.apple.mpegurl", > - "application/vnd.rn-realmedia", > - "application/x-3gp", Adding/changing type(s) AND changing the indentation in the same patch is unfortunate because it makes it very difficult to see what changed.
Well the ChangeLog mentions what was added, so I guess it's ok?
(In reply to comment #19) > Well the ChangeLog mentions what was added, so I guess it's ok? It is fine for someone looking at the log for the whole commit, but someone in the future just comparing file revisions isn't likely to be so happy.
(In reply to comment #20) > (In reply to comment #19) > > Well the ChangeLog mentions what was added, so I guess it's ok? > > It is fine for someone looking at the log for the whole commit, but someone in the future just comparing file revisions isn't likely to be so happy. IOW, I would prefer to see the cleanup done in a separate patch.
Comment on attachment 182122 [details] Patch Alright I agree with Eric, can the patch be splitted please? It'd be great to figure out the cr-linux EWS failure as well.
(In reply to comment #22) > (From update of attachment 182122 [details]) > Alright I agree with Eric, can the patch be splitted please? It'd be great to figure out the cr-linux EWS failure as well. Indeed splitting in two will make the history much cleaner. I will be sending another path fixing with the indentation, and then come back to this bug. Thanks!
Created attachment 182766 [details] Patch
Created attachment 182768 [details] Patch
Christophe, how about our bots, are they covered?
Updated patch: * Added failure expectation for Chromium, should fix the red EWS. * Added failure expectation for EFL, so its bots will not be red, should be changed once the bots have libopus installed. As an additional note, at least the 64-bit GTK bots already have libopus and the GStreamer plugin installed.
Created attachment 182777 [details] Patch
Comment on attachment 182777 [details] Patch Attachment 182777 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15884623 New failing tests: media/media-can-play-ogg.html
Hi Andrew, would you have some time to try this patch on Chromium? It seems the updated test fails but the EWS doesn't provide test diffs anymore (why is that BTW?) Any help would be appreciated!
Here's the diff for Chromium: --- /src/chrome/src/webkit/Debug/layout-test-results/media/media-can-play-ogg-expected.txt +++ /src/chrome/src/webkit/Debug/layout-test-results/media/media-can-play-ogg-actual.txt @@ -5,7 +5,7 @@ EXPECTED (video.canPlayType('audio/ogg') == 'maybe') OK EXPECTED (video.canPlayType('video/ogg') == 'maybe') OK EXPECTED (video.canPlayType('audio/ogg; codecs=vorbis') == 'probably') OK -EXPECTED (video.canPlayType('audio/ogg; codecs=opus') == 'probably'), OBSERVED '' FAIL +EXPECTED (video.canPlayType('audio/ogg; codecs=opus') == 'probably'), OBSERVED 'maybe' FAIL EXPECTED (video.canPlayType('video/ogg; codecs=theora') == 'probably') OK EXPECTED (video.canPlayType('video/ogg; codecs=theora,vorbis') == 'probably') OK END OF TEST We have preliminary Opus support but we haven't gotten it completely hooked up :) Thanks for the layout tests!
Thank you Andrew :)
(In reply to comment #31) > Here's the diff for Chromium: > [...] > > We have preliminary Opus support but we haven't gotten it completely hooked up :) > > Thanks for the layout tests! ...and thank you for the Chromium fix. I will re-upload an updated version of the patch, and now that Gustavo installed the dependencies in the GTK+ 32bit bot it should be safe to land this :)
Created attachment 183781 [details] Patch
Comment on attachment 183781 [details] Patch Attachment 183781 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16036092 New failing tests: media/media-can-play-ogg.html
(In reply to comment #35) > (From update of attachment 183781 [details]) > Attachment 183781 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/16036092 > > New failing tests: > media/media-can-play-ogg.html Still?!!
Comment on attachment 183781 [details] Patch Well... I'd say we land this and let the Chromium gardeners deal with their bots :)
Comment on attachment 183781 [details] Patch Clearing flags on attachment: 183781 Committed r140532: <http://trac.webkit.org/changeset/140532>
All reviewed patches have been landed. Closing bug.