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

[Michael Niedermayer]

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?

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


> 
> The packetizing code is largely modelled after rtp_aac.c - should the 
> copyright statements from that file be added here, too?

[...]
> +#include "avformat.h"
> +#include "rtpenc.h"
> +

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


> +
> +/* Packetize AMR frames into RTP packets according to RFC 3267 */
> +void ff_rtp_send_amr(AVFormatContext *s1, const uint8_t *buff, int size)

not doxygen compatible comment



> +{
> +    RTPMuxContext *s = s1->priv_data;
> +    int len, max_packet_size;
> +    uint8_t *p;
> +
> +    max_packet_size = s->max_payload_size - MAX_HEADER_TOC_SIZE;
> +
> +    /* test if the packet must be sent */
> +    len = (s->buf_ptr - s->buf);

> +    if ((s->num_frames == MAX_FRAMES_PER_PACKET) || (len && (len + size) > max_packet_size)) {

superflous ()


> +        int header_size = s->num_frames + 1;
> +        p = s->buf + MAX_HEADER_TOC_SIZE - header_size;
> +        if (p != s->buf) {
> +            memmove(p, s->buf, header_size);
> +        }
> +
> +        ff_rtp_send_data(s1, p, s->buf_ptr - p, 1);
> +
> +        s->num_frames = 0;
> +    }
> +    if (s->num_frames == 0) {
> +        s->buf[0] = 0xf0;
> +        s->buf_ptr = s->buf + MAX_HEADER_TOC_SIZE;
> +        s->timestamp = s->cur_timestamp;
> +    }
> +
> +    if (size < max_packet_size) {
> +        if (s->num_frames > 0) {
> +            /* Mark the previous TOC entry as having more entries following */
> +            s->buf[1 + s->num_frames - 1] |= 0x80;
> +        }
> +        /* Copy the frame type and quality bits */
> +        s->buf[1 + s->num_frames++] = buff[0] & 0x7C;
> +        buff++;
> +        size--;
> +        memcpy(s->buf_ptr, buff, size);
> +        s->buf_ptr += size;
> +    } else {

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

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

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/40011851/attachment.pgp>