[Ffmpeg-devel] RTP patches & RFC
Michael Niedermayer
michaelni
Tue Oct 17 02:25:47 CEST 2006
Hi
On Thu, Oct 12, 2006 at 04:29:09PM -0500, Ryan Martell wrote:
[...]
> On Oct 12, 2006, at 5:29 AM, Michael Niedermayer wrote:
> >Hi
> >
> >On Wed, Oct 11, 2006 at 10:41:36PM -0500, Ryan Martell wrote:
> >[...]
> >>>
> >>>either way you should set AVStream->need_parsing=1 and leave the
> >>>merging
> >>>of packets to the AVParser
> >>
> >>Okay, sorry to be dense about this, but I'm still confused. The
> >>packets that come over rtp have their size indicated in various ways,
> >>depending on the packet. That information is NOT in the NAL, and the
> >>NAL is NOT preceeded by the start sequence. So what I was doing was
> >>converting them to AvC style (preceded by length) packets. This is
> >
> >iam more in favor of adding the startcode prefix instead of the length
> >i dont think the avc style length stuff is supported by our AVParser
> >
>
> I changed the code, so that in my init routine you can specify if you
> want the bitstream version or the avcc version. Both work, as long
> as I don't set needs_parse= 1 for the stream. When I do that, I
> never get past the setup code (I don't know why). (Maybe it's
> because I only had 001 instead of 0001 on the sps/pps packets, from
> derk above?). This is the same issue i was having when i first
> started this, where there was no data in the extradata field, it
> wouldn't get to the point where it showed the stream types.
does this also happen if you simply dump the data to a file and then
try to play it with ffmpeg or ffplay? if yes then please upload such
a file ill take a look
[...]
> >stuff starting with underscores is reserved in c
> >and comments should be doxygen compatible
> >and the enum should get a name which is used in the types which are
>
> doxygen is pretty cool; never used it before.
>
> Here's all of my code; I didn't really want to get into rtp/rtsp that
> much, but I had to. It's still a work in progress (it times out
> after 2 minutes on Darwin Streaming Server, because i don't send the
> rtcp packets). But it's a start. I'd like to get this out there, in
> case others need it, but I have some other requirements that might
> pull me away from this for the next few days/week.
>
> All the jitter calculation/rtp sequence updating algorithms come from
> the rtp rfc.
>
> Michael, thanks for the code review, and hopefully this is more
> inline with the other code.
its too big, i tried to review it (see below) but 80k is simply too big
for a single selfcontained patch, also the rt*p code isnt really my area
which doesnt simplify this
so please try to split this somehow into small and independant patches
[...]
> @@ -179,43 +182,28 @@
> {-1, "", CODEC_TYPE_UNKNOWN, CODEC_ID_NONE, -1, -1}
> };
>
> -AVRtpDynamicPayloadType_t AVRtpDynamicPayloadTypes[]=
> +/* statistics functions */
> +RTPDynamicProtocolHandler *RTPFirstDynamicPayloadHandler= NULL;
> +
> +static RTPDynamicProtocolHandler mp4v_es_handler= {"MP4V-ES", CODEC_TYPE_VIDEO, CODEC_ID_MPEG4};
> +static RTPDynamicProtocolHandler mpeg4_generic_handler= {"mpeg4-generic", CODEC_TYPE_AUDIO, CODEC_ID_MPEG4AAC};
> +
> +static void register_dynamic_payload_handler(RTPDynamicProtocolHandler *handler)
> {
> - {"MP4V-ES", CODEC_TYPE_VIDEO, CODEC_ID_MPEG4},
> - {"mpeg4-generic", CODEC_TYPE_AUDIO, CODEC_ID_MPEG4AAC},
> - {"", CODEC_TYPE_UNKNOWN, CODEC_ID_NONE}
> -};
> + handler->next= RTPFirstDynamicPayloadHandler;
> + RTPFirstDynamicPayloadHandler= handler;
> +}
why do you change the AVRtpDynamicPayloadType_t to a linked list based
system? and this should be a seperate patch
[...]
> +void rtp_configure_dynamic_payloads()
> +{
> + if(RTPFirstDynamicPayloadHandler==NULL)
> + {
> + register_dynamic_payload_handler(&mp4v_es_handler);
> + register_dynamic_payload_handler(&mpeg4_generic_handler);
> + register_dynamic_payload_handler(&ff_h264_dynamic_handler);
> + }
> +}
this doesnt look thread safe
[...]
> @@ -264,13 +252,94 @@
> if (s->first_rtcp_ntp_time == AV_NOPTS_VALUE)
> s->first_rtcp_ntp_time = s->last_rtcp_ntp_time;
> s->last_rtcp_timestamp = decode_be32(buf + 16);
> +
> return 0;
cosmetic change (=whitespace only change) these should be in a seperate patch
if they are usefull for readablity, either way they must not be in non
cosmetic patches
[...]
> +
> +// returns 1 if we should handle this packet.
this should be a doxygen compatible comment /** @return ... */
> +static int rtp_valid_packet_in_sequence(RTPStatistics *s, uint16_t seq)
> +{
> + uint16_t udelta= seq - s->max_seq;
is there any specific reason why this one and many other variables are
(u)intXY_t and not simply int? (u)intXY_t types are exact size and can be
slower then int on some architectures, if you want to be pedantic and want
the fasterst type with at least X bits then uint_fast16_t, ... are the
correct ones
for arrays of course (u)intXY_t is ok due to the space saving
[...]
> +static void rtcp_update_jitter(RTPStatistics *s, uint32_t sent_timestamp, uint32_t arrival_timestamp)
> +{
> + uint32_t transit= arrival_timestamp - sent_timestamp;
> + int d= transit - s->transit;
> + s->transit= transit;
> + if(d<0) d= -d;
d= FFABS(transit - s->transit);
[...]
> - default:
> +
> + default:
> break;
cosmetic change
> }
> }
> @@ -372,19 +444,27 @@
> AVStream *st;
> uint32_t timestamp;
>
> - if (!buf) {
> - /* return the next packets, if any */
> - if (s->read_buf_index >= s->read_buf_size)
> - return -1;
> - ret = mpegts_parse_packet(s->ts, pkt, s->buf + s->read_buf_index,
> - s->read_buf_size - s->read_buf_index);
> - if (ret < 0)
> - return -1;
> - s->read_buf_index += ret;
> - if (s->read_buf_index < s->read_buf_size)
> - return 1;
> - else
> - return 0;
> + if (!buf)
> + {
> + if(s->st && s->dynamic_packet_handler)
> + {
> + return s->dynamic_packet_handler(s, pkt, 0, NULL, 0);
> + }
> + else
> + {
> + /* return the next packets, if any */
> + if (s->read_buf_index >= s->read_buf_size)
> + return -1;
> +
> + ret = mpegts_parse_packet(s->ts, pkt, s->buf + s->read_buf_index, s->read_buf_size - s->read_buf_index);
> + if (ret < 0)
> + return -1;
> + s->read_buf_index += ret;
> + if (s->read_buf_index < s->read_buf_size)
> + return 1;
> + else
> + return 0;
> + }
unneeded reindention, quoting development policy:
"... If you had to put if(){ .. } over a large (> 5 lines) chunk of code, then either do NOT change the indentation of the inner part within (don't move it to the right)! or do so in a separate commit" (http://ffmpeg.mplayerhq.hu/ffmpeg-doc.html)
> }
>
> if (len < 12)
> @@ -394,7 +474,7 @@
> return -1;
> if (buf[1] >= 200 && buf[1] <= 204) {
> rtcp_parse_packet(s, buf, len);
> - return -1;
> + return -1;
> }
cosmetic change
[...]
> s->read_buf_index = 0;
> return 1;
> }
> - } else {
> + } else {
cosmetic change
additionally trailing whitespace and tabs are forbidden in svn
see the clean-diff script in svn (it removes tabs and trailing
whitespace from patches)
[...]
> pkt->stream_index = s->st->index;
> + // rdm: compute pts?
> return 0;
> + break;
break after return?
[...]
> + if(lost>0xffffff) lost= 0xffffff; // clamp it since it's only 24 bits...
FFMIN()
[...]
> + *dst++= (RTP_VERSION << 6);
> + *dst++= 201; // reception report..
> + *dst++= 0; *dst++= 6; // total length in words...
> + *dst++= s->ssrc>>24; *dst++= s->ssrc>>16; *dst++= s->ssrc>>8; *dst++= s->ssrc; // data source being reported
> +
> + *dst++= fraction_and_cumulative_lost>>24;
> + *dst++= fraction_and_cumulative_lost>>16;
> + *dst++= fraction_and_cumulative_lost>>8;
> + *dst++= fraction_and_cumulative_lost; //8 bits of fraction lost since last sr/rr, followed by 24 bits of cumulative number of packets lost
> +
> + *dst++= extended_max>>24;
> + *dst++= extended_max>>16;
> + *dst++= extended_max>>8;
> + *dst++= extended_max; // extended last sequence number received.
> +
> + temp= stats->jitter>>4;
> + *dst++= temp>>24;
> + *dst++= temp>>16;
> + *dst++= temp>>8;
> + *dst++= temp; // extended last sequence number received.
the 32bit big endian write above is duplicated a few times
[...]
> -//#define DEBUG
> +#include "rtp_internal.h"
> +
> +#define DEBUG
uncommenting DEBUG is of course not accpetable
[...]
> + {
> + int width, height;
> +
> + rtsp_get_word_sep(buf1, sizeof(buf1), "-", &p);
> + width= atoi(buf1);
> + height= atoi(p+1); // skip the -
the width and height variables are unused
[...]
> @@ -712,7 +767,7 @@
> #ifdef DEBUG
> printf("Sending:\n%s--\n", buf);
> #endif
> - url_write(rt->rtsp_hd, buf, strlen(buf));
> + url_write(rt->rtsp_hd, (unsigned char *)buf, strlen(buf));
unrelated change (-> seperate patch)
[...]
> @@ -788,6 +843,8 @@
> rtp_parse_close(rtsp_st->rtp_ctx);
> if (rtsp_st->rtp_handle)
> url_close(rtsp_st->rtp_handle);
> + if (rtsp_st->dynamic_handler && rtsp_st->dynamic_protocol_data)
> + rtsp_st->dynamic_handler->protocol_free_extradata_proc(rtsp_st->dynamic_protocol_data);
> }
> av_free(rtsp_st);
> }
> @@ -806,7 +863,7 @@
> RTSPStream *rtsp_st;
> int protocol_mask;
> AVStream *st;
> -
> +
cosmetic
[...]
> @@ -1263,11 +1369,11 @@
>
> static int sdp_probe(AVProbeData *p1)
> {
> - const char *p = p1->buf, *p_end = p1->buf + p1->buf_size;
> + const unsigned char *p = p1->buf, *p_end = p1->buf + p1->buf_size;
>
> /* we look for a line beginning "c=IN IP4" */
> while (p < p_end && *p != '\0') {
> - if (p + sizeof("c=IN IP4") - 1 < p_end && strstart(p, "c=IN IP4", NULL))
> + if (p + sizeof("c=IN IP4") - 1 < p_end && strstart((char *) p, "c=IN IP4", NULL))
> return AVPROBE_SCORE_MAX / 2;
>
> while(p < p_end - 1 && *p != '\n') p++;
> @@ -1294,7 +1400,7 @@
> /* read the whole sdp file */
> /* XXX: better loading */
> content = av_malloc(SDP_MAX_SIZE);
> - size = get_buffer(&s->pb, content, SDP_MAX_SIZE - 1);
> + size = get_buffer(&s->pb, (unsigned char *)content, SDP_MAX_SIZE - 1);
> if (size <= 0) {
> av_free(content);
> return AVERROR_INVALIDDATA;
unrelated signed/unsigned type change
[...]
> static int base64_decode(unsigned char in[], unsigned char out[], int len)
> {
> int iLen = len;
> int oLen = 0;
>
> if ((iLen & 3) == 0) {
> int ip = 0;
> int op = 0;
>
> while (iLen > 0 && in[iLen - 1] == '=')
> iLen--;
> //oLen= ((iLen*3)/4) + (((iLen*6)%8)?1:0); // this is the correction to that java code.
> oLen = (iLen * 3 + 3) >> 2;
> // fprintf(stderr, "oLen: %d iLen: %d\n", oLen, iLen);
>
> while (ip < iLen) {
> int i0 = in[ip++];
> int i1 = in[ip++];
> int i2 = ip < iLen ? in[ip++] : 'A';
> int i3 = ip < iLen ? in[ip++] : 'A';
> if ((i0 | i1 | i2 | i3) <= 127) {
> int b0 = map2[i0];
> int b1 = map2[i1];
> int b2 = map2[i2];
> int b3 = map2[i3];
> if (b0 != 0xff && b1 != 0xff && b2 != 0xff && b3 != 0xff) {
> int o0 = (b0 << 2) | (b1 >> 4); // these were unsigned right shift.
> int o1 = (b1 << 4) | (b2 >> 2); // these were unsigned right shift in java
> int o2 = (b2 << 6) | b3;
> out[op++] = o0;
> if (op < oLen)
> out[op++] = o1;
> if (op < oLen)
> out[op++] = o2;
> } else {
> av_log(NULL, AV_LOG_ERROR,
> "Ilegal character in base64 data!");
> }
> } else {
> av_log(NULL, AV_LOG_ERROR,
> "Ilegal character in base64 data!");
> }
> }
> // fprintf(stderr, "OP: %d\n", op);
> } else {
> av_log(NULL, AV_LOG_ERROR, "Base64 must be a multiple of 4!");
> }
>
> return oLen;
something like the following is simpler (but untested!), though slower but i
think this is never used in any speed critical code
static int base64_decode(uint8_t *out, uint8_t *in, int len)
int i, v;
for(i=0; i<len && in[i]!='='; i++){
if(in[i] > 127 || map2[in[i]]==0xff)
return -1;
v= (v<<6) + map2[in[i]];
if(i&3)
*out++= v>>(6-2*(i&3));
}
}
[...]
> #if 1
> if (s->last_rtcp_ntp_time != AV_NOPTS_VALUE) {
> int64_t addend;
> int32_t delta_timestamp;
> /*
> ntp format represents seconds and fraction as a 64-bit unsigned fixed-point integer with decimal point
> to the left of bit 32 numbered from the left. The 32-bit seconds field spans about 136 years, while the
> 32-bit fraction field precision is about 232 picoseconds.
> */
> // fprintf(stderr, " (has timestamp) ");
> /* XXX: is it really necessary to unify the timestamp base ? */
> /* compute pts from timestamp with received ntp_time */
> delta_timestamp = timestamp - s->last_rtcp_timestamp;
> // if(delta_timestamp<0) fprintf(stderr, "Backwards in time: %d\n", delta_timestamp);
> /* convert to 90 kHz without overflow */
> addend = (s->last_rtcp_ntp_time - s->first_rtcp_ntp_time) >> 14;
> addend = (addend * 5625) >> 14;
> adjusted_timestamp = addend + delta_timestamp;
this is duplicated from rtp.c
[...]
> } else if (strstart(p, "fmtp:", &p)) {
> char attr[256];
> char value[4096];
>
> /* loop on each attribute */
> for (;;) {
> rtsp_skip_spaces(&p);
> if (*p == '\0')
> break;
> rtsp_get_word_sep(attr, sizeof(attr), "=", &p);
> if (*p == '=')
> p++;
> rtsp_get_word_sep(value, sizeof(value), ";", &p);
> if (*p == ';')
> p++;
> /* grab the codec extra_data from the config parameter of the fmtp line */
> sdp_parse_fmtp_config_h264(stream, h264_data, attr, value);
this code is duplicated from rtsp:sdp_parse_fmtp()
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is
More information about the ffmpeg-devel
mailing list