[FFmpeg-soc] G723.1 Frame Parser

[Ronald S. Bultje]

Hi,

On Wed, Mar 31, 2010 at 10:52 AM, Mohamed Naufal <naufal11 at gmail.com> wrote:
> Fixed the specified issues. Added an rtp depacketizer.

> +    temp = get_bits(&gb, 7);
> +    if (temp <= 123) {       // test if forbidden code
> +        frame->subframe[0].pitch_lag = temp + PITCH_MIN;
> +    } else {
> +        p->is_bad_frame  = 1; // transmission error
> +        return;
> +    }

return -1, and then handle the return value. That way you don't need
the (usually unused) bad_frame member in the context.

> +    frame->subframe[0].combined_gain = get_bits(&gb, 12);
> +    frame->subframe[1].combined_gain = get_bits(&gb, 12);
> +    frame->subframe[2].combined_gain = get_bits(&gb, 12);
> +    frame->subframe[3].combined_gain = get_bits(&gb, 12);
> +
> +    frame->subframe[0].grid_index = get_bits(&gb, 1);
> +    frame->subframe[1].grid_index = get_bits(&gb, 1);
> +    frame->subframe[2].grid_index = get_bits(&gb, 1);
> +    frame->subframe[3].grid_index = get_bits(&gb, 1);

for (i = 0; i < 4; i++) ..

Saves 3 lines of code, and the compiler will unroll it anyway.

> +        frame->subframe[0].pulse_pos = (temp / 90) / 9;
> +        frame->subframe[1].pulse_pos = (temp / 90) % 9;
> +        frame->subframe[2].pulse_pos = (temp % 90) / 9;
> +        frame->subframe[3].pulse_pos = (temp % 90) % 9;

Try using FASTDIV() here, and cache the result so you don't need the
%, but you can use temp - result * 90 (faster).

> +        frame->subframe[0].pulse_sign = get_bits(&gb, 6);
> +        frame->subframe[1].pulse_sign = get_bits(&gb, 5);
> +        frame->subframe[2].pulse_sign = get_bits(&gb, 6);
> +        frame->subframe[3].pulse_sign = get_bits(&gb, 5);
> +    } else { // Frame5k3
> +        frame->subframe[0].pulse_pos  = get_bits(&gb, 12);
> +        frame->subframe[1].pulse_pos  = get_bits(&gb, 12);
> +        frame->subframe[2].pulse_pos  = get_bits(&gb, 12);
> +        frame->subframe[3].pulse_pos  = get_bits(&gb, 12);
> +
> +        frame->subframe[0].pulse_sign = get_bits(&gb, 4);
> +        frame->subframe[1].pulse_sign = get_bits(&gb, 4);
> +        frame->subframe[2].pulse_sign = get_bits(&gb, 4);
> +        frame->subframe[3].pulse_sign = get_bits(&gb, 4);
> +    }

More loopable stuff.

> +++ b/libavformat/rtpdec_g723_1.c
[..]
> +    if (st->codec->codec_id != CODEC_ID_G723_1) {
> +        av_log(ctx, AV_LOG_ERROR, "Bad codec ID\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    if (st->codec->channels != 1) {
> +        av_log(ctx, AV_LOG_ERROR, "G.723.1 supports mono only");
> +        return AVERROR_INVALIDDATA;
> +    }

Both can be removed, decoder should check that, not RTP depacketizer.

> +    // The frame size and codec type is determined from
> +    // the least 2 bits of the first byte.
> +    frame_len         = frame_sizes[buf[0] & 3];
> +    pkt->stream_index = st->index;
> +    ptr               = pkt->data;
> +
> +    if (av_new_packet(pkt, len) < 0) {
> +        av_log(ctx, AV_LOG_ERROR, "Out of memory\n");
> +        return AVERROR_NOMEM;
> +    }
> +
> +    if (frame_len > len)
> +        av_log(ctx, AV_LOG_WARNING, "Too little data in the RTP packet\n");
> +
> +    memcpy(ptr, buf, len);
> +    pkt->size = len;
> +
> +    if (frame_len < len) {
> +        av_log(ctx, AV_LOG_WARNING, "Too much data in the RTP packet\n");
> +        ptr += frame_len;
> +        memset(ptr, 0, len - frame_len);
> +        pkt->size = frame_len;
> +    }

That looks messy, are you sure that's correct? I figure frames are
generally small and RTP packets would be bigger, so this would trigger
all the time. Shouldn't this split the packets? Or simply copy them
all (as the AMR depacketizer does)?

Ronald