[Ffmpeg-devel] RTP patches & RFC
Michael Niedermayer
michaelni
Thu Oct 12 00:27:28 CEST 2006
Hi
On Tue, Oct 10, 2006 at 08:08:04PM -0500, Ryan Martell wrote:
> This is a rough draft of my rtp_h264 code. This will not compile
> without adding some diffs, but having never done this before, I
> figured I would post the file and see what I need to fix (formatting
> wise)
>
> I ran it through indent, as per the RFC, and it nuked my tabs (some
> may have been reintroduced through editing though- i'll re-indent it
> before I post the final version. I'm not a fan of the format (i like
> more spaces), but that's just me whining. ;-)
>
> My other reason for posting is to see if anyone can address any of my
> current issues:
> TODO:
> 1) Fix video/audio syncing! (frame rate wanders; sometimes it's 10,
> sometimes 29.97. I suspect I'm not doing something right.
> 2) I hand packets to the decoder as I get them, since h264 can be
> backward and forward looking. i am only updating pts though, and
> those are not strictly increasing. Is this right?
yes pts arent monotone before the decoder
[...]
> 4) I am using my own packet queues. This means that a packet comes
> in (from network), gets allocated and copied to a packet, and once a
> full frame is received, is conglomerated and
> copied into an AVPacket. I got rid of the extra allocations and
> reallocations and copies with the fragmentation packets (see the
> add_h264_partial_data)
either
A. you support out of order packets (like with a n packet buffer which
reorders packets) and then after reordering you output them
B. you output packets in their order and fragmentation
either way you should set AVStream->need_parsing=1 and leave the merging
of packets to the AVParser
[...]
> Any thoughts on any of the above, plus any and all criticism/
> commentary is appreciated!
some comments below (this is not a real review, just what i stumbled
across while lookin around a little)
[...]
> #define USE_ADJUSTED // adjusted by using rtcp stuff...
>
> /* -------------------------------- h264 RTP Packet crap */
> enum {
> _raw_packet = 0, // do nothing
> _prepend_start_sequence, // prepend packets with 001
> _prepend_length_1_byte, // prepend packet with a 1 byte length
> _prepend_length_2_bytes, //prepend packet with a 2 byte length
> _prepend_length_4_bytes //prepend packet with a 4 byte length
> };
stuff starting with underscores is reserved in c
and comments should be doxygen compatible
and the enum should get a name which is used in the types which are
supposed to hold values from the enum
furthermore looking at the later code this should be
enum {
raw_packet = 0,
prepend_length_1_byte,
prepend_length_2_bytes,
prepend_start_sequence,
prepend_length_4_bytes
};
that way the length matches the enum value and you can simplify the code
>
> struct h264_packet {
> uint8_t *data;
> uint32_t length;
> uint32_t timestamp;
>
> struct h264_packet *next;
> };
timestamps probably need to be 64bit
and maybe you could use AVPacketList
also use typedef struct to simplify the source code
[...]
> static void add_h264_partial_data(struct h264_rtp_extra_data *data, uint32_t timestamp, uint8_t reconstructed_nal, uint8_t start_bit, uint8_t end_bit, const uint8_t * buf, uint32_t len, uint32_t adjusted_timestamp);
too long line
> static void handle_h264_nal(RTPDemuxContext * s, struct h264_rtp_extra_data *data, uint32_t timestamp, const uint8_t * buf, uint32_t len);
>
> static void *convert_h264_initial_parameters_to_extradata(struct h264_rtp_extra_data *data, int *length); // this is an avcC block from the initial_parameters
> static int set_length_in_buffer(uint8_t *buffer, uint32_t len, int packet_mode);
>
> /* ---------------- packet prototypes */
> static struct h264_packet *new_h264_packet(uint32_t timestamp, const uint8_t * buf, uint32_t len, int packet_mode);
> static struct h264_packet *new_h264_packet_prepend_nal(uint32_t timestamp, const uint8_t * buf, uint32_t len, int packet_mode, uint8_t nal);
> static void free_h264_packet(struct h264_packet *packet);
>
> /* ---------------- packet queue prototypes */
> static void free_packet_queue(struct packet_queue *queue);
> static void put_packet_at_head_of_queue(struct packet_queue *queue, struct h264_packet *packet);
> static void queue_packet(struct packet_queue *queue, struct h264_packet *packet);
> static struct h264_packet *next_packet(struct packet_queue *queue);
> static struct h264_packet *next_packet_of_type(struct packet_queue *queue, uint8_t desired_type);
>
> /* ----------------- text to binary crap */
> static int base64_decode(unsigned char in[], unsigned char out[], int len);
> static uint8_t hex_chars_to_byte(char *buff);
are all these prototypes really needed? if no remove them if yes try to
reorder the funcions to make them unneeded
> #define TRUE (1)
> #define FALSE (0)
unaccpetable, remove these
>
> #define MAGIC_COOKIE (0xdeadbeef)
> #define DEAD_COOKIE (0xdeaddead)
document what these mean
>
> /* ---------------- code */
> void *new_h264_extradata()
non static functions need some av_ or ff_ prefix to avoid namespace clashes
> {
> struct h264_rtp_extra_data *data =
> av_malloc(sizeof(struct h264_rtp_extra_data) + FF_INPUT_BUFFER_PADDING_SIZE);
>
> if (data)
> {
> int packet_header_length;
>
> memset(data, 0, sizeof(struct h264_rtp_extra_data));
av_mallocz()
> data->cookie = MAGIC_COOKIE;
> data->packet_mode = _prepend_length_4_bytes;
> switch (data->packet_mode) {
> case _prepend_start_sequence: // prepend packets with 001
> packet_header_length = 3;
> break;
> case _prepend_length_1_byte: // prepend packet with a 1 byte length
> packet_header_length = 1;
> break;
> case _prepend_length_2_bytes: //prepend packet with a 2 byte length
> packet_header_length = 2;
> break;
> case _prepend_length_4_bytes: //prepend packet with a 4 byte length
> packet_header_length = 4;
> break;
> case _raw_packet: // do nothing
> default:
> packet_header_length = 0;
> break;
> }
packet_header_length= data->packet_mode with the change of the enum values
i suggested above
>
> // set these up (need to know for debugging and looking for nals of certain types)
> data->network_packets.packet_header_length= packet_header_length;
> data->frame_packets.packet_header_length= packet_header_length;
> data->out_of_band_packets.packet_header_length= packet_header_length;
> data->partial_packets.packet_header_length= packet_header_length;
> }
>
> return data;
> }
>
> void free_h264_extra_data(void *d)
unused function
[...]
>
> /*
> int64_t pts; ///< presentation time stamp in time_base units
> int64_t dts; ///< decompression time stamp in time_base units
> uint8_t *data;
> int size;
> int stream_index;
> int flags;
> int duration; ///< presentation duration in time_base units (0 if not available)
> void (*destruct)(struct AVPacket *);
> void *priv;
> int64_t pos; ///< byte position in stream, -1 if unknown
> */
this junk doesnt belong here
> // return 0 on packet, no more left, 1 on packet, 1 on partial packet...
> // i don't have enough return values for this!
> int handle_h264_rtp_packet(RTPDemuxContext * s, AVPacket * pkt,
> uint32_t timestamp, const uint8_t * buf,
> int len)
> {
> struct h264_rtp_extra_data *data =
> s->rtp_payload_data->protocol_specific_data;
> struct h264_packet *packet;
> int result = -1;
> int done = 0;
>
> // temp
> assert(data);
> assert(data->cookie == MAGIC_COOKIE);
> // av_set_pts_info(s->st, 33, 1, 90000);
you must set the timebase correctly (the default of 90khz is very likly not
correct)
>
> // fprintf(stderr, "Buf: %p Length: %d\n", buf, len);
this should be av_log or removed (and that of course applies to all these
// *printf lines
[...]
> } else {
> av_log(NULL, AV_LOG_ERROR, "Out of memory. Sorry\n");
> exit(0);
no, exit() is unaccpetable
[...]
> case _prepend_length_2_bytes: //prepend packet with a 2 byte length
> assert(len >= 0 && len <= 0xffff);
> {
> uint16_t length = len;
> *((uint16_t *) buffer) = BE_16(&length);
segfault due to lack of alignement on non x86 architectures
one way to implement this page of messy and broken code is:
for(i=0; i<bytes; i++)
buffer[i]>>(8*(bytes-i-1));
> }
> extra_length = 2;
> break;
wrong indention
[...]
> *dst= nal;
> dst+= sizeof(uint8_t);
dst++;
[...]
> av_free(packet);
> packet = NULL;
av_freep()
[...]
> return;
> }
useless
[...]
> static uint8_t hex_chars_to_byte(char *buff)
> {
> uint8_t result = 0;
> int ii;
> int multiplier = 16;
>
> for (ii = 0; ii < 2; ii++) {
> if (buff[ii] >= 'A' && buff[ii] <= 'F') {
> result += ((buff[ii] - 'A') + 10) * multiplier;
> } else if (buff[ii] >= 'a' && buff[ii] <= 'f') {
> result += ((buff[ii] - 'a') + 10) * multiplier;
> } else if (buff[ii] >= '0' && buff[ii] <= '9') {
> result += ((buff[ii] - '0')) * multiplier;
> }
> multiplier /= 16;
> }
>
> return result;
> }
strtol()
>
> // 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%4 == 0)
use &, % is slow (and no gcc is too stupid to fix this)
> {
> 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;
and due to security reasons the available space should be feeded into
this function instead of blindly hoping that out[] will be large enough
> // fprintf(stderr, "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<=127 && i1<=127 && i2<=127 && i3<=127)
if((i0|i1|i2|i3)<=127)
> {
> int b0 = map2[i0];
> int b1 = map2[i1];
> int b2 = map2[i2];
> int b3 = map2[i3];
> if(b0>=0 && b1>=0 && b2>=0 && b3>=0)
map2 is uint8_t, this check is useless
> {
> int o0 = ( b0 <<2) | (b1>>4); // these were unsigned right shift.
> int o1 = ((b1 & 0xf)<<4) | (b2>>2); // these were unsigned right shift in java
> int o2 = ((b2 & 3)<<6) | b3;
the & 0xf and &3 are useless
> out[op++] = (uint8_t)o0;
> if(op<oLen) out[op++] = (uint8_t)o1;
> if(op<oLen) out[op++] = (uint8_t)o2;
the uint8_t casts arent needed either
[...]
--
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
More information about the ffmpeg-devel
mailing list