[Ffmpeg-devel] RTP patches & RFC

[Michael Niedermayer]

Hi

On Thu, Oct 26, 2006 at 11:17:16PM -0500, Ryan Martell wrote:
> 
> 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  
> these...
> 
> 

[small indention fix patch]

looks ok


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

[AAC move patch]

looks ok


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

hmm, rfc says:
"
   When SDP Offer/Answer model or any other capability exchange
   procedure is used in session setup, the properties of the received
   stream SHOULD be such that the receiver capabilities are not
   exceeded.  In the SDP Offer/Answer model, the receiver can indicate
   its capabilities to allocate a deinterleaving buffer with the deint-
   buf-cap MIME parameter.  The sender indicates the requirement for the
   deinterleaving buffer size with the sprop-deint-buf-req MIME
   parameter.  It is therefore RECOMMENDED to set the deinterleaving
   buffer size, in terms of number of bytes, equal to or greater than
   the value of sprop-deint-buf-req MIME parameter.  See section 8.1 for
   further information on deint-buf-cap and sprop-deint-buf-req MIME
   parameters and section 8.2.2 for further information on their use in
   SDP Offer/Answer model.
"

"
       deint-buf-cap:   This parameter signals the capabilities of a
                        receiver implementation and indicates the
                        amount of deinterleaving buffer space in units
                        of bytes that the receiver has available for
                        reconstructing the NAL unit decoding order.  A
                        receiver is able to handle any stream for which
                        the value of the sprop-deint-buf-req parameter
                        is smaller than or equal to this parameter.

                        If the parameter is not present, then a value
                        of 0 MUST be used for deint-buf-cap.  The value
                        of deint-buf-cap MUST be an integer in the
                        range of 0 to 4294967295, inclusive.

                            Informative note: deint-buf-cap indicates
                            the maximum possible size of the
                            deinterleaving buffer of the receiver only.

                            When network jitter can occur, an
                            appropriately sized jitter buffer has to
                            be provisioned for as well.
"

"
      As specified above, an offerer has to include the size of the
      deinterleaving buffer in the offer for an interleaved H.264
      stream.  To enable the offerer and answerer to inform each other
      about their capabilities for deinterleaving buffering, both
      parties are RECOMMENDED to include "deint-buf-cap".  This
      information MAY be used when the value for "sprop-deint-buf-req"
      is selected in a second round of offer and answer.  For
      interleaved streams, it is also RECOMMENDED to consider offering
      multiple payload types with different buffering requirements when
      the capabilities of the receiver are unknown.
"

this doesnt sound like the reordering is required for receivers ... or did
i missunderstand the rfc?


> 
> >>
> >>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:
> 	Pros:
> 		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..).

invalid data must be handled sanely anyway when the source is the internet


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

good


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

i did and would be happy if you could try it instead of the large
thing below, if it doesnt work tell me and ill look at it

also, if you send unrelated patches as individual mails, they will get
replies individually and consequently get applied quicker ...

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