Bug 78244 - [GStreamer] html5test.com says that gstreamer ports do not support WebM for audio
: [GStreamer] html5test.com says that gstreamer ports do not support WebM for a...
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC Linux
: P3 Normal
Assigned To:
: http://html5test.com
:
:
: 40829
  Show dependency treegraph
 
Reported: 2012-02-09 08:39 PST by
Modified: 2012-02-14 05:07 PST (History)


Attachments
Patch (1.68 KB, patch)
2012-02-09 09:55 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (2.43 KB, patch)
2012-02-11 08:43 PST, Martin Robinson
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch for landing, for real (2.43 KB, patch)
2012-02-11 09:06 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-02-09 08:39:45 PST
html5test.com says that we support WebM for video, but not audio. Philippe has confirmed that we just need to advertise a supporting another mime type.
------- Comment #1 From 2012-02-09 09:55:14 PST -------
Created an attachment (id=126320) [details]
Patch
------- Comment #2 From 2012-02-09 10:03:44 PST -------
(From update of attachment 126320 [details])
OK... webm isnt' a compile-time option for gstreamer?
------- Comment #3 From 2012-02-09 10:04:06 PST -------
Did you file a bug with gstreamer to fix their advertising?  Can we remove this once they fix such?
------- Comment #4 From 2012-02-09 10:08:41 PST -------
(In reply to comment #3)
> Did you file a bug with gstreamer to fix their advertising?  Can we remove this once they fix such?

Thanks for the review!

Philippe, who typically handles the gstreamer integration, has asked upstream about fixing this. I guess it's kicked off an interesting discussion, but I don't have the details. One thing preventing us from removing the work-around is that we still want to work with older gstreamer versions. At some point, when our dependency jumps (likely when we drop support for pre-0.11 GStreamer) we can remove this work-around.
------- Comment #5 From 2012-02-09 10:10:29 PST -------
(From update of attachment 126320 [details])
It would be nice if you would explain that in the code. :)  Ideally by linking to some gstreamer bug url (so that later hackers can know when to remove this).
------- Comment #6 From 2012-02-09 10:13:46 PST -------
(In reply to comment #2)
> (From update of attachment 126320 [details] [details])
> OK... webm isnt' a compile-time option for gstreamer?

WebM support is shipped in separate plugins but the core of gstreamer provides what they call typefinders, able to detect the media container of a given input byte stream.

(In reply to comment #3)
> Did you file a bug with gstreamer to fix their advertising?  Can we remove this once they fix such?

Well the caps infrastructure used in gstreamer is different from the mime-types used in this code.

In the future they might consider advertizing audio/webm but for the time being we need this workaround in webkit.
------- Comment #7 From 2012-02-09 10:19:58 PST -------
You all do what you need to do. :)  My concerns are: 1.  Making sure that your'e not inadverntaantly advertising support when you don't hav eit.  2.  Adding code which will be obsolute (or worse, wrong) at a later date, w/o any comments in the code as to when to remove it. :)

But those are general concerns.  This is a pretty simple patch, and you all are both quite experianced at this webkit thing.  I trust you do do the right things here (whatever those are).
------- Comment #8 From 2012-02-09 10:21:17 PST -------
(In reply to comment #5)
> (From update of attachment 126320 [details] [details])
> It would be nice if you would explain that in the code. :)  Ideally by linking to some gstreamer bug url (so that later hackers can know when to remove this).

Good point. Philippe, is the following comment accurate?

// There isn't a one-to-one correspondance of caps to supported mime types in GStreamer.
// Thus, we need to manually map from caps to supported mime-types here. At some point in
// the future, GStreamer may map caps to mime-types directly and then we can remove this code.

I didn't link to the the bug URL, because I'm not sure if this is considered a bug in upstream yet. Perhaps Philippe can help me out here...

I also notice that the video/x-h264 cap doesn't add a video/x-h264 mime-type and neither does the video/x-theora add a video/x-theora mime type. Is that on purpose?
------- Comment #9 From 2012-02-09 10:23:30 PST -------
(In reply to comment #7)

> 1.  Making sure that your'e not inadverntaantly advertising support when you don't hav eit. 

At least in this case, it seems that the audio/x-vorbis cap implies audio/webm mime type support.

> 2.  Adding code which will be obsolute (or worse, wrong) at a later date, w/o any comments in the code as to when to remove it. :)

I definitely think it makes sense to add a comment here explaining the situation.
------- Comment #10 From 2012-02-10 00:30:25 PST -------
(In reply to comment #8)
> (In reply to comment #5)
> > (From update of attachment 126320 [details] [details] [details])
> > It would be nice if you would explain that in the code. :)  Ideally by linking to some gstreamer bug url (so that later hackers can know when to remove this).
> 
> Good point. Philippe, is the following comment accurate?
> 
> // There isn't a one-to-one correspondance of caps to supported mime types in GStreamer.
> // Thus, we need to manually map from caps to supported mime-types here. At some point in
> // the future, GStreamer may map caps to mime-types directly and then we can remove this code.
> 

It's ok apart from the last sentence. Here's my take :)
At some point in the future, GStreamer may reduce the differences between some caps and some of the mime-types mentionned in this code.

> I didn't link to the the bug URL, because I'm not sure if this is considered a bug in upstream yet. Perhaps Philippe can help me out here...
> 

It's not considered a bug yet. I think if I open one now it will probably be quickly closed as INVALID :) But don't worry, I'm aware of this issue and eager to simplify this mimeTypeCache() function as soon as we can.

> I also notice that the video/x-h264 cap doesn't add a video/x-h264 mime-type and neither does the video/x-theora add a video/x-theora mime type. Is that on purpose?

Yes, I don't think it's necessary. For instance the media/W3C/video/canPlayType tests use video/mp4 and video/ogg.
------- Comment #11 From 2012-02-11 08:43:41 PST -------
Created an attachment (id=126635) [details]
Patch for landing
------- Comment #12 From 2012-02-11 08:52:16 PST -------
(From update of attachment 126635 [details])
Rejecting attachment 126635 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11506144
------- Comment #13 From 2012-02-11 09:06:46 PST -------
Created an attachment (id=126636) [details]
Patch for landing, for real
------- Comment #14 From 2012-02-11 10:36:52 PST -------
(From update of attachment 126636 [details])
Clearing flags on attachment: 126636

Committed r107482: <http://trac.webkit.org/changeset/107482>
------- Comment #15 From 2012-02-11 18:04:37 PST -------
(From update of attachment 126320 [details])
Clearing review bit.