After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 737810 - payloaders: VP8 and Opus payloader should probably suppport Google Chrome encoding-names
payloaders: VP8 and Opus payloader should probably suppport Google Chrome enc...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal blocker
: 1.5.1
Assigned To: Nicolas Dufresne (ndufresne)
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-10-02 22:27 UTC by Nicolas Dufresne (ndufresne)
Modified: 2015-02-19 14:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] rtpopus: Use OPUS encoding name (1.68 KB, patch)
2014-11-01 14:11 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] rtpvp8: Use VP8 encoding name (1.65 KB, patch)
2014-11-01 14:14 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
negotiate vp8 rtp encoding name (1.53 KB, patch)
2015-02-05 10:31 UTC, Vincent Penquerc'h
committed Details | Review
negotiate opus rtp encoding name (1.54 KB, patch)
2015-02-05 10:32 UTC, Vincent Penquerc'h
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2014-10-02 22:27:39 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.
Comment 1 Olivier Crête 2014-10-03 16:52:20 UTC
Do it, and the VP8/OPUS ones should be the default
Comment 2 Nicolas Dufresne (ndufresne) 2014-10-03 17:01:26 UTC
(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 ?
Comment 3 Olivier Crête 2014-10-03 17:11:48 UTC
In SDP, the names are not case sensitive. in GStreamer they are, so the convention is to put them in ALL CAPITALS
Comment 4 Nicolas Dufresne (ndufresne) 2014-10-03 17:41:20 UTC
Ok, great.
Comment 5 Nicolas Dufresne (ndufresne) 2014-11-01 14:11:12 UTC
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(-)
Comment 6 Nicolas Dufresne (ndufresne) 2014-11-01 14:14:54 UTC
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(-)
Comment 7 Olivier Crête 2014-11-01 14:35:51 UTC
Review of attachment 289782 [details] [review]:

Wrong bug number in the commit message.. Otherwise please merge.
Comment 8 Nicolas Dufresne (ndufresne) 2014-11-01 15:26:03 UTC
Ah, oops, missing a zero, must have truncated it when copy-paste, will fix and merge.
Comment 9 Nicolas Dufresne (ndufresne) 2014-11-01 15:28:46 UTC
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 10 Nicolas Dufresne (ndufresne) 2014-11-01 15:30:18 UTC
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 11 Nicolas Dufresne (ndufresne) 2014-11-01 15:30:56 UTC
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
Comment 12 Nicolas Dufresne (ndufresne) 2014-11-01 15:31:39 UTC
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
Comment 13 Sebastian Dröge (slomo) 2014-11-01 17:16:58 UTC
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)?
Comment 14 Nicolas Dufresne (ndufresne) 2014-11-28 18:16:48 UTC
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);
}
Comment 15 Nicolas Dufresne (ndufresne) 2014-12-14 17:04:25 UTC
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.
Comment 16 Sebastian Dröge (slomo) 2014-12-15 09:03:43 UTC
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.
Comment 17 Vincent Penquerc'h 2015-02-05 10:31:55 UTC
Created attachment 296178 [details] [review]
negotiate vp8 rtp encoding name
Comment 18 Vincent Penquerc'h 2015-02-05 10:32:24 UTC
Created attachment 296179 [details] [review]
negotiate opus rtp encoding name
Comment 19 Vincent Penquerc'h 2015-02-19 13:59:59 UTC
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
Comment 20 Sebastian Dröge (slomo) 2015-02-19 14:02:08 UTC
First call make_writable(), then truncate() :) Otherwise looks good and close this bug after fixing these issues.
Comment 21 Nicolas Dufresne (ndufresne) 2015-02-19 14:04:25 UTC
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.
Comment 22 Vincent Penquerc'h 2015-02-19 14:07:53 UTC
Eagle eye, fixed the ordering.
Comment 23 Sebastian Dröge (slomo) 2015-02-19 14:09:10 UTC
Yes, that too... even if it leaves a little backwards compatibility breakage hole :)
Comment 24 Vincent Penquerc'h 2015-02-19 14:09:25 UTC
(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 ?
Comment 25 Sebastian Dröge (slomo) 2015-02-19 14:12:05 UTC
RTSP does not require a specific encoding name downstream of the payloader. It just takes whatever it gets and puts that into the SDP.
Comment 26 Vincent Penquerc'h 2015-02-19 14:31:06 UTC
OPUS and VP8 are now the defaults.