[FFmpeg-devel] qt-faststart update
Frank Barchard
fbarchard
Wed Jun 24 10:52:36 CEST 2009
Hi Baptiste,Thanks for the feedback! Responses below
On Tue, Jun 23, 2009 at 8:18 PM, Baptiste Coudurier <
baptiste.coudurier at gmail.com> wrote:
> 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.
I tried for style consistency, and didn't think
it warranted a separate patch for that particular line.
But as you have flagged it, I'll remove the main() change.
Fixed.
>
>
> The goal is to minimize the size of the diff.
I just learned that today, and did try a few diffs, pasting back old code to
minimize the changes.
The overall file size remained about the same, but the order changed and
some code dropped into if statements, causing diff to think its a big
change.
>
> 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.
Okay.
>
> 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 ?
The worst case is a browser or protocol that doesnt handle seeks and
will have to cache the entire file before the movie can start.
Depends on the connection, but on the order of 30 seconds.
The next case is a protocol that can handle seeks, but needs to open a
new connection. Roughly 2 seconds.
The best case is harddrive or really fast protocol, where the savings
is about 150 milliseconds.
This change doesnt improve on savings - it just makes it possible for
ffmpeg to work efficiently with content created by mp4box, quicktime
and other tools.
>
> > 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.
Yes, I think we should look at that too. But the library change
is riskier, at least for a newbie like me, and doesn't eliminate the need
for qt-faststart.
It would only help with mp4box files, not ffmpeg or quicktime muxed files,
where the moov header can still be at the end.
>
>
> > 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 ?
This took awhile to gather. I've only got 41 interesting mov/mp4 files
handy... they tend to come from vimeo or apple.
29 of the files had a free atom, but not necessarily at the end.
1 had a free atom at end, and mdat just before that. So it was simply
truncated.
20 had moov after the mdat
8 of those had a free atom before the moov atom.
1 of those 20 with moov at the end, had a free atom after the moov.
1 of those 20 also had a uuid near the start of file, and was correctly
fixed up.
1 file had a uuid at end of file, and was not fixable. The moov header is
at the start of file, but ffmpeg will still seek to end of file to read the
uuid atom.
That leaves 20 of the 41 files that were already okay.
>
>
> '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.
qt-faststart maintains those atoms. It'll move the 'moov' and keep
everything else.
But those other atoms will cause runtime seeks.
I haven't seen this in practice.
If anyone runs into a case where qt-faststart could help, I'd be happy to
get sample data and try to fix it.
>
>
> > 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.
There are 4 variations of this, and it would be hard to backtrack and
isolate the changes now.
One case is the moov is near the end, but followed by a 'free'.
Another is the moov is near the end, but preceeded by a 'free'. This would
work, but it was necessary to call the tool twice to get rid of the trailing
free.
And the last is the last atom is a 'free' atom and the tool needs to
continue with just the copy rest of file.
Another is just error handling, if its not a valid case and it was crashing.
>
> > [...]
> >
> > 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.
I assumed that could happen, but in practice, large files use 64 bit atoms.
The behavior hasn't changed in this respect.
The over file content is moved by the size of the 'moov' header and
relocation only needs to know how much the overall file shifted.
>
> > 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.
Fixed
>
>
> > /* 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 :)
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.
>
> > [...]
> >
> > - /* 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.
Its 9 lines and a slight logic change with else and continue, but not an
absolutely necessary logic change.
So I've reverted it, but kept the new cleanup.
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?
>
> > [...]
> >
> > + 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.
If you're referring to the atom check "encountered non-QT", it was moved
after the atom print so you could see what atom failed. Which led to me
discovering uuid atoms, and a few files that claimed to be quicktime, but
were obviously not.
The printf is where it is, so it can respect 64 bit atoms.
> It would be great if you could resend a patch avoiding this.
If the printf were removed, the atom type check could be put back to where
it was. But it would make the tool a bit less friendly/useful?
>
>
> Thanks for the patch nonetheless :)
Thanks for reviewing it! Updated diff and full c file attached
>
> [...]
>
> --
> Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA
> Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
> FFmpeg maintainer http://www.ffmpeg.org
> _______________________________________________
> 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: qt-faststart.diff
Type: application/octet-stream
Size: 15119 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090624/c6eab944/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: qt-faststart.c
Type: application/octet-stream
Size: 11935 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090624/c6eab944/attachment-0001.obj>
More information about the ffmpeg-devel
mailing list