[FFmpeg-devel] [PATCH] png parser

[Peter Holik]

Michael Niedermayer schrieb:
> On Mon, Jul 13, 2009 at 05:01:00PM +0200, Peter Holik wrote:
>> Michael Niedermayer schrieb:
>> > On Mon, Jul 06, 2009 at 09:17:23AM +0200, Peter Holik wrote:
>> >> 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.
>> >
>> > Iam quite unhappy about that as it is worse readabilitywise IMHO and requires
>> > chunk_length to be part of the context vs. a local variable but if its
>> > important to you ...
>>
>> chunk_length has to be part of the context because if buffer ends with
>> the chunk length it can not be a local var.
>
> its in state64

no, please have a look at ff_combine_frame.
the only position where state64 is updated is if next is negative.

>> >> +    for (;ppc->pc.frame_start_found && i < buf_size; i++) {
>> >> +        ppc->pc.state = (ppc->pc.state<<8) | buf[i];
>> >
>> >> +        if (ppc->index == 3)
>> >> +            ppc->chunk_length = AV_RL32(&ppc->pc.state) + 4;
>> >> +        else if (ppc->index == 7) {
>> >
>> > if(){
>> > }else
>>
>> ok done, but i don't know why needed.
>
> because if you write
>
> if (ppc->index == 3)
>     ppc->chunk_length = AV_RL32(&ppc->pc.state) + 4;
> else if (ppc->index == 7) {
>
> and later add 1 line, then you get a patch like:
>
> -if (ppc->index == 3)
> +if (ppc->index == 3) {
>      ppc->chunk_length = AV_RL32(&ppc->pc.state) + 4;
> -else if (ppc->index == 7) {
> +    ppc->foobar=1;
> +} else if (ppc->index == 7) {
>
> vs.
>
>  if (ppc->index == 3) {
>      ppc->chunk_length = AV_RL32(&ppc->pc.state) + 4;
> +    ppc->foobar=1;
>  } else if (ppc->index == 7) {
>
> i think its obvious which is more readable

ok to have a beautiful patch i accept your comment.

>> >> +            if (AV_RB32(&ppc->pc.state) == MKTAG('I', 'E', 'N', 'D')) {
>> >> +                if (ppc->chunk_length >= buf_size - i) {
>> >
>> >> +                    ppc->remaining_size = ppc->chunk_length - buf_size + i + 1;
>> >
>> > this can still overflow
>>
>> how to prevent a overflow?
>
> id simply check that chunk_length is sane, not sure if that is all that is
> needed

ok, now i check for ppc->chunk_length > 0x7fffffff and let ff_combine_frame
accumulate all but set frame_start_found to 0 -> the decoder should hande this.

cu Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: png-parser.diff
Type: text/x-diff
Size: 5242 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090717/0578d870/attachment.diff>