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

[Luca Abeni]

Hi Martin,

Martin Storsj? wrote:
[...]
>> 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

Committed, thanks (with few small modifications to make patcheck happier).


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

Uh... Looks like you are right. I'll commit this later.


			Thanks again,
				Luca