GNOME Bugzilla – Bug 737810
payloaders: VP8 and Opus payloader should probably suppport Google Chrome encoding-names
Last modified: 2015-02-19 14:31:06 UTC
While implement WebRTC client that speak with Google Chrome, I found that encoding-name for draft payload RFC won't match the one in GStreamer. For VP8, Google uses "VP8", where GStreamer uses "VP8-DRAFT-IETF-01". For opus, Google uses "opus" where GStreamer uses "X-GST-OPUS-DRAFT-SPITTKA-00". In order to inter-operate better, I suggest we could add these two "well known" names to our payloader / depayloaders. This would avoid having to implement codec specific hacks in when parsing codecs in SDP. Let's me know if this is acceptable, if so I'll provide patches. This is likely to help inter-operability on RTSP side too, as the parser there does not have any of such hacks.
Do it, and the VP8/OPUS ones should be the default
(In reply to comment #1) > Do it, and the VP8/OPUS ones should be the default ok, note it's small case "opus" being used, I was wondering if this was an odd choice from google, or if it should not be case sensitive, and I should simply do a toupper() on that ?
In SDP, the names are not case sensitive. in GStreamer they are, so the convention is to put them in ALL CAPITALS
Ok, great.
Created attachment 289782 [details] [review] [PATCH] rtpopus: Use OPUS encoding name Both Firefox and Chrome uses OPUS as the encoding in their SDP. Adding this now defacto standard name remove the need for special case in SDP parsing code. https://bugzilla.gnome.org/show_bug.cgi?id=73781 --- ext/opus/gstrtpopusdepay.c | 2 +- ext/opus/gstrtpopuspay.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Created attachment 289783 [details] [review] [PATCH] rtpvp8: Use VP8 encoding name Both Firefox and Chrome uses VP8 as the encoding in their SDP. Adding this now defacto standard name removes the need for special case in SDP parsing code. https://bugzilla.gnome.org/show_bug.cgi?id=73781 --- gst/rtp/gstrtpvp8depay.c | 2 +- gst/rtp/gstrtpvp8pay.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Review of attachment 289782 [details] [review]: Wrong bug number in the commit message.. Otherwise please merge.
Ah, oops, missing a zero, must have truncated it when copy-paste, will fix and merge.
That is really weird, the bug number is right in my git trees, something is broken in git-send-bugzilla, time to setup git-bz I guess.
Comment on attachment 289782 [details] [review] [PATCH] rtpopus: Use OPUS encoding name Actually, the bug number is right the patch itself. commit 0f4f948f5feba69336772c4234069b3fba9ebf6e Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Sat Nov 1 10:14:31 2014 -0400 rtpvp8: Use VP8 encoding name Both Firefox and Chrome uses VP8 as the encoding in their SDP. Adding this now defacto standard name removes the need for special case in SDP parsing code. https://bugzilla.gnome.org/show_bug.cgi?id=737810
Comment on attachment 289783 [details] [review] [PATCH] rtpvp8: Use VP8 encoding name commit 0f4f948f5feba69336772c4234069b3fba9ebf6e Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Sat Nov 1 10:14:31 2014 -0400 rtpvp8: Use VP8 encoding name Both Firefox and Chrome uses VP8 as the encoding in their SDP. Adding this now defacto standard name removes the need for special case in SDP parsing code. https://bugzilla.gnome.org/show_bug.cgi?id=73781
Oops, OPUS one is: commit 54602c04f6e1769d825e2152c7f42150c4e7dad5 Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Sat Nov 1 10:10:27 2014 -0400 rtpopus: Use OPUS encoding name Both Firefox and Chrome uses OPUS as the encoding in their SDP. Adding this now defacto standard name remove the need for special case in SDP parsing code. https://bugzilla.gnome.org/show_bug.cgi?id=737810
Review of attachment 289782 [details] [review]: ::: ext/opus/gstrtpopuspay.c @@ +49,3 @@ "payload = (int) " GST_RTP_PAYLOAD_DYNAMIC_STRING ", " "clock-rate = (int) 48000, " + "encoding-name = (string) { \"OPUS\", \"X-GST-OPUS-DRAFT-SPITTKA-00\" }") Doesn't this require code inside the payloader subclass to properly negotiate the encoding-name? Or is that magically handled by the baseclass (by just calling gst_caps_fixate() if after intersection with downstream there are still multiple options, which then effectively always uses OPUS)?
Hmm, good point, specially with code like this: static gboolean gst_rtp_vp8_pay_set_caps (GstRTPBasePayload * payload, GstCaps * caps) { gst_rtp_base_payload_set_options (payload, "video", TRUE, "VP8-DRAFT-IETF-01", 90000); return gst_rtp_base_payload_set_outcaps (payload, NULL); }
I'll be looking into this as soon as I find the time. Meanwhile note that farstream miss-behave with encoding-name being a set. With these two patches, both opus and vp8 codecs are now gone from Farstream codec list. I suspect spliting the caps in two structure would work, seems like a limitation in farstream.
This currently breaks farstream and rtsp. The payloader base class should do something more clever with negotiation here, and also allow subclasses to override it.
Created attachment 296178 [details] [review] negotiate vp8 rtp encoding name
Created attachment 296179 [details] [review] negotiate opus rtp encoding name
I just pushed those two as they seem good and no comments were forthcoming. I think there are no leftover issues for this bug ? commit b866c989f5edf6439bd4d0dd0e58e9a5cdd9c511 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Thu Feb 5 10:29:26 2015 +0000 rtpvp8pay: negotiate encoding name Chrome uses a different one than gstreamer. https://bugzilla.gnome.org/show_bug.cgi?id=737810 commit 3be7d0748a8758b52d7e5804c2e91d0637316cfb Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Thu Feb 5 10:27:51 2015 +0000 rtpopuspay: negotiate the encoding name Chrome uses a different encoding name that gstreamer. https://bugzilla.gnome.org/show_bug.cgi?id=737810
First call make_writable(), then truncate() :) Otherwise looks good and close this bug after fixing these issues.
Sorry, forgot about that one, I wanted to add that the Chrome one should be the default, not the GStreamer specific ones. This is to fix RTSP interop.
Eagle eye, fixed the ordering.
Yes, that too... even if it leaves a little backwards compatibility breakage hole :)
(In reply to Nicolas Dufresne (stormer) from comment #21) > Sorry, forgot about that one, I wanted to add that the Chrome one should be > the default, not the GStreamer specific ones. This is to fix RTSP interop. That might break backward compatibility, and if something requires the new names, it'll negotiate them, no ?
RTSP does not require a specific encoding name downstream of the payloader. It just takes whatever it gets and puts that into the SDP.
OPUS and VP8 are now the defaults.