[FFmpeg-soc] Bitstream writer from vorbis_enc.c

[Michael Niedermayer]

On Sun, Aug 24, 2008 at 11:27:50PM +0200, Bartlomiej Wolowiec wrote:
> Sunday 24 August 2008 13:43:20 Michael Niedermayer napisał(a):
> > On Sun, Aug 24, 2008 at 01:34:53PM +0200, Bartlomiej Wolowiec wrote:
> > > Sunday 24 August 2008 05:00:43 Michael Niedermayer napisał(a):
> > > > On Sun, Aug 24, 2008 at 01:56:10AM +0200, Bartlomiej Wolowiec wrote:
> > > > > Saturday 23 August 2008 03:33:27 Michael Niedermayer napisał(a):
> > > > > > On Sat, Aug 23, 2008 at 12:31:48AM +0200, Bartlomiej Wolowiec wrote:
> > > > > > > In nellymoserenc.c is a duplicate of bitstream writer code from
> > > > > > > vorbis_enc.c. How should I remove this duplicate? Add to
> > > > > > > bitstream.h this code and make activation by BITSTREAM_WRITER_LE
> > > > > > > available?
> > > > > >
> > > > > > The put_bits code in vorbis_enc.c is extreemly poorly written and
> > > > > > it needs be cleaned up before it can be used by another encoder.
> > > > > > Actually i wonder how this code managed to get into svn ...
> > > > > >
> > > > > > [...]
> > > > >
> > > > > I tried to wrote patch to bitstream.h. I hope that it's better than
> > > > > previous code.
> > > > >
> > > > > --
> > > > > Bartlomiej Wolowiec
> > > > >
> > > > > Index: bitstream.h
> > > > > ===================================================================
> > > > > --- bitstream.h	(wersja 14909)
> > > > > +++ bitstream.h	(kopia robocza)
> > > > > @@ -123,6 +123,7 @@
> > > > >  #ifdef ALT_BITSTREAM_WRITER
> > > > >      align_put_bits(s);
> > > > >  #else
> > > > > +#ifndef BITSTREAM_WRITER_LE
> > > > >      s->bit_buf<<= s->bit_left;
> > > > >      while (s->bit_left < 32) {
> > > > >          /* XXX: should test end of buffer */
> > > > > @@ -130,6 +131,14 @@
> > > > >          s->bit_buf<<=8;
> > > > >          s->bit_left+=8;
> > > > >      }
> > > > > +#else
> > > > > +    while (s->bit_left < 32) {
> > > > > +        /* XXX: should test end of buffer */
> > > > > +        *s->buf_ptr++=s->bit_buf;
> > > > > +        s->bit_buf>>=8;
> > > > > +        s->bit_left+=8;
> > > > > +    }
> > > > > +#endif
> > > >
> > > > theres some common code between if and else
> > > >
> > > > >      s->bit_left=32;
> > > > >      s->bit_buf=0;
> > > > >  #endif
> > > > > @@ -190,6 +199,7 @@
> > > > >
> > > > >      //    printf("n=%d value=%x cnt=%d buf=%x\n", n, value, bit_cnt,
> > > > > bit_buf); /* XXX: optimize */
> > > > > +#ifndef BITSTREAM_WRITER_LE
> > > > >      if (n < bit_left) {
> > > > >          bit_buf = (bit_buf<<n) | value;
> > > > >          bit_left-=n;
> > > > > @@ -210,7 +220,27 @@
> > > > >          bit_left+=32 - n;
> > > > >          bit_buf = value;
> > > > >      }
> > > > > +#else // BITSTREAM_WRITER_LE
> > > > > +    bit_buf |= value << (32 - bit_left);
> > > > > +    if (n >= bit_left) {
> > > > > +#ifdef UNALIGNED_STORES_ARE_BAD
> > > > > +        if (3 & (intptr_t) s->buf_ptr) {
> > > > > +            s->buf_ptr[0] = bit_buf;
> > > > > +            s->buf_ptr[1] = bit_buf >> 8;
> > > > > +            s->buf_ptr[2] = bit_buf >> 16;
> > > > > +            s->buf_ptr[3] = bit_buf >> 24;
> > > > > +        } else
> > > > > +#endif
> > > > > +        *(uint32_t *)s->buf_ptr = le2me_32(bit_buf);
> > > > > +        s->buf_ptr+=4;
> > > > >
> > > > >
> > > > > +        //FIXME: value>>32 == value... ?!
> > > >
> > > > value>>32 == undefined
> > > >
> > > > [...]
> > >
> > > Ok. Here is improved patch.
> >
> > [...]
> >
> > > @@ -210,6 +218,24 @@
> > >          bit_left+=32 - n;
> > >          bit_buf = value;
> > >      }
> > > +#else // BITSTREAM_WRITER_LE
> > > +    bit_buf |= value << (32 - bit_left);
> > > +    if (n >= bit_left) {
> > > +#ifdef UNALIGNED_STORES_ARE_BAD
> > > +        if (3 & (intptr_t) s->buf_ptr) {
> > > +            s->buf_ptr[0] = bit_buf      ;
> > > +            s->buf_ptr[1] = bit_buf >>  8;
> > > +            s->buf_ptr[2] = bit_buf >> 16;
> > > +            s->buf_ptr[3] = bit_buf >> 24;
> > > +        } else
> > > +#endif
> > > +        *(uint32_t *)s->buf_ptr = le2me_32(bit_buf);
> > > +        s->buf_ptr+=4;
> > > +        bit_buf = (bit_left==32)?0:value >> bit_left;
> > > +        bit_left+=32;
> > > +    }
> > > +    bit_left-=n;
> > > +#endif // BITSTREAM_WRITER_LE
> >
> > I think #ifdef + #else is clearer than #ifndef + #else
> > also if you do add comments to #else and #endif, make sure they are
> > correct, the ones above are not. The comment needed for a endif
> > after #ifndef is different than #ifdef also it makes a difference
> > if a #else was in between.
> >
> >
> > [...]
> 
> Ok, I hope that it's good now...

ok

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20080825/f542ffa0/attachment.pgp>