[FFmpeg-devel] [PATCH] png parser

[Peter Holik]

Michael Niedermayer schrieb:
> On Thu, Jun 18, 2009 at 01:16:52PM +0200, Peter Holik wrote:
>> 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!
>
> did you ever maintain code with such hidden assumtions?
> a little change in parser.c or returning negative next and you have some hours
> of funny debugging ahead ...
>
>
> [...]
>> >> >> >> +                    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.
>
> if i change 1 byte of the signature at the start that doesnt turn a frame
> into garbage.
> a decoder might very well be able to still decode it
> same for a missing end chunk


Now a new version of my patch without re-used variables and special workaround.

I've tested successfully with two png Files (2kBytes / 8kBytes) -
because the parser gets 4kByte Chunks:

for i in $(seq 0 9999); do \
  echo -n "$i " && \
  dd if=/dev/urandom of=/tmp/ffm.tmp bs=1 count=$i 2>/dev/null && \
  cat /tmp/ffm.tmp /tmp/a.png /tmp/ffm.tmp /tmp/a.png | \
  ffmpeg -f image2pipe -vcodec png -i - /tmp/c%d.png 2>&1 | \
  grep "frame=" | grep -v "frame=    2"
done

cu Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: png_parser.diff
Type: text/x-diff
Size: 5805 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090623/9cc75d60/attachment.diff>