[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