[FFmpeg-devel] [PATCH] png parser

[Peter Holik]

Michael Niedermayer schrieb:
> paOn Thu, Jun 18, 2009 at 12:47:49PM +0200, Peter Holik wrote:
>> Michael Niedermayer schrieb:
>> > On Thu, Jun 18, 2009 at 09:34:30AM +0200, Peter Holik wrote:
>> >> Michael Niedermayer schrieb:
>> >> > On Sat, Jun 13, 2009 at 01:54:46PM +0200, Peter Holik wrote:
>> >> > [...]
>> >> >> +static int png_parse(AVCodecParserContext *s, AVCodecContext *avctx,
>> >> >> +                     const uint8_t **poutbuf, int *poutbuf_size,
>> >> >> +                     const uint8_t *buf, int buf_size)
>> >> >> +{
>> >> >> +    ParseContext *pc = s->priv_data;
>> >> >> +    uint32_t *state_ptr = &pc->state;
>> >> >
>> >> >> +    uint32_t *chunk_length_ptr = (uint32_t *)&pc->state64;
>> >> >
>> >> > this looks like you misuse the field
>> >>
>> >> yes i use unused fields, and to be able to read the code i do not use the
>> >
>> > you use fields that are intended for something quite different than what
>> > you use them for, and your parser isnt even the only code writing to them ...
>>
>> No, in this case my parser is the only one.
>
> ff_combine_frame() does
>
>     /* 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];
>         pc->overread++;
>     }

but that part is never reached for my parser!

>> >> unused variables directly - i use this construct to have a better understanding
>> >> and not to have waste memory.
>> >>
>> >>
>> >> >> +    uint32_t *remaining_size_ptr = &pc->overread_index;
>> >> >
>> >> > thats a very obfuscated way o access a field from the struct
>> >>
>> >> i think it is better to do it this way than have to do something like
>> >
>> > iam not even certain that you need to access overread_index at all
>>
>> overread_index is only used in ff_combine_frame if next is negative, that
>> is in my parser never the case.
>
> sounds like a bug in your parser

This is NOT the case, but i see you do not like my approach of reusing unused variabled


>> >> >> +                    next = buf_size;
>> >> >> +                    goto fail;
>> >> >> +                }
>> >> >> +                return i;
>> >> >> +            }
>> >> >> +        }
>> >> >> +    } else
>> >> >> +        if (*remaining_size_ptr) {
>> >> >> +            i = FFMIN(*remaining_size_ptr, buf_size);
>> >> >> +            *remaining_size_ptr -= i;
>> >> >> +            if (*remaining_size_ptr)
>> >> >> +                goto flush;
>> >> >> +            if (pc->frame_start_found == -1) {
>> >> >> +                next = i;
>> >> >> +                goto flush;
>> >> >> +            }
>> >> >> +        }
>> >> >> +
>> >> >> +    for (;pc->frame_start_found && i < buf_size; i++) {
>> >> >> +        *state_ptr = (*state_ptr << 8) | buf[i];
>> >> >> +        if (pc->frame_start_found == 4) {
>> >> >> +                *chunk_length_ptr = AV_RL32(state_ptr) + 4;
>> >> >> +                if (*chunk_length_ptr > 0x7fffffff) {
>> >> >> +                    next = buf_size;
>> >> >> +                    goto fail;
>> >> >> +                }
>> >> >
>> >> > i think this discards data?
>> >> > if so, thats wrong, parsers should not discard any data, just split it the
>> >> > best they can.
>> >> > a decoder might very well be able to decode the correct parts of a frame ...
>> >>
>> >> if taken this check out of the decoder, whats wrong here?
>> >
>> > parsers should not discard data.
>>
>> why using a parser if a decoder (should) filter out garbage?
>
> because its annoying to write decoders that can handle 1 byte input at a time
> (there are more reasons ...)
>
> its the job of a parser to take random chunks that can span any numbe
r of
> frames or by any part of a  frame down to a single byte and convert that to
> 1 frame per buffer for easy consumtion and easy timestamp management


1 frame means a complete picture. ok then a parser is dropping garbage.

cu Peter