[FFmpeg-devel] [PATCH] png parser

[Michael Niedermayer]

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


> 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


[...]
> >> +    int next = END_NOT_FOUND;
> >> +    int i = 0;
> >> +
> >> +    *poutbuf_size = 0;
> >> +    if (buf_size == 0)
> >> +        return 0;
> >> +
> >> +    if (!pc->frame_start_found) {
> >> +        uint64_t *state64_ptr = &pc->state64;
> >> +
> >> +        for (; i < buf_size; i++) {
> >> +            *state64_ptr = (*state64_ptr << 8) | buf[i];
> >> +            if (*state64_ptr == PNGSIG || *state64_ptr == MNGSIG) {
> >> +                pc->frame_start_found = 1;
> >> +                if (++i == 8)
> >> +                    break;
> >
> >> +                /* workaround to start packet with a signature */
> >> +                buf_size = 8;
> >> +                *state64_ptr = be2me_64(*state64_ptr);
> >> +                if (ff_combine_frame(pc, next, (const uint8_t **)&state64_ptr, &buf_size) !=
> >> -1) {
> >
> > whatever this is supposed to do, it looks very hackish
> 
> my idea of a parser is to take good content out of a garbaged stream.

what is good depends on what the decoder can still use, and the output from
the parser may be stored in a file and decoded 50 years in the future with a
more advanced decoder.


> 
> well in this case if there were some bytes of the header in the last packet
> i form a packet starting with a signature.
> 
> because most of ffmpeg decoders do not search for a begin of a signature, they drop.

Improvments to decoders are welcome


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

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

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- 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/ed82f300/attachment.pgp>