[Ffmpeg-devel] RTP patches & RFC
Ryan Martell
rdm4
Thu Oct 12 05:41:36 CEST 2006
On Oct 11, 2006, at 5:27 PM, Michael Niedermayer wrote:
> Hi
>
> On Tue, Oct 10, 2006 at 08:08:04PM -0500, Ryan Martell wrote:
>> TODO:
>> 1) Fix video/audio syncing! (frame rate wanders; sometimes it's 10,
>> sometimes 29.97. I suspect I'm not doing something right.
>
I still haven't sorted this one out yet; my frame rate seems random
(I'm obviously not setting something up right).
>
> [...]
>> 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
Okay, sorry to be dense about this, but I'm still confused. The
packets that come over rtp have their size indicated in various ways,
depending on the packet. That information is NOT in the NAL, and the
NAL is NOT preceeded by the start sequence. So what I was doing was
converting them to AvC style (preceded by length) packets. This is
step one, which I don't think the need_parsing can handle for me
(right?). (Or does the AVParser know enough about h264 nals to
determine where they start and end?). There are no sequence packets
in the stream (it relies on the rtp timestamps). Furthermore, if
they are fragmented, they aren't just a fragmented bitstream, they
have header data that must be stripped first, and they have to be
accumulated, and only if certain parameters are met.
Step two, is accumulating all of the packets with the same timestamp
into a frame. I can see how the AVParser could do this for me, based
on the pts of the packet.
If it can handle step one, I'm not seeing it.
If it can handle step two, by setting need_parsing=1 and pushing all
my individual nal packets to it (with timestamps set), then I can
implement that quickly enough. (and reduce code size as well)
> [...]
>> 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)
I appreciate it. At first i was a bit defensive about all the stuff,
below, but after looking over the things, they are all either things:
1) I should have done better (the entire prepend stuff was initially
started while i was beating on various options to get it to work),
2) Coding styles (ie: I'm top down, so my external functions are at
the top, and that requires prototypes, because all of my helper
functions are at the bottom).
> enum {
> raw_packet = 0,
> prepend_length_1_byte,
> prepend_length_2_bytes,
> prepend_start_sequence,
> prepend_length_4_bytes
> };
excellent suggestion.
>>
>> struct h264_packet {
>> uint8_t *data;
>> uint32_t length;
>> uint32_t timestamp;
>>
>> struct h264_packet *next;
>> };
>
> timestamps probably need to be 64bit
they're only 32 bit in the rtp packet.
> and maybe you could use AVPacketList
Will check this out; some of it was trying to work within existing
rtp framework (limit my destruction to the smallest area for a first
rev)
> also use typedef struct to simplify the source code
okay.
>>
>> /* ---------------- code */
>> void *new_h264_extradata()
>
> non static functions need some av_ or ff_ prefix to avoid namespace
> clashes
>> void free_h264_extra_data(void *d)
>
> unused function
in function table not in this code. ;-)
>> // av_set_pts_info(s->st, 33, 1, 90000);
>
> you must set the timebase correctly (the default of 90khz is very
> likly not
> correct)
h264/rtp is automatically 90khz. It's also specified on the sdp line,
but it will always be 90kHz. Is this the correct way to set it?
>>
>> // 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
all fprintfs will be removed (if they were internal testing), or
av_log if useful to others.
> [...]
>> 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));
no problem. i might just get rid of prepending anything other than 4
bytes, as the others are untested...
> [...]
>> 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()
surprisingly, never used strol before. Thanks.
I'm also having to get the rtsp rtcp packet (receiver reports)
working, because that's why the server is cutting me off after two
minutes.
Finally, I can give a Blocksize parameter to the server on RTSP
initial handshake. With this, I could specify a maximum packet size,
which would allow me to preallocate all of my internal packets (using
around 2k buffers each, since MTU is 1500 by default). This would
basically mean that there would be initial memory allocation for
packets until I had enough in the packet pool, and I could retire
them to another linked list and pull them from there. So there would
be no malloc's after an initial startup period. I think this would
be a good thing, but I'm not sure how the rest of FFMPEG feels about
memory allocation. Is this a better approach?
More information about the ffmpeg-devel
mailing list