[Ffmpeg-devel] RTP patches & RFC

[Michael Niedermayer]

Hi

On Wed, Oct 25, 2006 at 02:32:14PM -0500, Ryan Martell wrote:
> Here's another one; all of that stuff should be sorted out (I hope!)
> 
> 
> 
> On Oct 23, 2006, at 7:15 PM, Michael Niedermayer wrote:
> 
> >Hi
> >
> >On Mon, Oct 23, 2006 at 06:38:13PM -0500, Ryan Martell wrote:
> >[...]
> >>>[...]
> >>>
> >>
> >>I didn't want to touch the indentation initially.  I think the
> >>included is what you're looking for.
> >
> >no, i meant the new code should be indented between the levels of the
> >existing code like in my example above (IMHO that makes it more  
> >readable
> >then having 2 blocks which should be on different indention levels  
> >on the
> >same level)
> >or alternatively your original
> >change but with a _seperate_ patch which only indents the then wrongly
> >idented block by +4 (seperating them helps keeping svn log / svn
> >history easily readable)
> 
> Fixed.
> 
> >[...]
> >>@@ -457,8 +457,13 @@
> >>             memcpy(pkt->data, buf, len);
> >>             break;
> >>         default:
> >>-            av_new_packet(pkt, len);
> >>-            memcpy(pkt->data, buf, len);
> >>+            if(s->dynamic_packet_handler)
> >>+            {
> >>+                return s->dynamic_packet_handler(s, pkt,  
> >>timestamp, buf, len);
> >>+            } else {
> >>+                av_new_packet(pkt, len);
> >>+                memcpy(pkt->data, buf, len);
> >>+            }
> >>             break;
> >
> >the {} placement is inconsistant here IMHO
> 
> Fixed.
> 
> >[...]
> >>     int i;
> >>@@ -153,12 +158,22 @@
> >>        see if we can handle this kind of payload */
> >>     get_word_sep(buf, sizeof(buf), "/", &p);
> >>     if (payload_type >= RTP_PT_PRIVATE) {
> >>-        /* We are in dynmaic payload type case ... search into  
> >>AVRtpDynamicPayloadTypes[] */
> >>-        for (i = 0; AVRtpDynamicPayloadTypes[i].codec_id !=  
> >>CODEC_ID_NONE; ++i)
> >>-            if (!strcmp(buf, AVRtpDynamicPayloadTypes 
> >>[i].enc_name) && (codec->codec_type == AVRtpDynamicPayloadTypes 
> >>[i].codec_type)) {
> >>-                codec->codec_id = AVRtpDynamicPayloadTypes 
> >>[i].codec_id;
> >>-                break;
> >>-            }
> >>+        {
> >>+            RTPDynamicProtocolHandler *handler=  
> >>RTPFirstDynamicPayloadHandler;
> >>+            while(handler)
> >>+            {
> >>+                if (!strcmp(buf, handler->enc_name) && (codec- 
> >>>codec_type == handler->codec_type)) {
> >>+                    codec->codec_id = handler->codec_id;
> >>+                    rtsp_st->dynamic_handler= handler;
> >>+                    if(handler->protocol_new_extradata_proc)
> >>+                    {
> >>+                        rtsp_st->dynamic_protocol_data= handler- 
> >>>protocol_new_extradata_proc();
> >>+                    }
> >>+                    break;
> >>+                }
> >>+                handler= handler->next;
> >>+            }
> >>+        }
> >>     } else {
> >
> >the first and last {} are unneeded
> >the other {} dont match the placement in this file
> 
> Fixed.
> 
> >[...]
> >>@@ -451,7 +466,15 @@
> >>                 st = s->streams[i];
> >>                 rtsp_st = st->priv_data;
> >>                 if (rtsp_st->sdp_payload_type == payload_type) {
> >>-                    sdp_parse_fmtp(st, p);
> >>+                    if(rtsp_st->dynamic_handler && rtsp_st- 
> >>>dynamic_handler->protocol_handle_sdp_a_line_proc)
> >>+                    {
> >>+                        if(!rtsp_st->dynamic_handler- 
> >>>protocol_handle_sdp_a_line_proc(st, rtsp_st- 
> >>>dynamic_protocol_data, buf))
> >>+                        {
> >>+                            sdp_parse_fmtp(st, p);
> >>+                        }
> >>+                    } else {
> >>+                        sdp_parse_fmtp(st, p);
> >>+                    }
> >>                 }
> >>             }
> >>         }
> >
> >same {} problem
> 
> Fixed.
> 
> >[...]
> >>-        if (!rtsp_st->rtp_ctx) {
> >>-            err = AVERROR_NOMEM;
> >>-            goto fail;
> >>-        }
> >>+        if(rtsp_st->rtp_ctx)
> >>+        {
> >>+            if(rtsp_st->dynamic_handler)
> >>+            {
> >>+                rtsp_st->rtp_ctx->dynamic_protocol_data= rtsp_st- 
> >>>dynamic_protocol_data;
> >>+                rtsp_st->rtp_ctx->dynamic_packet_handler= rtsp_st- 
> >>>dynamic_handler->protocol_handle_packet_proc;
> >>+            }
> >>+        } else {
> >>+            err = AVERROR_NOMEM;
> >>+            goto fail;
> >>+        }
> >>     }
> >
> >here too
> >
> Fixed & made smaller patcfh (left in the if (!r, and handled as an else)
> 
> >>
> >>     /* use callback if available to extend setup */
> >>@@ -1323,7 +1355,14 @@
> >>         if (!st)
> >>             s->ctx_flags |= AVFMTCTX_NOHEADER;
> >>         rtsp_st->rtp_ctx = rtp_parse_open(s, st, rtsp_st- 
> >>>sdp_payload_type, &rtsp_st->rtp_payload_data);
> >>-        if (!rtsp_st->rtp_ctx) {
> >>+        if(rtsp_st->rtp_ctx)
> >>+        {
> >>+            if(rtsp_st->dynamic_handler)
> >>+            {
> >>+                rtsp_st->rtp_ctx->dynamic_protocol_data= rtsp_st- 
> >>>dynamic_protocol_data;
> >>+                rtsp_st->rtp_ctx->dynamic_packet_handler= rtsp_st- 
> >>>dynamic_handler->protocol_handle_packet_proc;
> >>+            }
> >>+        } else {
> >>             err = AVERROR_NOMEM;
> >
> >and here
> >
> >but note, i wont reject this because of the {} placement if you  
> >disslike
> >it (fixing it myself would just need ~5min or so ...)
> 
> Hope this is a winner; I really want to get the rest of this stuff in...

could you document the fields of RTPDynamicProtocolHandler?
and rename the following, unless i missunderstood their intent, but their
current names really confused me :)
protocol_new_extradata_proc()    -> open()
protocol_handle_sdp_a_line_proc()-> parse_sdp_a_line()
protocol_free_extradata_proc()   -> close()
dynamic_protocol_data            -> dynamic_protocol_context
protocol_handle_packet_proc      -> parse() or parse_packet() or even handle_packet()
dynamic_packet_handler()         -> parse_dynamic_packet()

and what would be your oppinion about changing 
DynamicPayloadPacketHandlerProc to RTPDynamicProtocolHandler in 
RTPDemuxContext? i think it would be better to have the whole struct
available instead of just one function pointer from it

also a  int priv_data_size; could be added to RTPDynamicProtocolHandler
similar to the other stuff in lav* so that the context alloc/free can be
done outside/without open/close

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