[FFmpeg-devel] [PATCH] png parser

[Peter Holik]

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!

please apply

cu Peter