[Ffmpeg-devel] RTP patches & RFC

Ryan Martell rdm4
Fri Oct 27 06:17:16 CEST 2006

On Oct 26, 2006, at 8:07 PM, Michael Niedermayer wrote:

> Hi
> On Thu, Oct 26, 2006 at 06:46:19PM -0500, Ryan Martell wrote:
>> 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
> [...]

Here's most of my indention issues fixed; if I missed any, I'll catch  
them as I update the other code.  I'll need to get more familiar with  
the svn diff of older revisions to make sure that i catch all of  

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

> [...]
>>> [...]

[ .. me whining deleted ...]
>> 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)
> well that can be done, or at least you can try ...
> there are many ways, one is just to have a directory for each "state"
> so theres the unchanged-svn-head/myfile.c, ...
> and indent-fixup/myfile.c, ....
> and indent-fixup-and-aac-move/myfile.c, ...
> and then you just use diff between the directories (without any svn  
> involvment)
> another solution is to install svn (server) locally and commit to that
> that way you can use svn diff
> yet another is to hope that the patches wont conflict
> so you build your first patch, and then reverse it locally
> (svn revert or patch -R) and then build your second as if the first
> didnt exist (this of course can fail when we attepmt to apply the  
> patch
> if the patches change the same lines of code ...)
> and then there are things like git which some people use locally to
> simplify their work with patches, sadly i cant say much about that as
> ive never really used it, though  at least one developer who has used
> it successfully submitted large numbers of patches within short  
> time ...
> the most ideal case is to submit changes early before they become very
> big, yes i know that doesnt help you now ...

Here's a patch of the sound movement stuff....

(This also added two cases below, but they aren't reached in this  
iteration, as both the h264 codec and the AAC codec return before  
getting there)  I removed the patches from above from this file by  
hand; hopefully I did that correctly.

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

Once those are applied, I'll add the rtp modification patch.

Next the h264 patch (Makefile, rtp modification, rtp_h264.c, rtp_h264.h)

Next I'll fix the AAC frequency/stereo initialization bug in a patch.

Then the RTCP statistics patch (maintaining the stats)

Then I'll integrate the RTCP sender stuff.

> [...]
>>>> /**
>>>>    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.
> hmm, is this other mode used by anyone? is it mandatory in the sense
> that if the decoder doesnt support it its out of luck instead of the
> server just switching to the normal mode?

The packetization modes are specified by the server, and I don't  
think you can request a different one.  0 is simply NALs coming  
across unaltered.  1 adds fragmenting large nals and conglomerating  
small nals.  2 adds decoding order & out of order stuff.  So yes, I  
think you are out of luck.

>> 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.
> well, but the decoder should be able to decode the part of a NAL unit
> until the missing part, so droping the whole just isnt correct
> but note, i dont know how well this currently works with h264.c,  
> its just
> supposed to work and does work with the mpeg/h263 decoders ...

We can do either.  Here's my thoughts on dropping the entire packet:
		I _know_ that it was not complete.
		That's what the RFC says to do.
		Prevents weird invalid data stream issues in the decode (it might  
not like it if it doesn't get a certain byte..).

		More complex networking code,

Personally, I like dropping.  On a similar note, I do occasionally on  
startup get a repeating error from the h264 decoder:

[h264 @ 0x396540]illegal short term buffer state detected

> and about receiving packets out of order due to network delay/routing
> (=sequence issues?) this is a generic problem and should be solved
> (or ignored) generically and not just in rtp-h264.c

That's handled in rtp.c with the code that I wrote for the statistics  
handling (from the RFC).  The function that gathers statistics also  
returns whether you should handle the packet or not, and that will  
handle the out of order sequence number stuff.

> [...]
>>> 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.c base64.h in libavformat, we can always move or rename it  
> later
>> base64 encode in the source somewhere, but it's static.
> that could also be moved into base64.c/h ...

base64.[ch].  I don't like the decode routine, i swiped it from  
elsewhere (and fixed it), I'm sure it can be improved on (I think you  
even sent a suggestion before)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: base64.c
Type: application/octet-stream
Size: 4594 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20061026/4f137fe6/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: base64.h
Type: application/octet-stream
Size: 936 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20061026/4f137fe6/attachment-0001.obj>
-------------- next part --------------


More information about the ffmpeg-devel mailing list