[FFmpeg-devel] [PATCH] Add support for packetizing AMR into RTP

[Michael Niedermayer]

On Fri, Apr 03, 2009 at 07:33:52PM +0300, Martin Storsj? wrote:
> On Fri, 3 Apr 2009, Michael Niedermayer wrote:
> 
> > On Fri, Apr 03, 2009 at 07:03:35PM +0300, Martin Storsj? wrote:
> > > Hi,
> > > 
> > > The attached patch adds support for packetizing AMR (both NB and WB) into 
> > > RTP packets, according to RFC 3267.
> > 
> > against which RTP clients has this code been tested?
> 
> AMR-NB has been tested with QuickTime Player and with RealPlayer on 
> Symbian/S60, AMR-WB has only been tested with the same RealPlayer (since 
> QuickTime doesn't support AMR-WB).

good


[...]
> > > +#define MAX_FRAMES_PER_PACKET (s->max_frames_per_packet ? s->max_frames_per_packet : 12)
> > > +#define MAX_HEADER_TOC_SIZE (1 + MAX_FRAMES_PER_PACKET)
> > 
> > iam against use of #defines to hide non constants unless theres some sense
> > in it. (i doubt there is here)
> 
> Ok, so I should move them into the function, making them const ints?

thats an option i suspect


> 
> > > +        if (s->buf_ptr != s->buf + MAX_HEADER_TOC_SIZE) {
> > > +            av_log(s1, AV_LOG_ERROR, "Strange...\n");
> > > +            av_abort();
> > > +        }
> > 
> > you cannot call *abort() in a library in general
> 
> This is the same as in rtp_aac.c;

so its broken too


> it should only fail if there's some 
> large logic error in the code, like some kind of assertion. It could, in 
> principle, be removed altoghether. Or would a plain assert() be better?

an assert() would be better

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090403/1f165f4e/attachment.pgp>