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

[Martin Storsjö]

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

> also this patch needs to be reviewed by some of our RT*P experts, my review
> just for the obvious non RT*P issues

Ok, I'll await some more comments before sending a revised patch.

> > +#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?

> > +        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; 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?

// Martin