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 719829 - rtp: Add RFC4571 framing/de-framing element
rtp: Add RFC4571 framing/de-framing element
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-12-04 09:55 UTC by Sebastian Dröge (slomo)
Modified: 2013-12-04 20:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
rtptcpframe: Add RFC4571 RTP TCP framing element (10.31 KB, patch)
2013-12-04 09:56 UTC, Sebastian Dröge (slomo)
rejected Details | Review
rtptcpdeframe: Add RFC4571 RTP TCP de-framing element (8.69 KB, patch)
2013-12-04 09:56 UTC, Sebastian Dröge (slomo)
rejected Details | Review
rtptcpframe: Don't copy RTP memory but just append the existing ones without copying (1.48 KB, patch)
2013-12-04 10:17 UTC, Sebastian Dröge (slomo)
rejected Details | Review

Description Sebastian Dröge (slomo) 2013-12-04 09:55:26 UTC
The attached patches add RFC4571 framing/de-framing elements: https://tools.ietf.org/html/rfc4571

Any suggestions about a better element name, and the caps handling would be great :)
Comment 1 Sebastian Dröge (slomo) 2013-12-04 09:56:30 UTC
Created attachment 263488 [details] [review]
rtptcpframe: Add RFC4571 RTP TCP framing element
Comment 2 Sebastian Dröge (slomo) 2013-12-04 09:56:42 UTC
Created attachment 263489 [details] [review]
rtptcpdeframe: Add RFC4571 RTP TCP de-framing element
Comment 3 Sebastian Dröge (slomo) 2013-12-04 10:17:23 UTC
Created attachment 263493 [details] [review]
rtptcpframe: Don't copy RTP memory but just append the existing ones without copying
Comment 4 Sebastian Dröge (slomo) 2013-12-04 13:01:23 UTC
So, question is how to call the element and the caps. This is not just for TCP but for any connection-based transport protocol. And is "framing" a good word here? The element could also be called payloader, but just putting two bytes at the front called payloading...? Or a muxer/demuxer?

For the caps and the first part of the name... instead of TCP we could also just call it like the RFC maybe? Any other ideas?


Support for this could also be added to sdpdemux.
Comment 5 Olivier Crête 2013-12-04 16:05:22 UTC
Review of attachment 263493 [details] [review]:

::: gst/rtp/gstrtptcpframe.c
@@ +170,3 @@
 
+  /* Resize to 2 bytes and append all RTP memory without copying */
+  gst_buffer_resize (outbuf, 0, 2);

Resize a buffer here doesn't make sense. Possibly not a good use-case for basetransform.
Comment 6 Olivier Crête 2013-12-04 16:10:19 UTC
Even though the RFC defines this as RTP/RTCP specific, the framing really isn't. 
I'd call them rfc4571frame and rfc4571deframe with caps application/x-rfc4571-stream and "ANY" on the other side. Possibly having application/x-rfc4571-stream, original-caps=(caps)"original caps...." for the output of the encoder. And I think they probably fit as payloader/depayloader
Comment 7 Sebastian Dröge (slomo) 2013-12-04 16:18:47 UTC
(In reply to comment #5)
> Review of attachment 263493 [details] [review]:
> 
> ::: gst/rtp/gstrtptcpframe.c
> @@ +170,3 @@
> 
> +  /* Resize to 2 bytes and append all RTP memory without copying */
> +  gst_buffer_resize (outbuf, 0, 2);
> 
> Resize a buffer here doesn't make sense. Possibly not a good use-case for
> basetransform.

True, but all other base classes don't fit at all and just going from GstElement will require a lot of code :) I'm fine with changing this to a plain GstElement if everybody agrees that this is a good idea, the cost of downsizing the buffer there and allocating it larger should be minimal though.

(In reply to comment #6)
> Even though the RFC defines this as RTP/RTCP specific, the framing really
> isn't. 
> I'd call them rfc4571frame and rfc4571deframe with caps
> application/x-rfc4571-stream and "ANY" on the other side. Possibly having
> application/x-rfc4571-stream, original-caps=(caps)"original caps...." for the
> output of the encoder. And I think they probably fit as payloader/depayloader

While it is usable for anything else too, I don't think we should allow that as it won't interoperate with anything and would not comply with the RFC at all. We could call that application/x-uint16be-prefixed-packets then ;)

Having the original RTP/RTCP caps with just a different structure name inside the RFC4571 caps has the advantage that the element can automatically transform them and put them on the srcpad. Otherwise with ANY caps on the srcpad you'll need something else to set the caps there again, instead of just between tcp*src and the depayloader.
Comment 8 Olivier Crête 2013-12-04 16:27:04 UTC
What about application/x-rtp-stream then ? You also need to add application/x-srt[c]p in there. And application/x-dtls-rt[c]p, etc
Comment 9 Sebastian Dröge (slomo) 2013-12-04 16:31:06 UTC
(In reply to comment #8)
> What about application/x-rtp-stream then ? You also need to add
> application/x-srt[c]p in there. And application/x-dtls-rt[c]p, etc

That's a good suggestion, thanks. So rtpstream(de)pay, application/x-rt(c)p-stream. srt(c)p and dtls-rt(c)p are not listed in that RFC, is there another one that allows usage of these packets here? Or are they just normal RT(C)P packets?
Comment 10 Olivier Crête 2013-12-04 16:46:05 UTC
It's not defined in any RFC, but it's still used in practice.
Comment 11 Sebastian Dröge (slomo) 2013-12-04 16:49:36 UTC
Ok, I'll add those then :) Any other caps to add?
Comment 12 Olivier Crête 2013-12-04 17:05:53 UTC
wait for the dtls one, as the elements arent finalized yet, we can all add them at the same time.
Comment 13 Sebastian Dröge (slomo) 2013-12-04 20:58:56 UTC
commit d87f6cf48386af4a62097b1ddc6f315e908eba52
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Dec 4 21:17:03 2013 +0100

    rtpstreamdepay: Add RFC4571 RTP stream depayloading element
    
    https://bugzilla.gnome.org/show_bug.cgi?id=719829

commit c5284dc0475701aa3df7749e29a965c684727be2
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Dec 4 10:12:46 2013 +0100

    rtpstreampay: Add RFC4571 RTP stream payloading element
    
    https://bugzilla.gnome.org/show_bug.cgi?id=719829