[FFmpeg-soc] RTP packetizers

[Martin Storsjö]

Hi Luca,

CC:ing this discussion to -soc, to let Josh and others take part in it, 
too.

On Mon, 9 Aug 2010, Luca Abeni wrote:

> Hi Martin,
> 
> I had a look at the code you committed, and I have few
> questions/suggestions/comments
> (sorry if they have been already discussed)
> 1) ident. I do not know what it is, but I see it must be identical in the RTP
>    payload and in the SDP... I think it would be good to have a constant (or a
>    #define) somewhere in a header file, and use it. For example, change
> 	    *q++ = 0xfe;
> 	    *q++ = 0xcd;
> 	    *q++ = 0xba;
>    into
> 	    *q++ = (XIPH_IDENT >> 16) & 0xFF;
> 	    *q++ = (XIPH_IDENT >>  8) & 0xFF;
> 	    *q++ = XIPH_IDENT         & 0xFF;
>    in rtpenc_xiph.c (and a similar change in sdp.c).
>    In this way, if "ident" is changed for some reason, the change is propagated
>    in both sdp and rtpenc.

Changed it to use a define like suggested, commited.

>    As an alternative, you can implement a function returning
>    the "ident" (in this way, it can be changed from stream to stream). Or you  can
>    store it somewhere in the AVCodecContext

That's an option, too, but a bit more complicated, and as far as I know, 
not really needed in practice (yet at least).

> 2) If I understand the code correctly, the default of max_frames_per_packet is set
>    to 15 for theora... Is this requested somewhere? For video, it would be more
>    common to have 1 frame per packet...

Yes, I guess that would be more sensible. If you pack multiple frames into 
one packet, you can't really make out the timestamps of the later frames.

However, the spec (at 
http://svn.xiph.org/trunk/theora/doc/draft-ietf-avt-rtp-theora-00.txt) 
says:

   Any Theora data packet that is less than path MTU SHOULD be bundled
   in the RTP packet with as many Theora packets as will fit, up to a
   maximum of 15.

So I'm a bit undecided about this.

> 3) In rtpenc_xiph.c, can s->num_frames be larger than s->max_frames_per_packet?
>    If yes, how? If not, the
> 	if ((s->num_frames > 0 && remaining < 0) ||
>             s->num_frames >= s->max_frames_per_packet) {
>    condition should be changed since it is confusing...

It shouldn't ever be larger - personally I prefer this code style since 
it's more robust to unforseen issues.

> 4) Don't the RFCs define some specific way to split a frame in case of fragmentation?
>    (I mean: generally, the various payload specifications mandate how to fragment
>    a frame, splitting it in some specific points so that lost packets can be better
>    tolerated - for example, MPEG{1,2} frames must be split in slices).
>    If I correctly understand the code, rtpenc_xiph.c splits the frames at random
>    points...

Yes, it just splits packets at random points, I don't see anything else 
mentioned in the RFC. (FWIW, the same is done for MPEG4 video, too.)

> > The VP8 packetizer spec proposal was updated a few days ago, so once Josh
> > updates the code according to that, I think that one is ready to be
> > committed, too.
> 
> Ok... Having a working VP8 in RTP stack would be very nice :)
> But the latest VP8/RTP proposal I saw (it was some weeks ago, so I did not
> look at the updated one) did not make too much sense. I suppose it will change
> a lot in the future... So, I think it would be good to mark the packetiser as
> experimental in some way (if there are no other ways, just add an av_log() in
> write_header()).

That's a good idea, thanks!

// Martin