Bug 98759 – [GStreamer] Add Opus MIME type to the list of supported ones

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adrian Perez]

[GStreamer] Add Opus MIME type to the list of supported ones

[Adrian Perez]

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.

[Adrian Perez]

Created attachment 167758 [details]
Patch

[WebKit Review Bot]

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.

[Adrian Perez]

Created attachment 167760 [details]
Patch

[WebKit Review Bot]

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

[Adrian Perez]

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

[Philippe Normand]

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

[Philippe Normand]

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!

[Philippe Normand]

Comment on attachment 167760 [details]
Patch

r- for red EWS

[Adrian Perez]

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

[Adrian Perez]

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

[Martin Robinson]

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.

[Adrian Perez]

Created attachment 182122 [details]
Patch

[Philippe Normand]

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

[WebKit Review Bot]

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

[Chris Dumez]

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.

[EFL EWS Bot]

Comment on attachment 182122 [details]
Patch

Attachment 182122 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15775772

[Eric Carlson]

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.

[Philippe Normand]

Well the ChangeLog mentions what was added, so I guess it's ok?

[Eric Carlson]

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

[Eric Carlson]

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

[Philippe Normand]

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.

[Adrian Perez]

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

[Adrian Perez]

Created attachment 182766 [details]
Patch

[Adrian Perez]

Created attachment 182768 [details]
Patch

[Dominik Röttsches (drott)]

Christophe, how about our bots, are they covered?

[Adrian Perez]

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.

[Adrian Perez]

Created attachment 182777 [details]
Patch

[WebKit Review Bot]

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

[Philippe Normand]

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!

[Andrew Scherkus]

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!

[Philippe Normand]

Thank you Andrew :)

[Adrian Perez]

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

[Adrian Perez]

Created attachment 183781 [details]
Patch

[WebKit Review Bot]

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

[Philippe Normand]

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

[Philippe Normand]

Comment on attachment 183781 [details]
Patch

Well... I'd say we land this and let the Chromium gardeners deal with their bots :)

[Philippe Normand]

Comment on attachment 183781 [details]
Patch

Clearing flags on attachment: 183781

Committed r140532: <http://trac.webkit.org/changeset/140532>

[Philippe Normand]

All reviewed patches have been landed.  Closing bug.