From patchwork Tue Jul 18 00:54:26 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dale Curtis X-Patchwork-Id: 4347 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.1.76 with SMTP id 73csp833257vsb; Mon, 17 Jul 2017 17:54:58 -0700 (PDT) X-Received: by 10.28.199.200 with SMTP id x191mr179615wmf.94.1500339297941; Mon, 17 Jul 2017 17:54:57 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1500339297; cv=none; d=google.com; s=arc-20160816; b=S0S1bNYGSleMWiUkgr/XjHTRh5n9Eea7NgXV7Ht0/DwOdlePU5kvc0cn3fcFaMvJWN YCRe7vUcN9JwQ48XkiBvl81o8ASfeANsI9Zsfz0mowCaqc5/Njcx0dTUA/0Snun4j613 h4Mds76+QSa5BFivl6l0pT00R9LmT4JnSgtRZzOhMZD/PV8K/k6CvQ3UK87/aFPZ2pRd nPxIbzS2oQ01Uzt+9I+3rlrX+QLurNgKp769zTpEBBhhipNGPBZz8iVoTSpVmq3Q78F9 omP7PeSs5Ell03kr8NFTWCQhXEeY/w9QNlf7cYNEpDM+C6X2SNO7oI2F5OPQuO3lfVva zQSA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:cc:reply-to:list-subscribe:list-help:list-post :list-archive:list-unsubscribe:list-id:precedence:subject:to :message-id:date:from:references:in-reply-to:mime-version :dkim-signature:dkim-signature:delivered-to :arc-authentication-results; bh=jNL1aQCYvRVMj3RNGPyMl1jmIr/mehg7pux78tebFTs=; b=jsVe0ceMSPnEqEyTdtcpJ14+QNHsjMgQ5yMTOwcLmKtIBi/q7oxk72u0kx4NEGaluH WWOSAqoaMRBCMQU/tHP5uuCCRY3wHXN3FuUGShShhkbQINjTzke3Os2QYpuOsz1Wxh7q dHOLkOIKBQDJmgd3/2W0Ti3EgL8tY3bW7BtD8M7uLiH9kkOFCUnmcAaC5XkBdgWtVybs S2NNOydwBrbkvTKwxKkOiwrW82DMOTy5a+9uG4CllpYf4UlTffGZ3QX9riCDSFrMxV65 KhsnqvFi9k8GjIVgPZUmbZiDd4L3lMNymUl8EbRZvt9RYpeavykgaoCiUfQoM/S3jzt2 aZBg== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@google.com header.b=izsLY6Ld; dkim=neutral (body hash did not verify) header.i=@chromium.org header.b=QbkvHv5p; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=chromium.org Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id i135si11294840wmd.125.2017.07.17.17.54.57; Mon, 17 Jul 2017 17:54:57 -0700 (PDT) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@google.com header.b=izsLY6Ld; dkim=neutral (body hash did not verify) header.i=@chromium.org header.b=QbkvHv5p; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 2175B6882BD; Tue, 18 Jul 2017 03:54:48 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr0-f170.google.com (mail-wr0-f170.google.com [209.85.128.170]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 8A98B688295 for ; Tue, 18 Jul 2017 03:54:41 +0300 (EEST) Received: by mail-wr0-f170.google.com with SMTP id w4so6396521wrb.2 for ; Mon, 17 Jul 2017 17:54:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=eUqlefBsA8EVDo/DqfJrk/lASfC+4ehMXB4CrL5WhqA=; b=izsLY6Ld5fhmEptaVKKh7QX3DI4X1qyTJiXYXwlaKU2fvlCXQTFt3fmSYAug8UF6nY NCXiaqajvd0C6YvvKQwrDbnvFAU+hAWF/yeTMTYZM+Crpod4yQC6NBHP8AeUfq7gmDu1 mXUalwxEO4iDB4yHQRavp9eFH9ADWzM9LfBQdnaPQBC5Yz2nSS+8mcRXRkJoV+QmKAW4 SQNMq+aTTAS8WZDhC/miROlYLpGeixsMlZcOgwEdMAChGU/8WYy8T3HAWkfm4yUh6XhH TbiCpvEtVcpiHEaRuinKbdAd6n8VhhrXCjWLm0o1AnKKEf2tQbB9p8OoG4QFSLJ90f27 J0rg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=eUqlefBsA8EVDo/DqfJrk/lASfC+4ehMXB4CrL5WhqA=; b=QbkvHv5p7WxbOy8i+E6vUQiWALnvx01heO0IydBIY5ZZWCVMqpdxmlfk8oCfd/1Ddw t5aoCyfWuJAPRmph0i00GRBdSHjC/nxGdxAgN5bijvFAppndJhPWC4TnDO0bUXoX9cx5 NYNOs4mkiZv6nqeCulxp07BXX0IqCf1KByVu0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=eUqlefBsA8EVDo/DqfJrk/lASfC+4ehMXB4CrL5WhqA=; b=cP6SyKMfaRfa620d8EU2B3KFh2LUeSnl+XO4i8atMEGRXUL087JxINFEVipsjHjX2E BIae8TBS4XNNO6rtRBgpwGVn3eH2Xx4ZQSOo99nE1iVZSkjdrhOjvFOZM53Rz+F3U2Qy Ulblkpnwd4tYaDsyztHRkQOeJHf24imOLO+JjioFC3hAq3/wKqq5DbznUU44m+GYbDMk xkSgplftnstyV8osCt951xjlNyv9IdCEYSARCh8Ubuy+Di0BP9Y9EwyUJBvDxOIzchSQ mwYV2VyUwncTqCv8SqkXoX6M+rs1fRHeSMAF1OmjfTS7Be78ovp+Hr7M/Jr2PI0vatrZ te7A== X-Gm-Message-State: AIVw112If/rUMRPWLkOZTWZ/k+GdRxpuQxMCjBo5Qsh5A/v6xhCzEYRM Qs46N5gkZYaRK6Hbf9KEZRqx7YfA2o9IbBnhXw== X-Received: by 10.28.131.202 with SMTP id f193mr147101wmd.89.1500339287796; Mon, 17 Jul 2017 17:54:47 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.71.149 with HTTP; Mon, 17 Jul 2017 17:54:26 -0700 (PDT) In-Reply-To: References: <1444121423-25972-1-git-send-email-rodger.combs@gmail.com> <20151006161821.GA4540@nb4> <20160115225719.GL13213@nb4> <87F93A93-BC23-4611-AEA0-60887D5CB076@gmail.com> <20170715003859.GQ3740@nb4> From: Dale Curtis Date: Mon, 17 Jul 2017 17:54:26 -0700 X-Google-Sender-Auth: 9IbJxsy0QEwowaqiSwSor7eGLl8 Message-ID: To: FFmpeg development discussions and patches X-Content-Filtered-By: Mailman/MimeDel 2.1.20 Subject: Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: Dan Sanders , Dan Sanders Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Okay the following patch should fix this issue. Chromium/reviewable version here https://chromium-review.googlesource.com/c/572585/ - dale On Fri, Jul 14, 2017 at 6:31 PM, Dale Curtis wrote: > On Fri, Jul 14, 2017 at 5:38 PM, Michael Niedermayer < > michael@niedermayer.cc> wrote: > >> >> I dont think CTTS is the only affected atom. >> >> > Thanks for the response. Yes, I think that's likely true. The ctts one is > just likely the most important since it controls timestamp ordering. > > >> what you describe sounds like an option. but i might be missing something, >> possibly even something significant. I dont feel that confident with >> this code after quickly looking at it. >> > > Here are my experiments thus far if you have further thoughts: > https://chromium-review.googlesource.com/c/572730/ > https://chromium-review.googlesource.com/c/572585/ > > Both fixes and the existing code seem to suffer from assuming that the > sample number in the AVIndex and that of the ctts_data are the same; which > seems questionable. The second experiment should be viable with the > addition of memmove() when inserting in the middle of the array. Will look > at it more next week. > > - dale > > > From 4460d4b16472d1fd201818df6c18ba3e515941c4 Mon Sep 17 00:00:00 2001 From: Dale Curtis Date: Mon, 17 Jul 2017 17:38:09 -0700 Subject: [PATCH] Fix trampling of ctts during seeks when sidx support is enabled. When sidx box support is enabled, the code will skip reading all trun boxes (each containing ctts entries for samples inthat box). If seeks are attempted before all ctts values are known, the old code would dump ctts entries into the wrong location. These are then used to compute pts values which leads to out of order and incorrectly timestamped packets. This patch fixes ctts processing by always using the index returned by av_add_index_entry() as the ctts_data index. When the index gains new entries old values are reshuffled as appropriate. This approach makes sense since the mov demuxer is already relying on the mapping of AVIndex entries to samples for correct demuxing. Notes for future improvement: Probably there are other boxes (stts, stsc, etc) that are impacted by this issue... this patch only attempts to fix ctts since it completely breaks packet timestamping. This patch continues using an array for the ctts data, which is not the most ideal given the rearrangement that needs to happen (via memmove as new entries are read in). Ideally AVIndex and the ctts data would be set-type structures so addition is always worst case O(lg(n)) instead of the O(n^2) that exists now; this slowdown is noticeable during seeks. Additionally since ctts samples from trun boxes always have a count of 1, there's probably an opportunity to avoid the post-seek fixup that iterates O(n) over all samples with an O(1) when no non-1 count samples are present. Signed-off-by: Dale Curtis --- libavformat/isom.h | 1 + libavformat/mov.c | 46 +++++++++++++++++++++++++++++++--------------- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/libavformat/isom.h b/libavformat/isom.h index ff009b0896..fdd98c28f5 100644 --- a/libavformat/isom.h +++ b/libavformat/isom.h @@ -137,6 +137,7 @@ typedef struct MOVStreamContext { unsigned int stts_count; MOVStts *stts_data; unsigned int ctts_count; + unsigned int ctts_allocated_size; MOVStts *ctts_data; unsigned int stsc_count; MOVStsc *stsc_data; diff --git a/libavformat/mov.c b/libavformat/mov.c index 63f84be782..412290b435 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -4297,11 +4297,6 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) } if ((uint64_t)entries+sc->ctts_count >= UINT_MAX/sizeof(*sc->ctts_data)) return AVERROR_INVALIDDATA; - if ((err = av_reallocp_array(&sc->ctts_data, entries + sc->ctts_count, - sizeof(*sc->ctts_data))) < 0) { - sc->ctts_count = 0; - return err; - } if (flags & MOV_TRUN_DATA_OFFSET) data_offset = avio_rb32(pb); if (flags & MOV_TRUN_FIRST_SAMPLE_FLAGS) first_sample_flags = avio_rb32(pb); dts = sc->track_end - sc->time_offset; @@ -4312,26 +4307,28 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) unsigned sample_size = frag->size; int sample_flags = i ? frag->flags : first_sample_flags; unsigned sample_duration = frag->duration; + unsigned ctts_duration = 0; int keyframe = 0; + int ctts_index = 0; + int old_nb_index_entries = st->nb_index_entries; if (flags & MOV_TRUN_SAMPLE_DURATION) sample_duration = avio_rb32(pb); if (flags & MOV_TRUN_SAMPLE_SIZE) sample_size = avio_rb32(pb); if (flags & MOV_TRUN_SAMPLE_FLAGS) sample_flags = avio_rb32(pb); - sc->ctts_data[sc->ctts_count].count = 1; - sc->ctts_data[sc->ctts_count].duration = (flags & MOV_TRUN_SAMPLE_CTS) ? - avio_rb32(pb) : 0; - mov_update_dts_shift(sc, sc->ctts_data[sc->ctts_count].duration); + if (flags & MOV_TRUN_SAMPLE_CTS) ctts_duration = avio_rb32(pb); + + mov_update_dts_shift(sc, ctts_duration); if (frag->time != AV_NOPTS_VALUE) { if (c->use_mfra_for == FF_MOV_FLAG_MFRA_PTS) { int64_t pts = frag->time; av_log(c->fc, AV_LOG_DEBUG, "found frag time %"PRId64 " sc->dts_shift %d ctts.duration %d" " sc->time_offset %"PRId64" flags & MOV_TRUN_SAMPLE_CTS %d\n", pts, - sc->dts_shift, sc->ctts_data[sc->ctts_count].duration, + sc->dts_shift, ctts_duration, sc->time_offset, flags & MOV_TRUN_SAMPLE_CTS); dts = pts - sc->dts_shift; if (flags & MOV_TRUN_SAMPLE_CTS) { - dts -= sc->ctts_data[sc->ctts_count].duration; + dts -= ctts_duration; } else { dts -= sc->time_offset; } @@ -4343,7 +4340,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) } frag->time = AV_NOPTS_VALUE; } - sc->ctts_count++; + if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) keyframe = 1; else @@ -4352,11 +4349,30 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) MOV_FRAG_SAMPLE_FLAG_DEPENDS_YES)); if (keyframe) distance = 0; - err = av_add_index_entry(st, offset, dts, sample_size, distance, - keyframe ? AVINDEX_KEYFRAME : 0); - if (err < 0) { + ctts_index = av_add_index_entry(st, offset, dts, sample_size, distance, + keyframe ? AVINDEX_KEYFRAME : 0); + if (ctts_index >= 0 && old_nb_index_entries < st->nb_index_entries) { + unsigned int size_needed = st->nb_index_entries * sizeof(*sc->ctts_data); + unsigned int request_size = size_needed > sc->ctts_allocated_size ? + FFMAX(size_needed, 2 * sc->ctts_allocated_size) : size_needed; + sc->ctts_data = av_fast_realloc(sc->ctts_data, &sc->ctts_allocated_size, request_size); + if (!sc->ctts_data) { + sc->ctts_count = 0; + return AVERROR(ENOMEM); + } + + if (ctts_index != old_nb_index_entries) { + memmove(sc->ctts_data + ctts_index + 1, sc->ctts_data + ctts_index, + sizeof(*sc->ctts_data) * (sc->ctts_count - ctts_index)); + } + + sc->ctts_data[ctts_index].count = 1; + sc->ctts_data[ctts_index].duration = ctts_duration; + sc->ctts_count++; + } else { av_log(c->fc, AV_LOG_ERROR, "Failed to add index entry\n"); } + av_log(c->fc, AV_LOG_TRACE, "AVIndex stream %d, sample %u, offset %"PRIx64", dts %"PRId64", " "size %u, distance %d, keyframe %d\n", st->index, sc->sample_count+i, offset, dts, sample_size, distance, keyframe); -- 2.13.2.932.g7449e964c-goog