Bug 98759

Summary: [GStreamer] Add Opus MIME type to the list of supported ones
Product: WebKit Reporter: Adrian Perez <aperez>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cgarcia, dglazkov, d-r, eric.carlson, feature-media-reviews, gns, gyuyoung.kim, menard, mifenton, mrobinson, pnormand, rakuco, rwlbuis, scherkus, tonikitoo, webkit.review.bot, xan.lopez, yong.li.webkit, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 106812    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Adrian Perez 2012-10-09 08:04:23 PDT
[GStreamer] Add Opus MIME type to the list of supported ones
Comment 1 Adrian Perez 2012-10-09 08:07:25 PDT
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.
Comment 2 Adrian Perez 2012-10-09 08:13:25 PDT
Created attachment 167758 [details]
Patch
Comment 3 WebKit Review Bot 2012-10-09 08:18:03 PDT
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.
Comment 4 Adrian Perez 2012-10-09 08:34:55 PDT
Created attachment 167760 [details]
Patch
Comment 5 WebKit Review Bot 2012-10-09 09:20:56 PDT
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
Comment 6 Adrian Perez 2012-10-12 02:18:32 PDT
(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.
Comment 7 Philippe Normand 2012-10-14 23:59:46 PDT
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 8 Philippe Normand 2012-10-15 03:19:07 PDT
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 9 Philippe Normand 2012-11-27 00:27:21 PST
Comment on attachment 167760 [details]
Patch

r- for red EWS
Comment 10 Adrian Perez 2013-01-10 03:53:18 PST
(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.
Comment 11 Adrian Perez 2013-01-10 03:54:07 PST
(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 12 Martin Robinson 2013-01-10 05:19:44 PST
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.
Comment 13 Adrian Perez 2013-01-10 05:55:50 PST
Created attachment 182122 [details]
Patch
Comment 14 Philippe Normand 2013-01-10 06:12:24 PST
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 15 WebKit Review Bot 2013-01-10 06:55:41 PST
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
Comment 16 Chris Dumez 2013-01-10 07:04:34 PST
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 17 EFL EWS Bot 2013-01-10 07:33:27 PST
Comment on attachment 182122 [details]
Patch

Attachment 182122 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15775772
Comment 18 Eric Carlson 2013-01-10 07:34:44 PST
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.
Comment 19 Philippe Normand 2013-01-10 07:42:25 PST
Well the ChangeLog mentions what was added, so I guess it's ok?
Comment 20 Eric Carlson 2013-01-10 07:48:36 PST
(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.
Comment 21 Eric Carlson 2013-01-10 07:49:24 PST
(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 22 Philippe Normand 2013-01-11 03:56:07 PST
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.
Comment 23 Adrian Perez 2013-01-13 15:03:54 PST
(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!
Comment 24 Adrian Perez 2013-01-15 07:25:11 PST
Created attachment 182766 [details]
Patch
Comment 25 Adrian Perez 2013-01-15 07:30:08 PST
Created attachment 182768 [details]
Patch
Comment 26 Dominik Röttsches (drott) 2013-01-15 07:53:59 PST
Christophe, how about our bots, are they covered?
Comment 27 Adrian Perez 2013-01-15 08:02:46 PST
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.
Comment 28 Adrian Perez 2013-01-15 08:08:50 PST
Created attachment 182777 [details]
Patch
Comment 29 WebKit Review Bot 2013-01-15 08:48:45 PST
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
Comment 30 Philippe Normand 2013-01-15 13:43:44 PST
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!
Comment 31 Andrew Scherkus 2013-01-17 09:53:01 PST
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!
Comment 32 Philippe Normand 2013-01-18 00:20:31 PST
Thank you Andrew :)
Comment 33 Adrian Perez 2013-01-21 00:33:20 PST
(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 :)
Comment 34 Adrian Perez 2013-01-21 07:12:05 PST
Created attachment 183781 [details]
Patch
Comment 35 WebKit Review Bot 2013-01-21 07:51:53 PST
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
Comment 36 Philippe Normand 2013-01-21 08:01:35 PST
(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 37 Philippe Normand 2013-01-22 06:16:06 PST
Comment on attachment 183781 [details]
Patch

Well... I'd say we land this and let the Chromium gardeners deal with their bots :)
Comment 38 Philippe Normand 2013-01-23 04:05:52 PST
Comment on attachment 183781 [details]
Patch

Clearing flags on attachment: 183781

Committed r140532: <http://trac.webkit.org/changeset/140532>
Comment 39 Philippe Normand 2013-01-23 04:06:01 PST
All reviewed patches have been landed.  Closing bug.