On Mon, 9 Aug 2010, Luca Abeni wrote:
> On 08/09/2010 12:39 PM, Martin Storsjö wrote:
> [...]
> > > 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.
> Sorry, I do not know the theora terminology too well... Is a "theora packet"
> a frame? Or is it some smaller entity (such as a NAL for H.264)?
> In the first case, this requirement seems a little bit strange... Maybe
> someone knows why the draft is written in this way?
As far as I know, a theora packet is a complete frame. I guess Luca B can
shed some light on this issue?
> > > 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.
> I agree it's a matter of personal preferences...
> But if something unforseen happens, I'd like to be notified (so, I'd
> change it to "s->num_frames == s->max_frames_per_packet", and add an
> "assert(s->num_frames <= s->max_frames_per_packet)" just in case...).
True... Something like the attached, then?
> > > 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.
> If a "theora packet" is something smaller than a video frame (see above), I
> think this makes sense.
>
> > (FWIW, the same is done for MPEG4 video, too.)
> You got me :)
> mpeg4 video is currently handled in a suboptimal (and non standard) way.
> AFAIR, the RFC requires to properly split a frame according to some headers,
> but we currently use rtp_send_raw(). A proper rtp_send_mpeg4v() needs to be
> implemented, to increase error resilience and to be fully RFC-compliant...
> If noone does this, I'll have a look in the next weeks.
Oh, I never looked that far on this issue. Yes, it would be very much
appreciated if you'd improve this, don't think I'll have the time to look
into it.
// MartinFrom 803b1f957bf0df1554741f660247a85afff4f7f3 Mon Sep 17 00:00:00 2001
From: Martin Storsjo <[email protected]>
Date: Mon, 9 Aug 2010 14:38:31 +0300
Subject: [PATCH] Change a >= into a == since the greater-than case shouldn't happen, add an assert
---
libavformat/rtpenc_xiph.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/libavformat/rtpenc_xiph.c b/libavformat/rtpenc_xiph.c
index 10576c2..5768632 100644
--- a/libavformat/rtpenc_xiph.c
+++ b/libavformat/rtpenc_xiph.c
@@ -72,8 +72,9 @@ void ff_rtp_send_xiph(AVFormatContext *s1, const uint8_t *buff, int size)
uint8_t *ptr = s->buf_ptr + 2 + size; // what we're going to write
int remaining = end_ptr - ptr;
+ assert(s->num_frames <= s->max_frames_per_packet);
if ((s->num_frames > 0 && remaining < 0) ||
- s->num_frames >= s->max_frames_per_packet) {
+ s->num_frames == s->max_frames_per_packet) {
// send previous packets now; no room for new data
ff_rtp_send_data(s1, s->buf, s->buf_ptr - s->buf, 0);
s->num_frames = 0;
--
1.7.1
_______________________________________________
FFmpeg-soc mailing list
[email protected]
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
