[FFmpeg-devel] [PATCH] png parser

[Michael Niedermayer]

On Mon, Jul 06, 2009 at 09:17:23AM +0200, Peter Holik wrote:
> Michael Niedermayer schrieb:
> > On Wed, Jul 01, 2009 at 05:04:51PM +0200, Peter Holik wrote:
> >> Michael Niedermayer schrieb:
> >> > On Tue, Jun 30, 2009 at 08:47:48AM +0200, Peter Holik wrote:
> >> >> Michael Niedermayer schrieb:
> >> >> > On Thu, Jun 25, 2009 at 07:55:08PM +0200, Peter Holik wrote:
> >> >> >> Michael Niedermayer schrieb:
> >> >> >> > On Thu, Jun 25, 2009 at 02:14:14PM +0200, Peter Holik wrote:
> >> >> > [...]
> >> >> >
> >> >> >
> >> >> >> +    for (;ppc->pc.frame_start_found && i < buf_size; i++) {
> >> >> >> +        ppc->state = (ppc->state<<8) | buf[i];
> >> >> >
> >> >> > why is this not using the state variable from ParseContext ?
> >> >>
> >> >> ok, now ParseContext state is used
> >> >
> >> > [...]
> >> >
> >> >> +typedef struct PNGParseContext
> >> >> +{
> >> >> +    ParseContext pc;
> >> >> +    uint32_t index;
> >> >
> >> >> +    uint32_t chunk_length;
> >> >
> >> > unneeded, it can be read out of state64, it not needed to be stored in the
> >> > context
> >>
> >> you really mean i should misuse an existing field?
> >>
> >> last time you told me NOT to do such things.
> >
> > no
> >
> > you already read it out of state(64) and thats what state64 is there
> > for, of course you must not store it in state* as a 32bit value rather
> > it naturally is in there because state64 represents the last 8 bytes
> 
> I read out of state64 for the signature but not for png chunks.
> Inside the for loop is not update of state64.
> Well i could misuse state64 for chunk_length but for sourcecode readability
> i prefer to use ppc->chunk_length.

Iam quite unhappy about that as it is worse readabilitywise IMHO and requires
chunk_length to be part of the context vs. a local variable but if its
important to you ...


[...]

> +    for (;ppc->pc.frame_start_found && i < buf_size; i++) {
> +        ppc->pc.state = (ppc->pc.state<<8) | buf[i];

> +        if (ppc->index == 3)
> +            ppc->chunk_length = AV_RL32(&ppc->pc.state) + 4;
> +        else if (ppc->index == 7) {

if(){
}else


> +            if (AV_RB32(&ppc->pc.state) == MKTAG('I', 'E', 'N', 'D')) {
> +                if (ppc->chunk_length >= buf_size - i) {

> +                    ppc->remaining_size = ppc->chunk_length - buf_size + i + 1;

this can still overflow


> +                    ppc->index = -1;
> +                } else
> +                    next = i + ppc->chunk_length + 1;
> +                break;
> +            } else {
> +                ppc->index = 0;
> +                if (ppc->chunk_length >= buf_size - i) {
> +                    ppc->remaining_size = ppc->chunk_length - buf_size + i + 1;
> +                    break;
> +                } else
> +                    i += ppc->chunk_length;
> +                continue;
> +            }

looks like some code can be factored out

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/20090708/61eeffcc/attachment.pgp>