[Ffmpeg-devel] RTP patches & RFC

[Michael Niedermayer]

Hi

On Mon, Oct 23, 2006 at 06:38:13PM -0500, Ryan Martell wrote:
[...]
> >[...]
> >
> >
> >>@@ -373,7 +366,13 @@
> >>     uint32_t timestamp;
> >>
> >>     if (!buf) {
> >>-        /* return the next packets, if any */
> >>+        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,
> >>@@ -385,6 +384,7 @@
> >>             return 1;
> >>         else
> >>             return 0;
> >>+        }
> >>     }
> >
> >this should rather look like:
> >
> >     if (!buf) {
> >         /* return the next packets, if any */
> >+      if(s->st && s->dynamic_packet_handler) {
> >+            return s->dynamic_packet_handler(s, pkt, 0, NULL, 0);
> >+      } else {
> >         if (s->read_buf_index >= s->read_buf_size)
> >             return -1;
> >         ret = mpegts_parse_packet(s->ts, pkt, s->buf + s- 
> >>read_buf_index,
> >@@ -385,6 +384,7 @@
> >             return 1;
> >         else
> >             return 0;
> >+      }
> >     }
> >
> >* the /* return the next packets, if any */ was reindented
> >* the {} placement doesnt match the rest of the file
> >* the indention level isnt optimal unless you plan to send a
> >  seperate patch to fix the indention after this one
> 
> 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)


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


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


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


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


>  
>      /* 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 ...)

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