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

[Luca Abeni]

Hi Martin,

On Mon, 2009-04-06 at 12:21 +0300, Martin Storsj? 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.
Good, thanks

> > 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.
Uhm... Yes, you are probably right; thanks for looking at this anyway.

[...]
> > > +        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.
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)

> 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.
If you add the check described above (and remove the useless "else"),
the patch would be ok as far as I can see.

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


			Thanks,
				Luca