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

[Kostya]

On Fri, May 01, 2009 at 06:08:37PM +0200, Laurent Aimar wrote:
> 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 ...

I'm no good at names either but WP_ID_EXTRA_BITS seems better than others 

> > >      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_?

yes unless it's too long

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

It's not that hard, n'est-ce pas?
 
> > > +            }
> > > +        }
> > >          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.

"Extra bits CRC error" may be clearer though, pick any ;)

> > > @@ -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 ;)

It's not - if size <=4 (as WavPack source code says), it should be treated as
error, so check is needed anyway (not for bits reading though).
 
> -- 
> fenrir