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

[Martin Storsjö]

Hi Luca,

Thanks for the review!

Answers to your questions below:

On Sun, 5 Apr 2009, Luca Abeni wrote:

> On Fri, 2009-04-03 at 19:03 +0300, Martin Storsj? wrote:
> > The packetizing code is largely modelled after rtp_aac.c - should the 
> > copyright statements from that file be added here, too?
> After reading the patch, I think yes.

Ok, copyright statement added.

> I am also wondering if it is possible to factorise some code between the 
> two files...

Perhaps yes, but I'm not totally convinced of how much the code size 
actually would be reduced. The refactored general part would perhaps look 
something like this:

ff_send_audio
  if (num_frames == max_num_frames or (pos > 0 and pos + size >= 
                                       max_packet_size))
    move header if necessary
    send stored packet
    num_frames = 0

  if (num_frames == 0)
    init buffering
    store timestamp

  if (size < max_packet_size)
    append frame to buffer
  else
    fail

This still would require codec specific implementation of both initing the 
buffering and appending the frames to the buffer, and for calculating the 
header sizes.

> > +/* Packetize AMR frames into RTP packets according to RFC 3267 */
> > +void ff_rtp_send_amr(AVFormatContext *s1, const uint8_t *buff, int size)
> > +{
> This patch is implementing the "Octet-aligned Mode", right? Can you
> clarify this in the comment?

Clarified

> > +    } else {
> > +        if (s->buf_ptr != s->buf + MAX_HEADER_TOC_SIZE) {
> > +            av_log(s1, AV_LOG_ERROR, "Strange...\n");
> > +            av_abort();
> > +        }
> > +        av_log(s1, AV_LOG_ERROR, "Unable to split an AMR frame into more than one RTP packet\n");
> > +    }
> Is this situation (AMR frame larger than the packet payload) possible?
> If yes, how does the RFC address it? (in case the RFC provides some way
> to split an AMR frame, how difficult would it be to implement it? How
> probable is this situation?)
> If for some reason this situation is not possible (if I understand well,
> AMR frames have a fixed length in time, and a fixed bitrate of 8 or 16
> kbps?), then the "} else {" branch can be removed, I think...

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.


A new patch is attached, addressing the things pointed out by you and 
Michael. Additionally, I added a changelog entry mentioning that this has 
been added, and made some more minor cosmetic formatting/cleanup in 
rtp_amr.c.

I also added a check for the channel count, since the code currently only 
supports mono. (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?

Regards,
// Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg-rtp-amr.patch
Type: text/x-diff
Size: 6915 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090406/0ffa718c/attachment.patch>