[FFmpeg-devel] [PATCH] png parser

[Michael Niedermayer]

On Sat, Jul 25, 2009 at 03:08:33PM +0200, Peter Holik wrote:
> Michael Niedermayer schrieb:
> > On Wed, Jul 22, 2009 at 08:32:09AM +0200, Peter Holik wrote:
> >> Michael Niedermayer schrieb:
> >> > On Sat, Jul 18, 2009 at 09:00:08AM +0200, Peter Holik wrote:
> >> >> Michael Niedermayer schrieb:
> >> >> > On Fri, Jul 17, 2009 at 09:40:13AM +0200, Peter Holik wrote:
> >> >> >> Michael Niedermayer schrieb:
> >> >> >> > On Mon, Jul 13, 2009 at 05:01:00PM +0200, Peter Holik wrote:
> >> >> >> >> Michael Niedermayer schrieb:
> >> >> >> >> > 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 ...
> >> >> >> >>
> >> >> >> >> chunk_length has to be part of the context because if buffer ends with
> >> >> >> >> the chunk length it can not be a local var.
> >> >> >> >
> >> >> >> > its in state64
> >> >> >>
> >> >> >> no, please have a look at ff_combine_frame.
> >> >> >> the only position where state64 is updated is if next is negative.
> >> >> >
> >> >> > grep state parser.c will show state64 is updated at every point that state is.
> >> >>
> >> >> #> grep state64 parser.c
> >> >> pc->state64 = (pc->state64<<8) | pc->buffer[pc->last_index + next];
> >> >>
> >> >> only one time, where state64 is updated
> >> >
> >> > yes, state is also only updated one time, and exactly in the line before
> >> > state64
> >> > so if you can use state you also should be able to use state64
> >>
> >> but this update is never available to me because i NEVER have a negative next for
> >> ff_combine_frame!
> >
> > its the same with state
> 
> yes!
> 
> So what is now wrong with my patch?

i think it can be simplified by using state64, if you convince me that this
is not the case id drop the issue.
(to convince me, just show me the code that uses state64, half of it is
 the chunk type the other the chunk_length, which is then loaded into a
 local var)

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

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- 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/20090725/acd17872/attachment.pgp>