[FFmpeg-devel] [PATCH] png parser
Peter Holik
peter
Thu Jun 18 09:34:30 CEST 2009
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
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
---
uint32_t index = pc->overread_index;
use index ....
pc->overread_index = index;
---
in my version i do
---
uint32_t *index = &pc->overread_index;
use *index
---
but if you won't like coding like this than i make an extra struct.
>> + 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.
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.
>> + 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?
cu Peter
More information about the ffmpeg-devel
mailing list