[FFmpeg-devel] [PATCH] png parser

[Peter Holik]

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.

>> 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.

> [...]
>> >> +    int next = END_NOT_FOUND;
>> >> +    int i = 0;
>> >> +
>> >> +    *poutbuf_size = 0;
>> >> +    if (buf_size == 0)
>> >> +        return 0;
>> >> +
>> >> +    if (!pc->frame_start_found) {
>> >> +        uint64_t *state64_ptr = &pc->state64;
>> >> +
>> >> +        for (; i < buf_size; i++) {
>> >> +            *state64_ptr = (*state64_ptr << 8) | buf[i];
>> >> +            if (*state64_ptr == PNGSIG || *state64_ptr == MNGSIG) {
>> >> +                pc->frame_start_found = 1;
>> >> +                if (++i == 8)
>> >> +                    break;
>> >
>> >> +                /* workaround to start packet with a signature */
>> >> +                buf_size = 8;
>> >> +                *state64_ptr = be2me_64(*state64_ptr);
>> >> +                if (ff_combine_frame(pc, next, (const uint8_t **)&state64_ptr, &buf_size) !=
>> >> -1) {
>> >
>> > whatever this is supposed to do, it looks very hackish
>>
>> my idea of a parser is to take good content out of a garbaged stream.
>
> what is good depends on what the decoder can still use, and the output from
> the parser may be stored in a file and decoded 50 years in the future with a
> more advanced decoder.
>
>
>>
>> well in this case if there were some bytes of the header in the last packet
>> i form a packet starting with a signature.
>>
>> because most of ffmpeg decoders do not search for a begin of a signature, they drop.
>
> Improvments to decoders are welcome
>
>
>>
>> >> +                    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?

cu Peter