[FFmpeg-devel] [PATCH] png parser

[Michael Niedermayer]

On Mon, Jul 27, 2009 at 08:48:02AM +0200, Peter Holik wrote:
> Michael Niedermayer schrieb:
> > On Sun, Jul 26, 2009 at 09:17:57AM +0200, Peter Holik wrote:
> >> Michael Niedermayer schrieb:
> >> > On Sat, Jul 25, 2009 at 03:08:33PM +0200, Peter Holik wrote:
> >> >> Michael Niedermayer schrieb:
> >> >> > On Wed, Jul 22, 2009 at 08:32:09AM +0200, Peter Holik wrote:
> >> >> >> Michael Niedermayer schrieb:
> >> >> >> > On Sat, Jul 18, 2009 at 09:00:08AM +0200, Peter Holik wrote:
> >> >> >> >> Michael Niedermayer schrieb:
> >> >> >> >> > On Fri, Jul 17, 2009 at 09:40:13AM +0200, Peter Holik wrote:
> >> >> >> >> >> 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.
> >> >> >> >> >
> >> >> >> >> > grep state parser.c will show state64 is updated at every point that state is.
> >> >> >> >>
> >> >> >> >> #> grep state64 parser.c
> >> >> >> >> pc->state64 = (pc->state64<<8) | pc->buffer[pc->last_index + next];
> >> >> >> >>
> >> >> >> >> only one time, where state64 is updated
> >> >> >> >
> >> >> >> > yes, state is also only updated one time, and exactly in the line before
> >> >> >> > state64
> >> >> >> > so if you can use state you also should be able to use state64
> >> >> >>
> >> >> >> but this update is never available to me because i NEVER have a negative next for
> >> >> >> ff_combine_frame!
> >> >> >
> >> >> > its the same with state
> >> >>
> >> >> yes!
> >> >>
> >> >> So what is now wrong with my patch?
> >> >
> >> > i think it can be simplified by using state64, if you convince me that this
> >> > is not the case id drop the issue.
> >> > (to convince me, just show me the code that uses state64, half of it is
> >> >  the chunk type the other the chunk_length, which is then loaded into a
> >> >  local var)
> >>
> >> i only use state64 for searching signature:
> >>
> >> +    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) {
> >>
> >> to parse the chunks only state is used:
> >>
> >> +    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);
> >
> > it doesnt sound like a terribly good idea to mix them ...
> > you can use a local 32bit var where speed matters (i doubt this is executed
> > often enogh though) but i would not mix state & state64
> 
> ???
> 
> is it ok if i use my own state and state64 variables in my PNGParseContext?

what would be the point?
there are already such variables in the context
and you need just 1 variable in the context to represent the past bytes
using 2 at different points is not good, you risk having the used variable
not updated properly in the past or if it is then use state64 for
the chunk_length as i described


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- 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/20090728/23be7f21/attachment.pgp>