On Fri, Jun 01, 2018 at 07:00:20AM +0000, Eran Kornblau wrote: > > On Thu, May 31, 2018 at 10:11:38AM +0000, Eran Kornblau wrote: > > > > > > > > -----Original Message----- > > > > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On > > > > Behalf Of Eran Kornblau > > > > Sent: Friday, May 25, 2018 4:40 PM > > > > To: FFmpeg development discussions and patches > > > > <ffmpeg-devel@ffmpeg.org> > > > > Subject: [FFmpeg-devel] qt-faststart bug near 4GB > > > > > > > > Hi all, > > > > > > > > We encountered a rather extreme edge case with qt-faststart - we > > > > transcoded some video with ffmpeg, and the offset of the last video > > > > frame in the resulting mp4 was slightly less than 4GB. > > > > Since it was less than 4GB, ffmpeg used an 'stco' atom and not a 'co64' > > > > atom. > > > > When we ran qt-faststart on this file, it added the moov atom size to > > > > all offsets in the 'stco' atom, causing an overflow in the offsets of > > > > the frames close to the end of the file. The end of the video was > > > > therefore corrupt and could not be played. > > > > I think the solution here is to 'upgrade' the 'stco' atom to 'co64' if > > > > such an edge case happens. However, looking at the code of > > > > qt-faststart, I see that it doesn't actually parse the atom tree, but > > > > rather looks for the strings 'stco' / 'co64'. Changing 'stco' to 'co64' > > > > requires updating the size of all the atom in which it's contained > > > > (moov, trak, mdia etc.) Therefore, such a change would probably be more > > > > of a rewrite of this utility than a patch, so wanted to check whether > > > > anyone has any thoughts on this before I start writing... > > > > > > > Attaching the patch for this issue. > > > As expected, it required significant changes... hope you will like it > > > :) > > > > > > Thanks! > > > > > > Eran > > > > about the AV_WB* macros, i like them alot :) but this seems not to apply > > cleanly: > > > > Applying: qt-faststart - stco offset bug fix Using index info to > > reconstruct a base tree... > > M tools/qt-faststart.c > > Falling back to patching base and 3-way merge... > > Auto-merging tools/qt-faststart.c > > CONFLICT (content): Merge conflict in tools/qt-faststart.c > > error: Failed to merge in the changes. > > Patch failed at 0001 qt-faststart - stco offset bug fix Use 'git am > > --show-current-patch' to see the failed patch When you have resolved this > > problem, run "git am --continue". > > If you prefer to skip this patch, run "git am --skip" instead. > > To restore the original branch and stop patching, run "git am --abort". > > > Sorry Michael, I forgot to revert the previous patch before committing. > Attaching a fixed patch. > > Thanks > > Eran > > > [...] > > > > -- > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > Into a blind darkness they enter who follow after the Ignorance, they as if > > into a greater darkness enter who devote themselves to the Knowledge alone. > > -- Isha Upanishad > >
> qt-faststart.c | 400
> +++++++++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 345 insertions(+), 55 deletions(-)
> e4e6ca876f190a354031d269b53655effa0eaa20
> 0001-qt-faststart-stco-offset-bug-fix.patch
> From be7f3b43f5039297809dd88ead218e2523769064 Mon Sep 17 00:00:00 2001
> From: erankor <eran.kornb...@kaltura.com>
> Date: Fri, 1 Jun 2018 09:55:45 +0300
> Subject: [PATCH] qt-faststart - stco offset bug fix
>
> when the last offsets in the stco atom are close to 4GB, the addition of
> the moov atom size can overflow, causing corruption near the end of the
> mp4 file.
> this patch upgrades all stco atoms to co64 when such an edge case is
> detected. in order to accomplish this, the implementation was changed to
> walk the atom tree, instead of searching for the strings 'stco'/'co64'.
> this was required since when an stco atom is changed to co64, its size
> changes, and the sizes of all containing atoms (moov, trak, etc.) have
> to be updated as well.
> ---
> tools/qt-faststart.c | 400
> ++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 345 insertions(+), 55 deletions(-)
>
> diff --git a/tools/qt-faststart.c b/tools/qt-faststart.c
> index d0ae7245f3..40eb3fcfc8 100644
> --- a/tools/qt-faststart.c
> +++ b/tools/qt-faststart.c
> @@ -28,6 +28,7 @@
> #include <stdlib.h>
> #include <inttypes.h>
> #include <string.h>
> +#include <limits.h>
>
> #ifdef __MINGW32__
> #undef fseeko
> @@ -43,8 +44,6 @@
>
> #define MIN(a,b) ((a) > (b) ? (b) : (a))
>
> -#define BE_16(x) ((((uint8_t*)(x))[0] << 8) | ((uint8_t*)(x))[1])
> -
> #define BE_32(x) (((uint32_t)(((uint8_t*)(x))[0]) << 24) | \
> (((uint8_t*)(x))[1] << 16) | \
> (((uint8_t*)(x))[2] << 8) | \
> @@ -59,6 +58,18 @@
> ((uint64_t)(((uint8_t*)(x))[6]) << 8) | \
> ((uint64_t)( (uint8_t*)(x))[7]))
>
> +#define AV_WB32(p, val) { \
> + ((uint8_t*)(p))[0] = ((val) >> 24) & 0xff; \
> + ((uint8_t*)(p))[1] = ((val) >> 16) & 0xff; \
> + ((uint8_t*)(p))[2] = ((val) >> 8) & 0xff; \
> + ((uint8_t*)(p))[3] = (val) & 0xff; \
> + }
> +
> +#define AV_WB64(p, val) { \
> + AV_WB32(p, (val) >> 32) \
> + AV_WB32(p + 4, val) \
> + }
> +
> #define BE_FOURCC(ch0, ch1, ch2, ch3) \
> ( (uint32_t)(unsigned char)(ch3) | \
> ((uint32_t)(unsigned char)(ch2) << 8) | \
> @@ -79,12 +90,342 @@
> #define UUID_ATOM QT_ATOM('u', 'u', 'i', 'd')
>
> #define CMOV_ATOM QT_ATOM('c', 'm', 'o', 'v')
> +#define TRAK_ATOM QT_ATOM('t', 'r', 'a', 'k')
> +#define MDIA_ATOM QT_ATOM('m', 'd', 'i', 'a')
> +#define MINF_ATOM QT_ATOM('m', 'i', 'n', 'f')
> +#define STBL_ATOM QT_ATOM('s', 't', 'b', 'l')
> #define STCO_ATOM QT_ATOM('s', 't', 'c', 'o')
> #define CO64_ATOM QT_ATOM('c', 'o', '6', '4')
>
> #define ATOM_PREAMBLE_SIZE 8
> #define COPY_BUFFER_SIZE 33554432
>
> +typedef struct {
> + uint32_t type;
> + uint32_t header_size;
> + uint64_t size;
> + unsigned char *data;
> +} atom_t;
> +
> +typedef struct {
> + uint64_t moov_atom_size;
> + uint64_t stco_offset_count;
> + uint64_t stco_data_size;
> + int stco_overflow;
> + uint32_t depth;
> +} update_chunk_offsets_context_t;
> +
> +typedef struct {
> + unsigned char *dest;
> + uint64_t original_moov_size;
> + uint64_t new_moov_size;
> +} upgrade_stco_context_t;
> +
> +typedef int (*parse_atoms_callback_t)(void *context, atom_t *atom);
> +
> +static int parse_atoms(
> + unsigned char *buf,
> + uint64_t size,
> + parse_atoms_callback_t callback,
> + void *context)
> +{
> + unsigned char *pos = buf;
> + unsigned char *end = pos + size;
> + atom_t atom;
> + int ret;
> +
> + while (end - pos >= ATOM_PREAMBLE_SIZE) {
> + atom.size = BE_32(pos);
> + atom.type = BE_32(pos + 4);
> + pos += ATOM_PREAMBLE_SIZE;
> + atom.header_size = ATOM_PREAMBLE_SIZE;
> +
> + switch (atom.size) {
> + case 1:
> + if (end - pos < 8) {
> + printf("not enough room for 64 bit atom size\n");
> + return -1;
> + }
> +
> + atom.size = BE_64(pos);
> + pos += 8;
> + atom.header_size = ATOM_PREAMBLE_SIZE + 8;
> + break;
> +
> + case 0:
> + atom.size = ATOM_PREAMBLE_SIZE + end - pos;
> + break;
> + }
> +
> + if (atom.size < atom.header_size) {
> + printf("atom size %"PRIu64" too small\n", atom.size);
errors should go to stderr if av_log() is not used
i see this is not introduced by the patch but was that way before so its
more a comment about the code in git than the patch ...
but ideally this should be fixed in a seperate patch either before or after
this one
some self test for the newly added feature would also be a good idea
also, was the new code tested with a fuzzer ?
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
