[Ffmpeg-devel] RTP patches & RFC

Ryan Martell rdm4
Fri Oct 27 01:46:19 CEST 2006


On Oct 26, 2006, at 5:30 PM, Michael Niedermayer wrote:

Some items in this conversation have been reordered.

> ok, IMHO first before anything from the patch below can be applied,  
> the
> indention of the current code (which was broken by your last patch)  
> must
> be fixed in a seperate patch

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: indent_patch.txt
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20061026/6ceb81d5/attachment.txt>
-------------- next part --------------

(This is simply rtp.c and rtsp.c and rtp_internal.h run through  
gnuindent -i4 -kr -nut).
(Which results in a huge patch)

There are no code changes here.

> Hi
>
> On Thu, Oct 26, 2006 at 04:29:20PM -0500, Ryan Martell wrote:
>> Okay, and now back to the original patch.
>>
>> This enables the h264 stream stuff; Michael, you have looked at a lot
>> of this code before.
>>
>> The big block of stuff that is moved in rtp.c relating to AAC is just
>> moved up from a lower switch statement, otherwise it's unchanged.
>
> can this be applied seperatly without breaking the code? if so submit
> a seperate patch

Yes, but I can't do it until the indent patch is accepted (see below)

> [...]
>> Index: libavformat/rtp.c
>> ===================================================================
>> --- libavformat/rtp.c	(revision 6798)
>> +++ libavformat/rtp.c	(working copy)
> [...]
>> @@ -364,14 +364,18 @@
>>  {
>>      unsigned int ssrc, h;
>>      int payload_type, seq, delta_timestamp, ret;
>> -    AVStream *st;
>> +    AVStream *st= s->st;
>>      uint32_t timestamp;
>> +    int rv= 0;
>>
>>      if (!buf) {
>>          /* return the next packets, if any */
>>          if(s->st && s->parse_packet) {
>> -            return s->parse_packet(s, pkt, 0, NULL, 0);
>> +            timestamp= 0; ///< Should not be used if buf is NULL,  
>> but should be set to the timestamp of the packet returned....
>> +            rv= s->parse_packet(s, pkt, &timestamp, NULL, 0);
>> +            goto calculate_timestamp;
>
> iam against gotos unless they are really needed (=please explain why
> that goto cannot be avoided)

No problem; I am too, but have noticed them in the code elsewhere.

>>          } else {
>> +            // TODO: Move this into a parse_packet proc...
>
> yes iam very strongly in favor of that, the 2 class system is ugly

Agreed.

> [...]
>>  void rtp_parse_close(RTPDemuxContext *s)
>> Index: libavformat/rtp_internal.h
>> ===================================================================
>> --- libavformat/rtp_internal.h	(revision 6798)
>> +++ libavformat/rtp_internal.h	(working copy)
>> @@ -23,7 +23,7 @@
>>
>>  typedef int (*DynamicPayloadPacketHandlerProc) (struct  
>> RTPDemuxContext * s,
>>                                                  AVPacket * pkt,
>> -                                                uint32_t timestamp,
>> +                                                uint32_t *timestamp,
>>                                                  const uint8_t * buf,
>>                                                  int len);
>>
>> @@ -82,5 +82,7 @@
>>  };
>>
>>  extern RTPDynamicProtocolHandler *RTPFirstDynamicPayloadHandler;
>> +
>> +int rtsp_next_attr_and_value(const char **p, char *attr, int  
>> attr_size, char *value, int value_size); ///< from rtsp.c, but  
>> used in dynamic packet handlers
>>  #endif /* RTP_INTERNAL_H */
>>
>> Index: libavformat/rtsp.c
>> ===================================================================
>> --- libavformat/rtsp.c	(revision 6798)
>> +++ libavformat/rtsp.c	(working copy)
>> @@ -200,6 +200,8 @@
>>                      i = atoi(buf);
>>                      if (i > 0)
>>                          codec->channels = i;
>> +                    // TODO: there is a bug here; if it is a mono  
>> stream, and less than 22000Hz, faad upconverts to stereo and twice  
>> the
>> +                    //  frequency.  No problem, but the sample  
>> rate is being set here by the sdp line.  Upcoming patch  
>> forthcoming. (rdm)
>>                  }
>>                  av_log(codec, AV_LOG_DEBUG, " audio samplerate  
>> set to : %i\n", codec->sample_rate);
>>                  av_log(codec, AV_LOG_DEBUG, " audio channels set  
>> to : %i\n", codec->channels);
>> @@ -287,6 +289,22 @@
>>      {NULL, -1, -1},
>>  };
>>
>> +int rtsp_next_attr_and_value(const char **p, char *attr, int  
>> attr_size, char *value, int value_size)
>> +{
>> +    skip_spaces(p);
>> +    if(**p)
>> +    {
>> +        get_word_sep(attr, attr_size, "=", p);
>> +        if (**p == '=')
>> +            (*p)++;
>> +        get_word_sep(value, value_size, ";", p);
>> +        if (**p == ';')
>> +            (*p)++;
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>> +
>>  /* parse a SDP line and save stream attributes */
>>  static void sdp_parse_fmtp(AVStream *st, const char *p)
>>  {
>> @@ -298,7 +316,8 @@
>>      AVCodecContext *codec = st->codec;
>>      rtp_payload_data_t *rtp_payload_data = &rtsp_st- 
>> >rtp_payload_data;
>>
>> -    /* loop on each attribute */
>> +    // TODO (Replace with rtsp_next_attr_and_value)
>> +    /* loop on each attribute */
>>      for(;;) {
>>          skip_spaces(&p);
>>          if (*p == '\0')
>> @@ -471,6 +490,19 @@
>>                      }
>>                  }
>>              }
>> +        } else if(strstart(p, "framesize:", &p)) {
>> +            // let dynamic protocol handlers have a stab at the  
>> line.
>> +            get_word(buf1, sizeof(buf1), &p);
>> +            payload_type = atoi(buf1);
>> +            for(i = 0; i < s->nb_streams;i++) {
>> +                st = s->streams[i];
>> +                rtsp_st = st->priv_data;
>> +                if (rtsp_st->sdp_payload_type == payload_type) {
>> +                    if(rtsp_st->dynamic_handler && rtsp_st- 
>> >dynamic_handler->parse_sdp_a_line) {
>> +                        rtsp_st->dynamic_handler->parse_sdp_a_line 
>> (st, rtsp_st->dynamic_protocol_context, buf);
>> +                    }
>> +                }
>> +            }
>>          }
>>          break;
>>      }
>
> many of the things above look quite generic and not h.264 specific,  
> they
> could be split into seperate patch(es)

Okay, I'm missing something, because the way i am doing this now is a  
pain in the ass.

I have my working copy, then I have the latest version in a separate  
directory.  I then copy the files from my working copy to the current  
version, and run a file merging tool on them.  I then look at every  
line, and take out a bunch of stuff to make the patches smaller.   
Then I have to fix a bunch of stuff that won't compile, because I  
took something out that I needed to leave in.  As a result, every  
time I submit a patch, it's taking quite awhile.

I understand the need for small patches, to keep a good idea of where  
the bugs are introduced, but what's the simple way to do this that I  
am missing?  I am trying to take small steps, but also trying to take  
big enough steps to get this in into the code base so I can go onto  
to fixing the bugs.

After I submit the patch, I can't work on the next patch until it is  
accepted.  (I use svn diff for my patches).  I just tried to move the  
AAC stuff out after my indent patch, but then I have to save the  
changes as a different filename, and run diff against that, which  
doesn't make sense, since there is no revision number or anything for  
the patch to apply to.

How can I do this so that I could submit a sequence of patches?  (For  
this email, I was going to submit an indent patch, a aac audio  
movement patch, a non-specific rtp/rtp.h change patch, then the h264  
patch)

I'm hoping there is a simple way, that I am missing.  Currently, it's  
taking me two to three times as long to get the patch submitted than  
it's taking to get the code written.... (initially, some of that was  
my ignorance about the procedures, but I think I'm getting better at  
that part of it....)

> [...]
>
>> /**
>>     Internal h264_packet structure.
>>     Used for reordering of packets, for accumulation of packets,  
>> and general maintenance prior to passing them on to an AVPacket
>> */
>> typedef struct h264_packet {
>>     uint8_t *data;              ///< The data. (duh!)
>>     int length;                 ///< Actual data size
>>     int maximum_length;         ///< The size of the data  
>> allocated...
>>     uint64_t timestamp;         ///< this is the rtp timestamp;  
>> only needs 32 bits of precision.
>>
>>     struct h264_packet *next;
>> } h264_packet;
>>
>> /**
>> Generic packet queue for h264 packets.
>>  */
>> typedef struct packet_queue {
>>     int packet_header_length;   ///< needed for looking for  
>> packets of a given type...
>>     struct h264_packet *first_packet;
>>     struct h264_packet *last_packet;
>> } packet_queue;
>
> why dont you use AVPacketList?

Unfamiliarity.  (See below)

>>
>> /**
>>     RTP/H264 specific private data.
>> */
>> typedef struct h264_rtp_extra_data {
>>     unsigned long cookie;       ///< sanity check, to make sure we  
>> get the pointer we're expecting.
>>
>>     struct packet_queue network_packets;        ///< network  
>> packets are in this list...
>>     struct packet_queue frame_packets;  ///< linked list of all  
>> the packets with the same timestamp.
>>     struct packet_queue out_of_band_packets;    ///< pps and  
>> sps... (trasmitted via sdp)
>>     struct packet_queue partial_packets;        ///< fragmentation  
>> unit packets.
>>     struct packet_queue packet_pool;    ///< preallocated packet  
>> pool; we get them from here first if we need them...
>
> one thing ive been curious about is why does h.264 need this mess  
> while the
> other codecs dont? what is the problem with just removing the extra  
> headers
> adding the 001 startcode prefixes and then passing the packets  
> through the
> AVParser? are the packets out of order in some way or not? maybe my  
> question
> is stupid but i plain dont understand why this complex buffering  
> system is
> needed for h.264 ...

They aren't out of order with this packetization mode, but if the  
other mode was implemented, it has Decoding Order Numbers in it,  
which would require out of order reordering.

The other part of the issue is that fragmentation packets should be  
reassembled before handing them to the AVParser, so that if there is  
a sequence issue or a missing packet, the entire packet can be  
dropped without going to the codec to corrupt the stream.  There is  
no way (IMHO) of doing this, if I just passed the packets up the  
chain.  I know the parser is resilient, but basically a packet could  
be broken anywhere.  it seems like a lot of strain to put on the  
decoder's error detection/correction, when at my level I KNOW if  
parts were dropped.


> [...]
>> /* ---------------- private code */
>> // ported from java (LGPL implementation at http://www.source- 
>> code.biz/snippets/java/Base64Coder.java.txt) (but it was wrong. joy.)
>> // static uint8_t map1[] =  
>> { "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789 
>> +/" };
>> static uint8_t map2[] =
>>     { 0x40, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,  
>> 0xff,
>>     0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,  
>> 0xff, 0xff,
>>     0xff, 0xff,
>>     0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,  
>> 0xff, 0xff,
>>     0xff, 0xff,
>>     0xff, 0xff, 0xff, 0xff, 0x3e, 0xff, 0xff, 0xff, 0x3f, 0x34,  
>> 0x35, 0x36,
>>     0x37, 0x38,
>>     0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0xff, 0xff, 0xff, 0xff, 0xff,  
>> 0xff, 0xff,
>>     0x00, 0x01,
>>     0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b,  
>> 0x0c, 0x0d,
>>     0x0e, 0x0f,
>>     0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x19,  
>> 0xff, 0xff,
>>     0xff, 0xff,
>>     0xff, 0xff, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20, 0x21,  
>> 0x22, 0x23,
>>     0x24, 0x25,
>>     0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f,  
>> 0x30, 0x31,
>>     0x32, 0x33,
>>     0xff, 0xff, 0xff, 0xff, 0xff
>> };
>>
>> 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;
>>         //      av_log(NULL, AV_LOG_ERROR, "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!");
>>             }
>>         }
>>         //      av_log(NULL, AV_LOG_ERROR, "OP: %d\n", op);
>>     } else {
>>         av_log(NULL, AV_LOG_ERROR, "Base64 must be a multiple of  
>> 4!");
>>     }
>>
>>     return oLen;
>> }
>
> base64_decode() should be in a seperate patch

Okay, but where should it be?  It's currently only used by the h264  
stuff, so I have it in my h264 code.  I did see that there was a  
base64 encode in the source somewhere, but it's static.

-Ryan



More information about the ffmpeg-devel mailing list