[Tremor] bitwise.c question

[Hans Petter Selasky]

Hi,

First, your bitwise.c is very cleverly written. Second, I plan to change the 
behaviour of oggpack_read(), so that it returns 0, if EOF is reached, instead 
of -1. Then have a separate function oggpack_get_valid_1() that will return 
true if there is 1 bit or more available in the buffer.

I see it is a pitfall that oggpack_read() can return -1. Consider this:

str = table[oggpack_read(&opb, 5)];

Does anyone have any comments on this change. Also I have a new bitwise.c that 
is some percent faster. Up to 50% for "bits > 20":

/* new */

/********************************************************************
 *                                                                  *
 * THIS FILE IS PART OF THE OggVorbis 'TREMOR' CODEC SOURCE CODE.   *
 *                                                                  *
 * USE, DISTRIBUTION AND REPRODUCTION OF THIS LIBRARY SOURCE IS     *
 * GOVERNED BY A BSD-STYLE SOURCE LICENSE INCLUDED WITH THIS SOURCE *
 * IN 'COPYING'. PLEASE READ THESE TERMS BEFORE DISTRIBUTING.       *
 *                                                                  *
 * THE OggVorbis 'TREMOR' SOURCE CODE IS (C) COPYRIGHT 1994-2002    *
 * BY THE Xiph.Org FOUNDATION http://www.xiph.org/                  *
 *                                                                  *
 ********************************************************************

  function: packing variable sized words into an octet stream

 ********************************************************************/

/* We're 'LSb' endian; if we write a word but read individual bits,
   then we'll read the lsb first */

#include <string.h>
#include <stdlib.h>
#include <stdio.h>
#include <sys/endian.h>
#include "ogg.h"

static const u_int32_t mask[]=
{0x00000000,0x00000001,0x00000003,0x00000007,0x0000000f,
 0x0000001f,0x0000003f,0x0000007f,0x000000ff,0x000001ff,
 0x000003ff,0x000007ff,0x00000fff,0x00001fff,0x00003fff,
 0x00007fff,0x0000ffff,0x0001ffff,0x0003ffff,0x0007ffff,
 0x000fffff,0x001fffff,0x003fffff,0x007fffff,0x00ffffff,
 0x01ffffff,0x03ffffff,0x07ffffff,0x0fffffff,0x1fffffff,
 0x3fffffff,0x7fffffff,0xffffffff };

/* the following will allow reading 
 * un-aligned 32-bit integers 
 * on some CPUs:
 */
struct u_int32_p {
   u_int32_t data;
} __attribute__((__packed__));

typedef struct u_int32_p u_int32_p_t;

struct u_int16_p {
   u_int16_t data;
} __attribute__((__packed__));

typedef struct u_int16_p u_int16_p_t;

static u_int32_t
oggpack_fetch_block(oggpack_buffer *buf)
{
    register ogg_reference *ref = buf->data_next;
    u_int32_t retval;

    /* advance in chained list of buffers */

    if (ref) {

        buf->data_pointer = 
          ref->buffer->data + 
          ref->begin ;

        buf->data_length = 
          ref->length;

        buf->data_next = 
          ref->next;

        retval = 0;

    } else {

        retval = 1;
    }
    return retval;
}

static void
oggpack_adv_byte(oggpack_buffer *buf)
{
    if(buf->data_length) {
       buf->data_pointer++;
       buf->data_length--;
    }
    return;
}

static u_int32_t 
oggpack_get_byte(oggpack_buffer *buf)
{
    while (buf->data_length == 0) {
        if(oggpack_fetch_block(buf)) {
            break;
        }
    }

    return buf->data_length ?
      *(buf->data_pointer) : 0;
}

/* oggpack_readinit - initialize a "oggpack_buffer"
 *
 */
void
oggpack_readinit(oggpack_buffer *buf, ogg_reference *ref)
{
    bzero(buf, sizeof(*buf));

    buf->data_next = ref;
    return;
}

/* oggpack_read - read 'bits' number of bits and
 *                advance the logical data stream
 *
 * NOTE: "bits <= 32" 
 */
u_int32_t
oggpack_read_m(oggpack_buffer *buf, u_int32_t bits)
{
    u_int32_t temp;
    u_int32_t shift;
    u_int8_t *ptr;
    u_int8_t buffer[5];

    if (bits > 32) {
        /* should not happen */
        bits = 32;
    }

    /* get mask */

    temp = mask[bits];

    /* get right-shift */

    shift = buf->data_bit_level;

    /* compute position of end-bit, exclusive */

    bits += shift;

    buf->data_bit_level = (bits & 7);

    if (buf->data_length < 5) {

        /* slow way to get the data */

        ptr = &buffer[0];

        *ptr++ = oggpack_get_byte(buf);

        while(bits >= 8) {

            oggpack_adv_byte(buf);

            if (bits > 8) {

                *ptr++ = oggpack_get_byte(buf);
            }

            bits -= 8;
        }

        ptr = &buffer[0];
        bits = 4; /* bytes */

    } else {

        ptr = buf->data_pointer;

        /* the fast way to get data;
         * 99% of the requests go here 
         */
        bits >>= 3;

        buf->data_length -= bits;
        buf->data_pointer += bits;
    }

    if (bits >= 2) {
      temp &= htole32(((u_int32_p_t *)ptr)->data) >> shift;

      if (bits >= 4) {
          if (shift) {
              temp &= ptr[4] << (32-shift);
          }
      }
    } else {
      temp &= htole16(((u_int16_p_t *)ptr)->data) >> shift;
    }
    return temp;
}

/* oggpack_look - read 'bits' number of bits and
 *                do not advance the logical data stream
 *
 * NOTE: "bits <= 32" 
 */
u_int32_t
oggpack_look_m(oggpack_buffer *buf, u_int32_t bits)
{
    oggpack_buffer buf_old = *buf;
    u_int32_t temp = oggpack_read_m(buf, bits);
    *buf = buf_old;
    return temp;
}

/* oggpack_adv - advance the logical data stream 
 *               'bits' number of bits
 *
 * NOTE: "bits <= 32" 
 */
void
oggpack_adv_m(oggpack_buffer *buf, u_int32_t bits)
{
    (void) oggpack_read_m(buf, bits);
    return;
}


Any comments ?

--HPS