Unpacking 12bits integer type into 16bits

[mathieu]

Hi there,

I am looking for comments on the following piece of code: is this
efficiently written ? I am only focusing on little endian system for
now.

out : will contains a buffer of 16bits values where each value is at
most 12bits
in: contains *packed* 12bits value
n: is the size in char of in (multiple of 2).

char *unpack12into16(char *out, const char *in, size_t n)
{
uint16_t *out16 = (uint16_t*)out;
char *pout = out;
const uint8_t *p = (const uint8_t*)in;
const uint8_t * end = p + n;
for(; p != end; )
{
uint8_t b0, b1, b2;
b0 = *p++;
b1 = *p++;
b2 = *p++;
*out16++ = ((b0 >> 4) << 8) + ((b0 & 0x0f) << 4) + (b1 & 0x0f);
*out16++ = ((b2 & 0x0f) << 8) + ((b1 >> 4) << 4) + (b2 >> 4);
}
return pout;
}

Thanks
-Mathieu

[Peter Nilsson]

mathieu wrote:
> Hi there,
>
> I am looking for comments on the following piece
> of code: is this efficiently written?

It's not inefficient.

> I am only focusing on little endian system for
> now.

Your unpack should mirror the pack.

> out : will contains a buffer of 16bits values where
> each value is at most 12bits
> in: contains *packed* 12bits value

How are they packed? Your code suggests they are packed
in nibbles.

> n: is the size in char of in (multiple of 2).

n needs to be a multiple of 3.

>
> char *unpack12into16(char *out, const char *in, size_t n)

Unsigned char is better than char (which may be signed).

> {
> uint16_t *out16 = (uint16_t*)out;
> char *pout = out;

You don't need pout as you never modify out.

> const uint8_t *p = (const uint8_t*)in;
> const uint8_t * end = p + n;
> for(; p != end; )
> {
> uint8_t b0, b1, b2;
> b0 = *p++;
> b1 = *p++;
> b2 = *p++;
> *out16++ = ((b0 >> 4) << 8) + ((b0 & 0x0f) << 4) + (b1 & 0x0f);
> *out16++ = ((b2 & 0x0f) << 8) + ((b1 >> 4) << 4) + (b2 >> 4);

So input bytes 0x12, 0x34, 0x56 extract to 0x0124, 0x0635?!

> }
> return pout;
> }

I would write it like...

void *unpack12into16_pn(void *out, const void *in, size_t n)
{
const uint8_t *in8 = in;
uint16_t *out16 = out;
unsigned full, half;

for (n /= 3; n--; )
{
full = *in8++;
half = *in8++;
*out16++ = (full << 4) | (half & 0x0F);

full = *in8++;
*out16++ = ((full & 0x0F) << 8) | (half & 0xF0) | (full >> 4);
}

return out;
}

Which only really changes ((b0 >> 4) << 8) + ((b0 & 0x0f) << 4)
to the obvious (with the specs spelled out) b0 << 4. Note that
b0 will promote to int which can support 0xFFF (4095).

--
Peter

[Bartc]

"mathieu" <mathieu....@gmail.com> wrote in message
news:bd84e845-c059-4f2a...@c65g2000hsa.googlegroups.com...

Does this code work? I couldn't get it to function properly, although we may
have different assumptions about bit-ordering.

I did try this version which appears to work. In this code, I know that char
is 8 bits and short is 16 bits:

void myunpack(short *q, unsigned char *p, int n)
{
unsigned char *end = p+n;
unsigned char b0,b1,b2;

while (p!=end)
{

b0 = *p++;
b1 = *p++;
b2 = *p++;

*q++ = ((b1 & 0xf) << 8) + b0;
*q++ = (b1>>4) + (b2<<4);
}
}

Even if we use different bit ordering, you appear to have quite a few extra
shifts in yours.

And I think there is some scope for improvement if you need speed. I've
moved declarations b0,b1,b2 outside the loop (probably makes no difference,
but just in case).

If your computer allows unaligned access, you may be able to read b0,b1 as a
single 16-bit value, and the bottom 12 bits will be retained. The top 4 is
added to the next byte. (You seem to be making the assumption that n is a
multiple of 3.)

--
Bartc

[mathieu]

On Apr 18, 1:04 am, "Bartc" <b...@freeuk.com> wrote:
> "mathieu" <mathieu.malate...@gmail.com> wrote in message

Indeed you were right ! My code was bogus. Thanks a bunch !

-Mathieu