[FFmpeg-devel] [PATCH] png parser

[Michael Niedermayer]

On Thu, Jun 18, 2009 at 01:16:52PM +0200, Peter Holik wrote:
> Michael Niedermayer schrieb:
> > 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++;
> >     }
> 
> but that part is never reached for my parser!

did you ever maintain code with such hidden assumtions?
a little change in parser.c or returning negative next and you have some hours
of funny debugging ahead ...


[...]
> >> >> >> +                    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 numbe
> r 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
> 
> 
> 1 frame means a complete picture. ok then a parser is dropping garbage.

if i change 1 byte of the signature at the start that doesnt turn a frame
into garbage.
a decoder might very well be able to still decode it
same for a missing end chunk

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

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- 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/bd55f964/attachment.pgp>