[FFmpeg-devel] [PATCH] png parser

Michael Niedermayer michaelni
Wed Jul 1 19:06:10 CEST 2009


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


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


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090701/ff3ca6f0/attachment.pgp>



More information about the ffmpeg-devel mailing list