[FFmpeg-devel] [PATCH] png parser

[Michael Niedermayer]

paOn Thu, Jun 18, 2009 at 12:47:49PM +0200, Peter Holik wrote:
> Michael Niedermayer schrieb:
> > On Thu, Jun 18, 2009 at 09:34:30AM +0200, Peter Holik wrote:
> >> Michael Niedermayer schrieb:
> >> > On Sat, Jun 13, 2009 at 01:54:46PM +0200, Peter Holik wrote:
> >> > [...]
> >> >> +static int png_parse(AVCodecParserContext *s, AVCodecContext *avctx,
> >> >> +                     const uint8_t **poutbuf, int *poutbuf_size,
> >> >> +                     const uint8_t *buf, int buf_size)
> >> >> +{
> >> >> +    ParseContext *pc = s->priv_data;
> >> >> +    uint32_t *state_ptr = &pc->state;
> >> >
> >> >> +    uint32_t *chunk_length_ptr = (uint32_t *)&pc->state64;
> >> >
> >> > this looks like you misuse the field
> >>
> >> yes i use unused fields, and to be able to read the code i do not use the
> >
> > you use fields that are intended for something quite different than what
> > you use them for, and your parser isnt even the only code writing to them ...
> 
> No, in this case my parser is the only one.

ff_combine_frame() does

    /* store overread bytes */
    for(;next < 0; next++){
        pc->state = (pc->state<<8) | pc->buffer[pc->last_index + next];
        pc->state64 = (pc->state64<<8) | pc->buffer[pc->last_index + next];
        pc->overread++;
    }



> 
> >> unused variables directly - i use this construct to have a better understanding
> >> and not to have waste memory.
> >>
> >>
> >> >> +    uint32_t *remaining_size_ptr = &pc->overread_index;
> >> >
> >> > thats a very obfuscated way o access a field from the struct
> >>
> >> i think it is better to do it this way than have to do something like
> >
> > iam not even certain that you need to access overread_index at all
> 
> overread_index is only used in ff_combine_frame if next is negative, that
> is in my parser never the case.

sounds like a bug in your parser

[...]
> >> >> +                    next = buf_size;
> >> >> +                    goto fail;
> >> >> +                }
> >> >> +                return i;
> >> >> +            }
> >> >> +        }
> >> >> +    } else
> >> >> +        if (*remaining_size_ptr) {
> >> >> +            i = FFMIN(*remaining_size_ptr, buf_size);
> >> >> +            *remaining_size_ptr -= i;
> >> >> +            if (*remaining_size_ptr)
> >> >> +                goto flush;
> >> >> +            if (pc->frame_start_found == -1) {
> >> >> +                next = i;
> >> >> +                goto flush;
> >> >> +            }
> >> >> +        }
> >> >> +
> >> >> +    for (;pc->frame_start_found && i < buf_size; i++) {
> >> >> +        *state_ptr = (*state_ptr << 8) | buf[i];
> >> >> +        if (pc->frame_start_found == 4) {
> >> >> +                *chunk_length_ptr = AV_RL32(state_ptr) + 4;
> >> >> +                if (*chunk_length_ptr > 0x7fffffff) {
> >> >> +                    next = buf_size;
> >> >> +                    goto fail;
> >> >> +                }
> >> >
> >> > i think this discards data?
> >> > if so, thats wrong, parsers should not discard any data, just split it the
> >> > best they can.
> >> > a decoder might very well be able to decode the correct parts of a frame ...
> >>
> >> if taken this check out of the decoder, whats wrong here?
> >
> > parsers should not discard data.
> 
> why using a parser if a decoder (should) filter out garbage?

because its annoying to write decoders that can handle 1 byte input at a time
(there are more reasons ...)

its the job of a parser to take random chunks that can span any number of
frames or by any part of a  frame down to a single byte and convert that to
1 frame per buffer for easy consumtion and easy timestamp management

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/20090618/c3e78f89/attachment.pgp>