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

[Michael Niedermayer]

Hi

On Fri, May 04, 2007 at 05:32:10PM +0200, Ivo wrote:
> Hi,
> 
> The following patch implements a demuxer and decoder for .txd files, used by 
> various games that use the Renderware engine, like Grand Theft Auto. See 
> http://wiki.multimedia.cx/index.php?title=TXD for details.
> 
> I decided to place the S3TC decoding functions in a separate file as they 
> are pretty generic and used in other (Microsoft) formats too, like 
> DirectShow Surfaces. This way they can easily be re-used in the future.
> 
> Currently, the code demuxes and decodes all GTA:SA files I have tested 
> correctly, except for one problem with mid-stream dimension changes which I 
> described here:
> 
> Message-Id: <200705040110.13009.ivop at euronet.nl>
> Subject: [FFmpeg-devel] Multiple frame dimensions in a single stream
> 
> I have traced the problem back to ffmpeg.c which sets the output 
> AVCodecContext parameters once at the beginning, based on the input 
> context, which is based on the first frame in the TXD case. It does not 
> handle changes of the input context. Fixing that is non-trivial because of 
> potential padding/cropping/scaling of the frames and is also outside the 
> scope of this patch. But I will look into it as it affects other codecs 
> that support multiple dimensions in one stream as well.
> 
> --Ivo

[...]
> +#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


> +        colors[3] = PACK_COLOR((2*r1+r0)/3, (2*g1+g0)/3, (2*b1+b0)/3, a);
> +    } else {
> +        colors[2] = PACK_COLOR((r0+r1)/2, (g0+g1)/2, (b0+b1)/2, a);
> +        colors[3] = 0;
> +    }
> +

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


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

why isnt d uint32_t?


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


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


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



[...]
> +                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()



> +                pkt->stream_index = 0;
> +                if (ret <= 0) {
> +                    av_free_packet(pkt);

this is uneeded av_get_packet() will free it in case of failure


> +                    return AVERROR_IO;
> +                }

> +                pkt->size = ret;

this also isnt needed

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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070504/c8a8fba5/attachment.pgp>