[FFmpeg-devel] qt-faststart update

Frank Barchard fbarchard
Wed Jun 24 14:14:39 CEST 2009



--------------------------------------------------
From: "Diego Biurrun" <diego at biurrun.de>
Sent: Wednesday, June 24, 2009 2:54 AM
To: "FFmpeg development discussions and patches" <ffmpeg-devel at mplayerhq.hu>
Subject: Re: [FFmpeg-devel] qt-faststart update

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

Okay, that should reduce the diffs.  I'll try -E -b -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.

I'm needing VC for debugging.  It also produces faster code if built with 
Intel C or VC for Windows.
Keep in mind this is a stand alone tool, not linked into ffmpeg libraries.
I'll submit a separate patch to the original for VC support.

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

-b reduces the diffs a bit.

>
>
> This patch is still far too big, get rid of all the reindentation.
>
> Also, please set a correct content-type for your attachments.

I'm not sure how to do that?  The previous attachments were thru gmail.
For this one I've switched to Live Mail in plain text format.

>
> Diego
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fast_nowin.diff
Type: application/octet-stream
Size: 9280 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090624/a19d549e/attachment.obj>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: qt-faststart.nowin.c
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090624/a19d549e/attachment.txt>



More information about the ffmpeg-devel mailing list