[FFmpeg-devel] [PATCH] TXD demuxer and decoder

[Ivo]

Hi,

On Friday 04 May 2007 21:08, Michael Niedermayer wrote:
> On Fri, May 04, 2007 at 05:32:10PM +0200, Ivo wrote:
> > [Renderware TXD demuxer/decoder]
>
> > +#define UNPACK_R(c, r)      r = (c&0x1f)<<3; r += r>>5;
> > +#define UNPACK_G(c, g)      g = (c>>3)&0xfc; g += g>>6;
> > +#define UNPACK_B(c, b)      b = (c>>8)&0xf8; b += b>>5;
> > +#define UNPACK_RGB(c,r,g,b) UNPACK_R(c,r); UNPACK_G(c,g);
> > UNPACK_B(c,b); +#define PACK_COLOR(r,g,b,a) (r+(g<<8)+(b<<16)+(a<<24))
> > +
> > +static inline void dxt1_decode_pixels(uint8_t *s, uint8_t *d,
> > +                                      unsigned int stride, unsigned
> > int flag, +                                      uint64_t alpha) {
> > +    unsigned int x, y, c0, c1, r0, g0, b0, r1, g1, b1, a = !flag *
> > 255; +    uint32_t colors[4], pixels;
> > +
> > +    c0 = le2me_16(*(uint16_t *)(s));
> > +    c1 = le2me_16(*(uint16_t *)(s+2));
> > +    UNPACK_RGB(c0, r0, g0, b0);
> > +    UNPACK_RGB(c1, r1, g1, b1);
> > +
> > +    colors[0] = PACK_COLOR(r0, g0, b0, a);
> > +    colors[1] = PACK_COLOR(r1, g1, b1, a);
> > +    if (c0 > c1 || flag) {
> > +        colors[2] = PACK_COLOR((2*r0+r1)/3, (2*g0+g1)/3, (2*b0+b1)/3,
> > a);
>
> r0+=r0>>5; r1+=r1>>5; 2*r0+r1
> == (except rounding)
> T=2*r0+r1; T+= T>>5;
>
> furthermore the T+=T>>5 and the /3 == * 11 >>5

Total number of instructions for the (c0>c1) case dropped by 21% and no more 
division by 3. For the (c0<=c1) case it dropped by 14%.

> > +    for (pixels=le2me_32(*(uint32_t *)(s+4)), y=0; y<4; d+=stride,
> > y++) +        for (x=0; x<4; x++, pixels>>=2, alpha>>=4) {
>
> may i suggest to put fewer statements on each line of code

Ok :) Fixed.

> > +            a  = (alpha & 0x0f) << 28;
> > +            a += a >> 4;
> > +            ((uint32_t *)d)[x] = a + colors[pixels&3];
>
> why isnt d uint32_t?

Fixed, up to the entry points of the decoding functions to avoid the need 
for callers to cast their pointers (which are uint8_t* most of the time if 
not always).

> > +void ff_decode_dxt1(const uint8_t *src, const uint8_t *dst,
> > +                    const unsigned int w, const unsigned int h,
> > +                    const unsigned int stride) {
> > +    unsigned int bx, by, stride3 = 4*stride - 4*w;
> > +    uint8_t *s = (uint8_t *) src, *d = (uint8_t *) dst;
>
> this is wrong, dst isnt const, src should stay const

Fixed.

> > +static int txd_init(AVCodecContext *avctx) {
> > +    TXDContext *s = avctx->priv_data;
> > +
> > +    avcodec_get_frame_defaults((AVFrame*)&s->picture);
> > +    avctx->coded_frame= (AVFrame*)&s->picture;
>
> useles cast

Fixed. Also in two other places.

> > +static int txd_probe(AVProbeData * pd) {
> > +    uint8_t *buf = pd->buf;
> > +    unsigned int id, marker;
> > +
> > +    id     = AV_RL32(buf);
> > +    marker = AV_RL32(buf+8);
> > +
> > +    if (id == 0x16 && marker == TXD_MARKER)
> > +        return AVPROBE_SCORE_MAX;
>
> the id and marker variables are redundant

Fixed.

> > +                if (av_new_packet(pkt, chunk_size) < 0)
> > +                    return AVERROR_IO;
> > +                ret = av_get_packet(&s->pb, pkt, chunk_size);
>
> memleak, av_get_packet() calls av_new_packet()

Ah, yes. I forgot to remove those two lines. They were left-overs from 
previous code. Fixed.

> > +                pkt->stream_index = 0;
> > +                if (ret <= 0) {
> > +                    av_free_packet(pkt);
>
> this is uneeded av_get_packet() will free it in case of failure

Fixed.

> > +                pkt->size = ret;
>
> this also isnt needed

Fixed.

I also removed some code duplication from the demuxer (pushing chunks on the 
stack for sub-chunks).

--Ivo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: txd.patch
Type: text/x-diff
Size: 17830 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070505/8311cc4e/attachment.patch>