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 _______________________________________________ FFmpeg-soc mailing list [email protected] https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
