[FFmpeg-devel] [PATCH] Electronic Arts TGQ decoder

[Michael Niedermayer]

On Tue, Oct 07, 2008 at 09:53:49PM +1100, Peter Ross wrote:
> On Sat, Oct 04, 2008 at 03:19:50AM +0200, Michael Niedermayer wrote:
> > On Fri, Oct 03, 2008 at 09:43:51PM +1000, Peter Ross wrote:
> > > On Sat, Sep 27, 2008 at 08:07:05PM +1000, Peter Ross wrote:
> > > > On Sat, Sep 27, 2008 at 10:12:34AM +1000, Peter Ross wrote:
> > > > > Patches enclosed.
> > > > > 
> > > > > Info: http://wiki.multimedia.cx/index.php?title=Electronic_Arts_TGQ
> > > > > Samples: http://samples.mplayerhq.hu/game-formats/ea-tgq-uv/
> > > > 
> > > > Thanks for the prompt feedback. Round two enclosed.
> > > 
> > > Round three. (See earlier posts for inv_aanscales.diff and eatgq-demux-r1.diff.)
> 
> Comments below. Round four enclosed.
> 

> > > --- libavcodec/dsputil.c	(revision 15532)
> > > +++ libavcodec/dsputil.c	(working copy)
> > > @@ -4137,6 +4137,64 @@
> > >      dest[0] = cm[dest[0] + ((block[0] + 4)>>3)];
> > >  }
> > >  
> > > +/* Electronic Arts TGQ/TQI/MAD IDCT algorithm */
> > > +#define A4 1.3065630f
> > > +#define A2 0.5411961f
> > > +#define A5 0.3826834f
> > > +#define IDCT_TRANSFORM(dest,s0,s1,s2,s3,s4,s5,s6,s7,d0,d1,d2,d3,d4,d5,d6,d7,munge,src) {\
> > 
> > I think this should be in a seperate file, dsputil is already rather bloated
> 
> Agree! (and at 4000+ lines, it now takes my personal super-computer a whole
> second to compile dsputil.o).

yes but the way you did it makes dsputil.c now depend on eatgq.c.
While i suspect users short on space (embeded hw) will like to
disable all but the most common things.

I think the idct should be in a seperate file ...
ideally would be if each idct we have could be disabled at configure
time like codecs ...

[...]

> +/* Electronic Arts TGQ/TQI/MAD IDCT algorithm */
> +#define A4 1.3065630f
> +#define A2 0.5411961f
> +#define A5 0.3826834f

maybe these should get the following comments:
cos(pi*1/16)sqrt(2)
cos(pi*2/16)sqrt(2)
cos(pi*6/16)sqrt(2)


> +#define IDCT_TRANSFORM(dest,s0,s1,s2,s3,s4,s5,s6,s7,d0,d1,d2,d3,d4,d5,d6,d7,munge,src) {\
> +    const int a1 = (src)[s1] + (src)[s7]; \
> +    const int a7 = (src)[s1] - (src)[s7]; \
> +    const int a5 = (src)[s5] + (src)[s3]; \
> +    const int a3 = (src)[s5] - (src)[s3]; \
> +    const int a2=  (src)[s2] + (src)[s6]; \

> +    const int a6= ((src)[s2] - (src)[s6])*M_SQRT1_2; \

why not *181 >> 8 ?
it seems using a float is a overkill here
similarely the others could be
*669 >> 9
*277 >> 9
*196 >> 9
?

Or do you want to maintain some bitwise equality to the binary decoder?

The rest of the patch looks ok

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I count him braver who overcomes his desires than him who conquers his
enemies for the hardest victory is over self. -- Aristotle
-------------- 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-devel/attachments/20081008/d58d103a/attachment.pgp>