[GStreamer] html5test.com says that gstreamer ports do not support WebM for audio

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 126320 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 126320 [details]
Patch

OK... webm isnt' a compile-time option for gstreamer?

[Eric Seidel (no email)]

Did you file a bug with gstreamer to fix their advertising?  Can we remove this once they fix such?

[Martin Robinson]

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

[Eric Seidel (no email)]

Comment on attachment 126320 [details]
Patch

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

[Philippe Normand]

(In reply to comment #2)
> (From update of attachment 126320 [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.

[Eric Seidel (no email)]

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

[Martin Robinson]

(In reply to comment #5)
> (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).

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?

[Martin Robinson]

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

[Philippe Normand]

(In reply to comment #8)
> (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.
> 

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.

[Martin Robinson]

Created attachment 126635 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 126635 [details]
Patch for landing

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

[Martin Robinson]

Created attachment 126636 [details]
Patch for landing, for real

[WebKit Review Bot]

Comment on attachment 126636 [details]
Patch for landing, for real

Clearing flags on attachment: 126636

Committed r107482: <http://trac.webkit.org/changeset/107482>

[Martin Robinson]

Comment on attachment 126320 [details]
Patch

Clearing review bit.