[FFmpeg-soc] G723.1 Frame Parser

[Mohamed Naufal]

On 1 April 2010 02:14, Ronald S. Bultje <rsbultje at gmail.com> wrote:

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


Fixed.


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


Fixed.


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


Done.


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


Removed.


>
> > +    // 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)?
>


You're right. Sorry. Corrected now. I suppose I'll need a parser too. I
believe the (yet to be committed) amr parser can be modified for this too.

Regards,
Naufal

[...]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: g723.1.patch
Type: text/x-patch
Size: 11126 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100405/13e6bacd/attachment.bin>