[FFmpeg-devel] [PATCH] png parser

[Michael Niedermayer]

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


[...]

> PS: Have you found a new ISP in Vienna?

as UPC is working again at least ATM and iam way too busy with too many
silly and unimportant things its still on my TODO, actually i halfway
forgot about it ...


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- 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/20090722/9259889f/attachment.pgp>