[FFmpeg-devel] qt-faststart update

[Baptiste Coudurier]

Hi,

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.

The goal is to minimize the size of the diff.

You don't reindent in the patch if reindent is needed on >=3 lines.
Reindent will be done in a separate step afterwards. This usually makes
the review process easier.

For example, old code:

a = b;
c = d;
e = f;

new code:
if (e > 5) {
a = b;
c = d;
e = f;
}

Don't reindent the middle 2 lines, this minimize the size of the diff.

> Why?
> When decoding mp4 files with ffmpeg, a seek to end of file slows down
> startup latency.

How much is the latency ?

> qt-faststart moves the 'moov' header atom to the beginning of the file.
>  Which works great if you use ffmpeg to mux, but if you use quicktime or
> mp4box to mux, ffmpeg demux will still seek to end of file?

Well, I believe the demuxer could be changed to avoid this case, which would
avoid modifying a ton of files if only the seek is causing problems,
on the other end, seek can be wanted, see below.

> Unless you use url_is_streamed, the open will scan thru all the atoms, and
> if anything follows mdat, a far seek will occur.
> In practice, 'free' atoms are being added by to the end of file.   The atoms
> are not required and removing them solves the seek.

Do you have statistics on the presence of only 'free' atoms at the end of
files ?

'mdat' can be followed by fragments and it will be better for the
demuxer to parse them at the beginning if file is seekable.
You can also have useful userdata which the user will definitely want,
no matter the structure of the file, so you might want to seek
at end of file to verify, I don't know much it happens in the wild though.

> How?
> The old code would move the 'moov' header after 'ftyp', adjust stco offsets,
> and output the rest of the file.
> It would fail if the last atom was not 'moov'.

That's a fix to the code, and would be very welcome as a separate
patch which only fix this.

> [...]
> 
> Future work
> Instead of moving the 'moov' to the beginning, it would be better to move
> the 'mdat' to the end.  This would stop other atoms, such as 'uuid' from
> causing seeks to the end.

Yes, indeed, beware that you can have multiple 'mdat' and fragments
which should not be moved.

> It would also allow 'free' atoms to be maintained, incase some tools are
> using them for meta information.
> But that would be a bigger change with more risk.
> 
> Attached is the diff, and full .c file.

> --- tools/qt-faststart.orig.c	2009-05-07 21:41:30.000000000 -0700
> +++ tools/qt-faststart.c	2009-06-23 18:55:32.789801000 -0700
> @@ -1,6 +1,7 @@
>
> [...]
>
> @@ -108,21 +123,10 @@ int main(int argc, char *argv[])
>          if (fread(atom_bytes, ATOM_PREAMBLE_SIZE, 1, infile) != 1) {
>              break;
>          }
> -        atom_size = (uint32_t)BE_32(&atom_bytes[0]);
> -        atom_type = BE_32(&atom_bytes[4]);
>
> [...]
>
> +        atom_type = BE_32(&atom_bytes[4]);
> +        atom_offset = ftello(infile) - ATOM_PREAMBLE_SIZE;
> +        atom_size = (uint32_t)BE_32(&atom_bytes[0]);

Reordering of lines can be avoided.

>          /* keep ftyp atom */
>          if (atom_type == FTYP_ATOM) {
> @@ -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 :)

> [...]
>
> -        /* 64-bit special case */
> -        if (atom_size == 1) {
> -            if (fread(atom_bytes, ATOM_PREAMBLE_SIZE, 1, infile) != 1) {
> -                break;
> -            }
> -            atom_size = BE_64(&atom_bytes[0]);
> -            fseeko(infile, atom_size - ATOM_PREAMBLE_SIZE * 2, SEEK_CUR);
>          } else {
> -            fseeko(infile, atom_size - ATOM_PREAMBLE_SIZE, SEEK_CUR);
> +            /* 64-bit special case */
> +            if (atom_size == 1) {
> +                if (fread(atom_bytes, ATOM_PREAMBLE_SIZE, 1, infile) != 1) {
> +                    break;
> +                }
> +                atom_size = BE_64(&atom_bytes[0]);
> +                fseeko(infile, atom_size - ATOM_PREAMBLE_SIZE * 2, SEEK_CUR);
> +            } else {
> +                fseeko(infile, atom_size - ATOM_PREAMBLE_SIZE, SEEK_CUR);
> +            }

Here you could avoid reindenting.

> [...]
>
> +        if ((atom_type != FREE_ATOM) &&
> +            (atom_type != JUNK_ATOM) &&
> +            (atom_type != MDAT_ATOM) &&
> +            (atom_type != MOOV_ATOM) &&
> +            (atom_type != PNOT_ATOM) &&
> +            (atom_type != SKIP_ATOM) &&
> +            (atom_type != WIDE_ATOM) &&
> +            (atom_type != PICT_ATOM) &&
> +            (atom_type != UUID_ATOM) &&
> +            (atom_type != FTYP_ATOM)) {
> +            printf ("encountered non-QT top-level atom (is this a Quicktime file?)\n");
> +            break;
> +        }

Code was moved around which makes review harder.
It would be great if you could resend a patch avoiding this.

Thanks for the patch nonetheless :)

[...]

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA    
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org