Created attachment 38335 [details] [review]
updates for rtp and udp directories of gst-plugins

UDP: adds a time to live Element Property to udpsink, fixes comments in udpsrc
RTP: 1) fairly complete rewrite of the encoding and parsing code, encoder now
passes analysis by ethereal and can play to the parser and to mplayer. 
2) Adds support for configurable dynamic payload processing using a new element
property "rtpmap" which is a string of rtpmap records in the same format as
would be present in an SDP record.
3) Adds support for big and little endian peers on the pads
4) Adds support for mtu element property on rtpL16enc and will split incoming
buffers to avoid IP layer fragmentation (should this be a capability?)

Oops, in the patch line:

+	if(sscanf(rtpL16parse->rtpmap, m, &rtpL16parse->frequency,
&rtpL16parse->channels)==3) {

The 3 should be a 2

I'll try to have a look at this patch tomorow but one thing that i find
obviously wrong is the inclusion of mtu property in the rtp elements. We used to
have this property when we didn't yet seperate the RTP packet encoding part and
the transport part. Since that seperation, the 'mtu' has been moved to the
udpsink/tcpsink elements. I wonder why thomas didn't include this property in
his tcp sink elements: tcpserversink and tcpclientsink.

Hi Ali - I'm still working on this code - I can send you a snapshot but I'll
wait a bit if that's OK. As far as mtu is concerned there are a number of
options, but JUST having it in the udpsink element won't work. Basically udpsink
MUST NOT fragment RTP packets. The problem is RTP doesn't have a length field,
it relies on UDP framing, so if UDP fragments then the receiver sees one short
RTP packet and another unidentifiable packet with no RTP header.

Note that tcp and udp unicast can use path mtu discovery so it doesn't really
need a parameter but udp multicast probably needs the parameter, and its the rtp
element that needs to know it.

One solution is for the rtp encoder to query the udpsink for mtu.

The solution I'm using is to decide a priori what the number of samples should
be and set it upstream in the pipeline. This is best for live streaming since it
keeps the packet encode latency known.

BTW - I'll propably make the plugin rtpLxx and handle 8/16/20/24 bit (20 is the
only hard one). Also, should the rtp elements be bigendian only (rtp is always
bigendian) or should they support endian swaps internally?

Julian

Finally, I was able to reviewe the patch. Although most of the code seems quite
cool, except for a few concerns:

gstrtpL16enc.c:

* Why have props for sample_rate, payload_type, rtpmap and the number of
channels when we get/derive these values from the caps?

* In the following code, you seem to doing some kind of mutual-recursion:

    this_samples = this_packet_len / 2 / rtpL16enc->channels;
    /* if space_for_samples not right for a fixed number of samples, adjust! */
    this_packet_len = 2 * rtpL16enc->channels * this_samples;

As the value of 'this_packet_len' shall remain to be the same after the
execution of the last statement.

gstrtpL16parse.c:

* The number of channels can be detected by the payload_type so there is no need
to have a seperate prop. for it. is it?

Now regarding wether to have the byte-swaping code inside the plugin or not:
IMHO it should be inside the rtp elements.

Now you got me into thinking how to optimize the byte-swaping loop using MMX
(where available) :) Either liboil shall be providing functions for this or
we'll have to add it to liboil and then use it in our element(s).

Sorry, I was just told/reminded of the 'audioconvert' element that is also
responsible to do BE and LE conversions. So we can safelly have our pads do only BE.

Hi Ali, sorry for the delay - wasn't checking home email over Easter.

This code:
    this_samples = this_packet_len / 2 / rtpL16enc->channels;
    /* if space_for_samples not right for a fixed number of samples, adjust! */
    this_packet_len = 2 * rtpL16enc->channels * this_samples;

is to cater for a possible case where the (remaining) buffer size is not a
multiple of the channels*depth (or is it width?). In this case we should not use
the last few octets of the buffer.

The sample_rate, payload_type, and the number of
channels properties could be readonly - no need to be able to set them. rtpmap
is fundamental for handling dynamic payload types, which is everything except
rate=44100, one or two channels 16 bit.

I'm attaching my latest codebase - this has much improved timestamping and a
number of other improvements.

I've changed the "mtu" parameter to "bytesperbuffer" since mtu means the size of
the IP datagram, not the UDP payload. I still think its useful to have here,
especially for file sources, etc. This is also what the sync parameter is for,
to pace sending files.

I haven't done the other depths & widths yet, and the code is a bit dirty but at
least it works better than what's there at the moment.


Julian

Perhaps you forgot to attach the 'latest codebase' you wrote about in your
recent comment as i am unable to find another attachement. :)

Your patch is commited to the CVS 0.8 branch. Kindly close the Bug as FIXED. For
more improvements, just submit a new bug report.

Created attachment 39656 [details]
full rtpL16 files

oops - they got uploaded to the wrong bug!

Julian

Created attachment 39657 [details]
diff from new cvs to the files I uploaded to the wrong bug. (equivalent to 39656)

I have noticed that you are not using the rtppacket module/api (provided inside
the plugin) to create/parse packets. May I ask why? If you find that API to lack
something, you are more than welcome to improve upon that.

Hi Ali,

   no very good reason - my programming style says "open code things first and
then create useful abstractions if it helps" the rtp headers are so simple that
the api doesn't help much - all it provides are some error checking that is
unnecessary if you know where you got the values in the first place. Now if the
library did RTCP I'd probably use it. (ccrtp looks interesting). The main
problem with the coding/parsing was that the hton/ntoh functions seemed broken -
hence my quick and dirty very verbose coding for these parts. Now it works its
on my "aint broke don't fix" list.  I'll send you my snapshot code - I made a
lot of changes to the clock/timing and got 8/16/20/24 bits working (20 is
untested so far). I think I know how to extract the pcm layer now - I may add it
to audioconvert so that we can have a truly general rtpenc/rtpparse. The big
problem I have now is lost packets due to blocking behaviour in the
osssink/alsasink elements.

Julian

Closing as obsolete, all RTP work is being done on 0.10 these days.