[Ffmpeg-devel] RTP patches & RFC

[Michael Niedermayer]

Hi

On Tue, Oct 17, 2006 at 04:05:28PM -0500, Ryan Martell wrote:
[...]
> >[...]
> >>+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
> 
> I couldn't find anything in the rtp code that was called at  
> initialization time.  What would you recommend for this?

put the register_dynamic_payload_handler calls in 
libavformat/allformats.c:av_register_all()


[...]
> >
> >[...]
> >>+                {
> >>+                    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
> 
> >[...]
> >>@@ -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
> 
> Yeah, i was just trying to remove the compiler warnings.  Can submit  
> as separate patch.

yes, please do


[...]
> >[...]
> >>    } 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()
> 
> This is true, but I have to parse that stuff out becfore I can get to  
> the values, which are then used internally.  Rather than have a  
> function pointer for parsing sdp streams, and a different function  
> pointer for parsing some of the other sdp lines, I figured it would  
> be better to have one function pointer for all sdp strings.   
> Unfortunately, it means that there would be slight duplication of  
> code (as per here).  I could add something like a single function to  
> rtp.c that would give me the attr & values for this function; would  
> that be better?

yes, i think so


> 
> So it sounds like my best solution (from all this) would be to submit  
> several incremental patches, instead of one monolithic one?

yes


> 
> I'll try and break this up somewhat and submit as smaller patches.

thanks


[...]
-- 
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