[FFmpeg-devel] [PATCH] png parser

[Peter Holik]

Peter Holik schrieb:
> 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
>
> #> grep -B30 state64 parser.c
>     /* copy into buffer end return */
>     if(next == END_NOT_FOUND){
>         void* new_buffer = av_fast_realloc(pc->buffer, &pc->buffer_size, (*buf_size) + pc->index +
> FF_INPUT_BUFFER_PADDING_SIZE);
>
>         if(!new_buffer)
>             return AVERROR(ENOMEM);
>         pc->buffer = new_buffer;
>         memcpy(&pc->buffer[pc->index], *buf, *buf_size);
>         pc->index += *buf_size;
>         return -1;
>     }
>
>     *buf_size=
>     pc->overread_index= pc->index + next;
>
>     /* append to buffer */
>     if(pc->index){
>         void* new_buffer = av_fast_realloc(pc->buffer, &pc->buffer_size, next + pc->index +
> FF_INPUT_BUFFER_PADDING_SIZE);
>
>         if(!new_buffer)
>             return AVERROR(ENOMEM);
>         pc->buffer = new_buffer;
>         memcpy(&pc->buffer[pc->index], *buf, next + FF_INPUT_BUFFER_PADDING_SIZE );
>         pc->index = 0;
>         *buf= pc->buffer;
>     }
>
>     /* 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];
>
> this is in parser.c/ff_combine_frame
>
> if next == END_NOT_FOUND there is no update of state64
>
> as you can see state64 is only updated id next < 0 (/* store overread bytes */)
>
>
> please apply my patch
>
> cu Peter
>
> PS: Have you found a new ISP in Vienna?

ping