[Ffmpeg-devel] RTP patches & RFC

[Michael Niedermayer]

Hi

On Mon, Oct 23, 2006 at 12:34:28PM -0500, Ryan Martell wrote:
[...]
> >>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
> 
> The first patch; this patch allows for dynamic protocol handlers.  I  
> tried not to do any code reformatting, and to make this as simple as  
> possible.  This code doesn't do anything, except pave the way.

thanks alot, this makes reviewing much easier


[...]

the patch contains tabs and trailing whitespace these arent
allowed in svn, a simple search and replace should fix this (or
clean-diff from ffmpeg-svn run over the patch)


[...]
> @@ -298,6 +290,7 @@
>          case CODEC_ID_MP2:
>          case CODEC_ID_MP3:
>          case CODEC_ID_MPEG4:
> +        case CODEC_ID_H264:
>              st->need_parsing = 1;

ideally this should not be in this patch though i wont reject it cause of
that, its pretty harmless ...

[...]


> @@ -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


[...]

>              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) {
> -            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;
> +            }

is it neccessary to have these duplicated in 2 structs?


[...]
> // these statistics are used for rtcp receiver reports...
> typedef struct {
>     uint16_t max_seq;           ///< highest sequence number seen
>     uint32_t cycles;            ///< shifted count of sequence number cycles
>     uint32_t base_seq;          ///< base sequence number
>     uint32_t bad_seq;           ///< last bad sequence number + 1
>     uint32_t probation;         ///< sequence packets till source is valid
>     uint32_t received;          ///< packets received
>     uint32_t expected_prior;    ///< packets expected in last interval
>     uint32_t received_prior;    ///< packets received in last interval
>     uint32_t transit;           ///< relative transit time for previous packet
>     uint32_t jitter;            ///< estimated jitter.
> } RTPStatistics;

this ideally belongs to a later statistics patch

except these the patch looks ok

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