[FFmpeg-devel] [PATCH] png parser

[Peter Holik]

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.

>>
>>
>> >> +    uint32_t remaining_size;
>> >> +} PNGParseContext;
>> >> +
>> >> +static int png_parse(AVCodecParserContext *s, AVCodecContext *avctx,
>> >> +                     const uint8_t **poutbuf, int *poutbuf_size,
>> >> +                     const uint8_t *buf, int buf_size)
>> >> +{
>> >> +    PNGParseContext *ppc = s->priv_data;
>> >> +    int next = END_NOT_FOUND;
>> >> +    int i = 0;
>> >> +
>> >> +    *poutbuf_size = 0;
>> >> +    if (buf_size == 0)
>> >> +        return 0;
>> >> +
>> >> +    if (!ppc->pc.frame_start_found) {
>> >> +        uint64_t state64 = ppc->pc.state64;
>> >> +        for (; i < buf_size; i++) {
>> >> +            state64 = (state64 << 8) | buf[i];
>> >> +            if (state64 == PNGSIG || state64 == MNGSIG) {
>> >> +                i++;
>> >> +                ppc->pc.frame_start_found = 1;
>> >> +                break;
>> >> +            }
>> >> +        }
>> >> +        ppc->pc.state64 = state64;
>> >> +    } else
>> >> +        if (ppc->remaining_size) {
>> >> +            i = FFMIN(ppc->remaining_size, buf_size);
>> >> +            ppc->remaining_size -= i;
>> >
>> >> +            if (ppc->remaining_size)
>> >> +                goto flush;
>> >
>> > unneeded, the for loop between here and flush wont execute for i=buf_size
>>
>> true, but if ppc->index == -1 then next is set!
>>
>
>> also if ppc->remaining_size < buf_size then i is < buf_size
>
> true, ive missed that,
> though the case should be handled as it can happen inside the for() too,
> ive not checked if it is

it is

to avoid possible overflow's i changed

-if (ppc->chunk_length + i >= buf_size) {
-    ppc->remaining_size = ppc->chunk_length + i - buf_size + 1;
+if (ppc->chunk_length >= buf_size - i) {
+    ppc->remaining_size = ppc->chunk_length - buf_size + i + 1;

please apply

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