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

[Michael Niedermayer]

Hi

On Sat, May 05, 2007 at 01:57:56PM +0200, Ivo wrote:
[...]
> +    if (c0 > c1 || flag) {
> +        rb2 = (((2*rb0+rb1) * 42) >> 7) & 0xff00ff;
> +        rb3 = (((2*rb1+rb0) * 42) >> 7) & 0xff00ff;
> +        g2  = (((2*g0 +g1 ) * 42) >> 7) & 0x00ff00;
> +        g3  = (((2*g1 +g0 ) * 42) >> 7) & 0x00ff00;

*42 >> 7 == *21 >> 6


[...]

> +void ff_decode_dxt1(const uint8_t *src, uint8_t *dst,
> +                    const unsigned int w, const unsigned int h,
> +                    const unsigned int stride) {
> +    unsigned int bx, by, qstride = stride/4;
> +    uint8_t *s = (uint8_t *) src;

casting/changing const to non const is bad, there is no need to change
src so it should stay const, casting it to non const makes the code look
like it will change src ...


[...]

> +    *picture = *(AVFrame *)&s->picture;

unneeded cast


[...]
> +static inline void push_state_stack(TxdState *state, unsigned int id,
> +                                    unsigned int chunk_size) {
> +    txd_chunk_t *next        = state->stack;
> +    state->stack             = av_mallocz(sizeof(txd_chunk_t));
> +    state->stack->id         = id;
> +    state->stack->chunk_left = chunk_size;
> +    state->stack->next       = next;
> +}
> +
> +static int txd_read_packet(AVFormatContext *s, AVPacket *pkt) {
> +    TxdState * const state = s->priv_data;
> +    txd_chunk_t *next;
> +    unsigned int id, chunk_size, marker;
> +    int ret;
> +
> +    for(;;) {
> +        if (state->end_of_txd_file)
> +            return AVERROR_IO;
> +        if (state->stack) {
> +            if (state->stack->chunk_left < 0) {
> +                av_log(NULL, AV_LOG_ERROR, "data is damaged\n");
> +                return -1;
> +            } else if (state->stack->chunk_left == 0) {
> +                next = state->stack->next;
> +                av_free(state->stack);
> +                state->stack = next;
> +                if (!state->stack) {
> +                    state->end_of_txd_file = 1;
> +                    continue;

the s/continue/return AVERROR_IO/ is IMHO more clear ...


> +                }
> +            }
> +        }
> +
> +        id         = get_le32(&s->pb);
> +        chunk_size = get_le32(&s->pb);
> +        marker     = get_le32(&s->pb);
> +
> +        if (marker != TXD_MARKER) {
> +            av_log(NULL, AV_LOG_ERROR, "marker does not match\n");
> +            return AVERROR_IO;
> +        }
> +
> +        if (state->stack)
> +            state->stack->chunk_left -= TXD_HEADER_SIZE;
> +
> +        if (state->stack == NULL) {
> +            if (id == TXD_FILE) {
> +                push_state_stack(state, id, chunk_size);
> +            } else goto unknown_chunk;
> +        } else if (state->stack->id == TXD_FILE) {
> +            state->stack->chunk_left -= chunk_size;
> +            if (id == TXD_INFO || id == TXD_EXTRA) {
> +                url_fskip(&s->pb, chunk_size);
> +            } else if (id == TXD_TEXTURE) {
> +                push_state_stack(state, id, chunk_size);
> +            } else goto unknown_chunk;
> +        } else if (state->stack->id == TXD_TEXTURE) {
> +            state->stack->chunk_left -= chunk_size;
> +            if (id == TXD_TEXTURE_DATA) {
> +                ret = av_get_packet(&s->pb, pkt, chunk_size);
> +                pkt->stream_index = 0;
> +                if (ret <= 0)
> +                    return AVERROR_IO;
> +                return ret;
> +            } else if (id == TXD_EXTRA) {
> +                url_fskip(&s->pb, chunk_size);
> +            } else goto unknown_chunk;
> +        }

whats the stack good for?
i think TXD_FILE / TXD_INFO should be read in the header reading code
like read and ignore TXD_FILE
skip all TXD_INFO

and then the packet read code could do
read next chunk

if(TXD_TEXTURE)
    ignore
if(TXD_TEXTURE_DATA)
    av_get_packet() ...
else
    skip 

or does that fail with any file? if so why?
your stack code might appear more robust at first (it did to me) but if the
sizes of TXD_TEXTURE and the contained packets missmatch then it will also
fail like my suggestion above and if they dont missmatch then i dont see
where the stack would do any good ...


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

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- 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/20070505/127382d9/attachment.pgp>