[FFmpeg-devel] qt-faststart update

[Diego Biurrun]

On Wed, Jun 24, 2009 at 01:52:36AM -0700, Frank Barchard wrote:
> 
> On Tue, Jun 23, 2009 at 8:18 PM, Baptiste Coudurier <
> baptiste.coudurier at gmail.com> wrote:
> 
> > On Tue, Jun 23, 2009 at 07:39:55PM -0700, Frank Barchard wrote:
> > > Patch to allow qt-faststart to work on output of mp4box and quicktime, as
> > > well as ffmpeg muxer, to reduce seeks and startup time when playing back
> > > with ffmpeg based tools.
> > > I'm new to this group, so I hope you'll be gentle as I try to submit my
> > > first change to the handy tools/qt-faststart.c
> >
> > Sure, first you have to separate cosmetics and functionnal changes.
> > For example the prototype for main and { placement should go in a
> > separate patch.
> 
> I tried for style consistency, and didn't think
> it warranted a separate patch for that particular line.

That was style inconsistency you introduced..

> But as you have flagged it, I'll remove the main() change.
> Fixed.

Good.

> > > @@ -131,134 +135,139 @@ int main(int argc, char *argv[])
> > >              if (!ftyp_atom) {
> > >                  printf ("could not allocate 0x%llX byte for ftyp
> > atom\n",
> > >                          atom_size);
> > > -                fclose(infile);
> > > -                return 1;
> > > +                goto exit_cleanup;
> >
> > Factorizing cleanup at exit is a good thing, in a separate patch :)
> 
> It would be hard to do the new code without the cleanup, but it wouldnt be
> hard to fix up the old code.
> Estimate 3 hours.  But the old tool is not useful to me as is, without the
> new fixes.

It should not take you 3 hours to separate the factorization into a
self-contained patch.

> note In my first attempt at reducing the diff, I introduced a bug.  The
> original had a continue, which I changed to and else, which avoided the
> fseek to skip the rest of the atom.
> I've had to put the else back in, but avoiding indenting the 10 lines within
> it.  Are you sure this is best practice?

Yes.

Try looking at some diff options to avoid whitespace changes, i.e. -w -b.

> --- tools/qt-faststart.orig.c	2009-05-07 21:41:30.000000000 -0700
> +++ tools/qt-faststart.c	2009-06-24 01:04:39.804309200 -0700
> @@ -26,7 +27,15 @@
>  
>  #include <stdio.h>
>  #include <stdlib.h>
> +#ifndef _MSC_VER
>  #include <inttypes.h>
> +#else
> +#define uint64_t unsigned __int64
> +#define uint32_t unsigned int
> +#define uint8_t unsigned char
> +#define ftello _ftelli64
> +#define fseeko _fseeki64
> +#endif

We do not support Visual Studio, get rid of this.  In any case, it would
have to be a separate patch.

> @@ -155,110 +154,121 @@ int main(int argc, char *argv[])
>  
> -    /* moov atom was, in fact, the last atom in the chunk; load the whole
> -     * moov atom */
> -    fseeko(infile, -atom_size, SEEK_END);
> -    last_offset = ftello(infile);
> -    moov_atom_size = atom_size;
> -    moov_atom = malloc(moov_atom_size);
> -    if (!moov_atom) {
> -        printf ("could not allocate 0x%llX byte for moov atom\n",
> -            atom_size);
> -        fclose(infile);
> -        return 1;
> -    }
> -    if (fread(moov_atom, atom_size, 1, infile) != 1) {
> -        perror(argv[1]);
> -        free(moov_atom);
> -        fclose(infile);
> -        return 1;
> -    }
>  
>  
> +        /* moov atom was, in fact, the last atom in the chunk; load the whole
> +         * moov atom */
> +        fseeko(infile, last_atom_offset, SEEK_SET);
> +        moov_atom_size = last_atom_size;
> +        moov_atom = malloc(moov_atom_size);
> +        if (!moov_atom) {
> +            printf ("could not allocate 0x%llX byte for moov atom\n",
> +                atom_size);
> +            goto exit_cleanup;
> +        }
> +        if (fread(moov_atom, moov_atom_size, 1, infile) != 1) {
> +            perror(argv[1]);
> +            goto exit_cleanup;
> +        }

I think you should be able to do this without all the reindentation
cosmetics, same elsewhere.

> @@ -266,20 +276,24 @@ int main(int argc, char *argv[])
>  
> -    /* dump the new moov atom */
> -    printf (" writing moov atom...\n");
> -    if (fwrite(moov_atom, moov_atom_size, 1, outfile) != 1) {
> -        perror(argv[2]);
> -        goto error_out;
> +    if (!just_remove_free) {
> +        /* dump the new moov atom */
> +        printf (" writing moov atom...\n");
> +        if (fwrite(moov_atom, moov_atom_size, 1, outfile) != 1) {
> +            perror(argv[2]);
> +            goto exit_cleanup;
> +        }

more reindentation cosmetics


This patch is still far too big, get rid of all the reindentation.

Also, please set a correct content-type for your attachments.

Diego