[FFmpeg-devel] rmdec.c: add SIPR codec try #2

[Kostya]

On Tue, Mar 17, 2009 at 02:29:03PM -0400, Ronald S. Bultje wrote:
> Hi Kostya & all,
> 
> On Tue, Mar 17, 2009 at 10:28 AM, Kostya <kostya.shishkov at gmail.com> wrote:
> > On Tue, Mar 17, 2009 at 09:29:39AM -0400, Ronald S. Bultje wrote:
> >> + ? ? ? ? ? ?ast->audio_framesize = st->codec->block_align;
> >> + ? ? ? ? ? ?if (st->codec->codec_id == CODEC_ID_SIPR) {
> >> + ? ? ? ? ? ? ? ?st->codec->block_align = sipr_subpk_size[flavor];
> >
> > bounds check needed (flavor is read as 16 bits and you have only 4
> > subpacket sizes)
> 
> Changed.
> 
> >> + ? ? ? ? ? ? ? ?/* perform reordering */
> >> + ? ? ? ? ? ? ? ?int n;
> >> + ? ? ? ? ? ? ? ?int bs = h * w * 2 / 96; ?// nibbles per subpacket
> >> +
> >> + ? ? ? ? ? ? ? ?for (n = 0; n < 38; n++) {
> >> + ? ? ? ? ? ? ? ? ? ?int j;
> >> + ? ? ? ? ? ? ? ? ? ?int i = bs * sipr_swaps[n][0];
> >> + ? ? ? ? ? ? ? ? ? ?int o = bs * sipr_swaps[n][1];
> >> + ? ? ? ? ? ? ? ? ? ?uint8_t *buf = ast->pkt.data;
> >> +
> >> + ? ? ? ? ? ? ? ? ? ?/* swap 4bit-nibbles of block 'i' with 'o' */
> >> + ? ? ? ? ? ? ? ? ? ?for (j = 0; j < bs; j++, i++, o++) {
> >> + ? ? ? ? ? ? ? ? ? ? ? ?int x = (buf[i >> 1] >> (4 * (i & 1))) & 0xF,
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?y = (buf[o >> 1] >> (4 * (o & 1))) & 0xF;
> > [style nit suppressed]
> >> +
> >> + ? ? ? ? ? ? ? ? ? ? ? ?buf[o >> 1] = (x << (4 * (o & 1))) |
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?(buf[o >> 1] & (0xF << (4 * !(o & 1))));
> >> + ? ? ? ? ? ? ? ? ? ? ? ?buf[i >> 1] = (y << (4 * (i & 1))) |
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?(buf[i >> 1] & (0xF << (4 * !(i & 1))));
> >
> > this is not very readable and the value is swapped twice.
> > I think full swapping should be done at each iteration, probably
> > with splitting for (i&1 == o&1) and (i&1 != o&1) cases
> 
> Gotcha, I think the attached should be faster. Not the prettiest
> stylistically... Anyway, resulting frames are still identical to
> original code.
> 
> >> + ? ? ? ? ? ? ? ? ? ?}
> >> + ? ? ? ? ? ? ? ?}
> >> + ? ? ? ? ? ?}
> >> +
> >
> > better move it to separate function like AC3 byte swapping
> > (or merge them into common codec_data_swap()
> 
> Done, see attached.
> 
> Ronald

[...]
The rest of the patch seems fine, let's review the
juiciest chunk in details.

> @@ -637,6 +661,39 @@
>      }
>  }
>  
> +/** perform 4-bit block reordering for SIPR data */
> +static void
> +rm_reorder_sipr_data (RMStream *ast)
> +{
> +    int n, bs = ast->sub_packet_h * ast->audio_framesize * 2 / 96; // nibbles per subpacket
> +
> +    for (n = 0; n < 38; n++) {

It would be nice to comment where that came from.
38 unlike 16, 256, 1024 or 42 is not an obvious number.

> +        int j;
> +        int i = bs * sipr_swaps[n][0];
> +        int o = bs * sipr_swaps[n][1];
> +        uint8_t *buf = ast->pkt.data;
> +
> +#define LOOP_ITER \
> +        for (j = 0; j < bs; j++, i++, o++) { \
> +            int mask1 = (i & 1) ? 0xF0 : 0x0F, \
> +                mask2 = (i & 1) ? 0x0F : 0xF0, \
> +                tmp = buf[i >> 1] & mask1;
> +#define LOOP_END }
> +
> +        /* swap 4bit-nibbles of block 'i' with 'o' */
> +        if (!((i ^ o) & 1)) LOOP_ITER {
> +            buf[i >> 1] = (buf[i >> 1] & mask2) | (buf[o >> 1] & mask1);
> +            buf[o >> 1] = (buf[o >> 1] & mask2) | tmp;
> +        } LOOP_END else if (i & 1) LOOP_ITER {
> +            buf[i >> 1] = (buf[i >> 1] & mask2) | ((buf[o >> 1] & mask2) << 4);
> +            buf[o >> 1] = (buf[o >> 1] & mask1) | (tmp >> 4);
> +        } LOOP_END else /* o & 1 */ LOOP_ITER {
> +            buf[i >> 1] = (buf[i >> 1] & mask2) | ((buf[o >> 1] & mask2) >> 4);
> +            buf[o >> 1] = (buf[o >> 1] & mask1) | (tmp << 4);
> +        } LOOP_END;

Yes, that's horrible.
Let's return to your original version and I'll try to optimize it in SVN.

> +    }
> +}
> +
>  int
>  ff_rm_parse_packet (AVFormatContext *s, ByteIOContext *pb,
>                      AVStream *st, RMStream *ast, int len, AVPacket *pkt,