[FFmpeg-devel] [PATCH] Added integer 32 bits support to wavpack

[Laurent Aimar]

On Fri, May 01, 2009, Kostya wrote:
> > diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
> > index f31402d..0aded35 100644
> > --- a/libavcodec/wavpack.c
> > +++ b/libavcodec/wavpack.c
> > @@ -57,7 +57,7 @@ enum WP_ID{
> >      WP_ID_INT32INFO,
> >      WP_ID_DATA,
> >      WP_ID_CORR,
> > -    WP_ID_FLT,
> > +    WP_ID_EXTRA,
> 
> Well, WavPack documentation says it's "special extended bitstream for
> floating point data or integers > 24 bit". While old name was not
> entirely correct, new one is even more ambiguous. Please try to give it
> a better name than mere "extra".
 I have no good idea for a better name, I am open to suggestion.
WP_ID_EXTRA_BITS, WP_ID_EXTENDED, WP_ID_EXTENSION, WP_ID_SENT_BITS ...

> 
> >      WP_ID_CHANINFO
> >  };
> >  
> > @@ -85,11 +85,15 @@ typedef struct WavpackContext {
> >      int joint;
> >      uint32_t CRC;
> >      GetBitContext gb;
> > +    int got_extra;
> > +    uint32_t crc_extra;
> 
> Again, maybe got_wvx and crc_wvx instead? 
 Well, wv is useless, we know we are in wavpack module I think.
Anyway, it should probably have the same name than for the WP_ID_?

> > +    GetBitContext gb_extra;
> >      int data_size; // in bits
> >      int samples;
> >      int terms;
> >      Decorr decorr[MAX_TERMS];
> >      int zero, one, zeroes;
> > +    int extra_bits;
> >      int and, or, shift;
> >      int post_shift;
> >      int hybrid, hybrid_bitrate;
> > @@ -344,6 +348,7 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d
> >      int A, B, L, L2, R, R2, bit;
> >      int pos = 0;
> >      uint32_t crc = 0xFFFFFFFF;
> > +    uint32_t crc_extra = 0xFFFFFFFF;
> >      int16_t *dst16 = dst;
> >      int32_t *dst32 = dst;
> >  
> > @@ -424,6 +429,18 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d
> >          if(s->joint)
> >              L += (R -= (L >> 1));
> >          crc = (crc * 3 + L) * 3 + R;
> > +        if(s->extra_bits){
> > +            L <<= s->extra_bits;
> > +            R <<= s->extra_bits;
> > +
> > +            if(s->got_extra){
> > +                L |= get_bits(&s->gb_extra, s->extra_bits);
> > +                crc_extra = crc_extra * 9 + (L&0xffff) * 3 + ((L>>16)&0xffff);
> > +
> > +                R |= get_bits(&s->gb_extra, s->extra_bits);
> > +                crc_extra = crc_extra * 9 + (R&0xffff) * 3 + ((R>>16)&0xffff);
> 
> + ((unsigned)R >> 16), I think (and Michael has pointed that out already).
 Arf, you seem to really want it, I will do it then.

> > +            }
> > +        }
> >          bit = (L & s->and) | s->or;
> >          if(hires)
> >              *dst32++ = (((L + bit) << s->shift) - bit) << s->post_shift;
> > @@ -441,6 +458,10 @@ static inline int wv_unpack_stereo(WavpackContext *s, GetBitContext *gb, void *d
> >          av_log(s->avctx, AV_LOG_ERROR, "CRC error\n");
> >          return -1;
> >      }
> > +    if(s->got_extra && crc_extra != s->crc_extra){
> > +        av_log(s->avctx, AV_LOG_ERROR, "CRC extra error\n");
> > +        return -1;
> > +    }
> 
> I read it as not very good way to say "An additional CRC error" and
> suspect it should mean something else.
 Well your proposed message error is better, I will include it.

> > @@ -533,7 +568,9 @@ static int wavpack_decode_frame(AVCodecContext *avctx,
> >  
> >      memset(s->decorr, 0, MAX_TERMS * sizeof(Decorr));
> >      memset(s->ch, 0, sizeof(s->ch));
> > +    s->extra_bits = 0;
> >      s->and = s->or = s->shift = 0;
> > +    s->got_extra = 0;
> >  
> >      s->samples = AV_RL32(buf); buf += 4;
> >      if(!s->samples){
> > @@ -701,7 +738,7 @@ static int wavpack_decode_frame(AVCodecContext *avctx,
> >                  continue;
> >              }
> >              if(buf[0])
> > -                s->post_shift = buf[0];
> > +                s->extra_bits = buf[0];
> >              else if(buf[1])
> >                  s->shift = buf[1];
> >              else if(buf[2]){
> > @@ -719,6 +756,12 @@ static int wavpack_decode_frame(AVCodecContext *avctx,
> >              buf += size;
> >              got_bs = 1;
> >              break;
> > +        case WP_ID_EXTRA:
> > +            init_get_bits(&s->gb_extra, buf, size * 8);
> > +            s->crc_extra = get_bits_long( &s->gb_extra, 32);
> 
> I'd use s->crc_extra = AV_RL32(buf) and open bit buffer after that but this
> looks fine too. Oh, and please add block size check.
 Using the bitstream reader here seems simpler to me, and additionnaly 
avoid the need of buffer size checks ;)

-- 
fenrir