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

[Martin Storsjö]

Hi Luca,

On Mon, 6 Apr 2009, Luca Abeni wrote:

> On Mon, 2009-04-06 at 12:21 +0300, Martin Storsj? wrote:
>
> > In normal use, no, this situation isn't possible. The largest AMR-NB
> > frame is 32 bytes, and the largest AMR-WB frame is 62 bytes. The RFC
> > doesn't mention any way of splitting frames, as far as I can see.
> > However, I still think the error logging is good to keep, for example
> > if the user has set the max packet size to some very low value (for
> > some strange reason), the user will at least be warned that nothing
> > actually is sent.
>
> I think in this case the right thing to do is to perform the check in
> rtp_write_header(), and to fail if the payload size is too small. (I do
> not like the idea of a function failing without returning an error code)

Ok, fixed. I had to move the default case for max_frames_per_packet to 
rtp_write_header in order to get a sensible check for the minimum 
max_payload_size.

> If you add the check described above (and remove the useless "else"),
> the patch would be ok as far as I can see.

Redundant else removed

> > I also added a check for the channel count, since the code currently
> > only supports mono.
> That's ok. I suspect the RFC mandates mono audio?

No, the RFC actually specifies how to format multichannel audio, too. But
this is quite hard to support in the RTP muxer before we actually can get
any form of multichannel AMR loaded either by some demuxer or encoded by
libavcodec.

> > (No AMR encoder or demuxer currently supports anything else than mono,
> > but a user of libavformat could be feeding some other data...)

> > I added this check in rtp_write_header; is this the right place or
> > does it belong to ff_rtp_send_amr instead?

> The check in rtp_write_header() is IMHO the right way to go.

Ok, good.


When checking for the corner cases in handling max_payload_size, I found a 
minor bug that I had inherited from rtp_aac.c; I fixed this in my version, 
and a fix for the case in rtp_aac.c is attached as a separate patch. 

     /* 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)) {
+    if ((s->num_frames == MAX_FRAMES_PER_PACKET) || (len && (len + size) > s->max_payload_size)) {

Since s->buf_ptr starts from s->buf + MAX_AU_HEADERS_SIZE, len shouldn't 
be checked against max_packet_size = s->max_payload_size - 
MAX_AU_HEADERS_SIZE, but checked against s->max_payload_size instead.

And the second hunk of the patch fixes cases where the data packet would 
fit exactly into the space available.

// Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg-rtp-amr.patch
Type: text/x-diff
Size: 6802 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090407/be6caaab/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg-rtp-aac.patch
Type: text/x-diff
Size: 955 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090407/be6caaab/attachment-0001.patch>