[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