From patchwork Tue Feb 27 21:11:56 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?WGlhb2hhbiBXYW5nICjnjovmtojlr5Ip?= X-Patchwork-Id: 7760 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.2.181.170 with SMTP id m39csp3005728jaj; Tue, 27 Feb 2018 13:12:27 -0800 (PST) X-Google-Smtp-Source: AG47ELsnwpHzmHdu+TIFHf/iwL2nUGROhfXX7cpryeogrDM+jB7dsEP9cD+myd4+kGGriB6YJ/W2 X-Received: by 10.28.17.77 with SMTP id 74mr4789572wmr.67.1519765947461; Tue, 27 Feb 2018 13:12:27 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1519765947; cv=none; d=google.com; s=arc-20160816; b=zK2L2gljoWAJkYvDYgolgwuSUsxnJKBUrqe3PYG+RU6qpSorW6iV/h8RiY4WlubG99 c+mGn/sWR/PYe0NkkKSKGaHaOVSNqvDIVWFlzj1ukS1k/tcT1BZLv+D/q6EWpGfpE0g0 T3LTQaQJT2lJY0A5f2At3FKo39hIcL7tQ/XanpTliuKiRws6X0u9DkuBpy0iXR5hrI+F mYmUsTdbau6CzpeMxtBCXv5DdNeMyyIT7Fpt/vksC+u0sICEhlSxcq8V9QBIEeCjgoVE ntU8sauim3u0S4XywpdZ5bZCcYpPoI3vzVXXf6rPzo37tQovnpBCeG2JRdTH5LRP62rm VyaQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to: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=eGjPgNeUp/DjdfuLYYcNZ1g3LR+ymCcOrDHfgsYzhyU=; b=j5zzbQ8jhorpDt7ZanmuL93c6qWwYiOawR5OdGomVoCbrF/6IrSeyU2b2HbhZ+wWw7 4E9aU1pi+oozfZL4wjd0yQ69ikumBkm40lWXYHe8/Bdy+rMghH5fWCTu/6OuoToPdHK9 gOfys1CJhserbZ1+TN1NaaTqJxWZ8ooFz1GM8HgwSYsHn1Q9+cBe1mqq5eGyG+MxwwPk UxObScnIg7v5AWhhgdnzD6fPsE93UakvCHiGgaO1MVqdCTZa1nevxDXi0uyDOjn270p/ REDKlyt1WeZ85/tmUbGB4hgq6F07xaf6B83ASiwUIy1XxTNeGRdkV56eT9gI0dCvcFXQ L6Vw== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@google.com header.s=20161025 header.b=dQPqxUY4; dkim=neutral (body hash did not verify) header.i=@chromium.org header.s=google header.b=EYYcPqIN; 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 m202si203644wmb.170.2018.02.27.13.12.26; Tue, 27 Feb 2018 13:12:27 -0800 (PST) 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.s=20161025 header.b=dQPqxUY4; dkim=neutral (body hash did not verify) header.i=@chromium.org header.s=google header.b=EYYcPqIN; 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 5D84868A27D; Tue, 27 Feb 2018 23:12:20 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-yb0-f196.google.com (mail-yb0-f196.google.com [209.85.213.196]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 4939968A13F for ; Tue, 27 Feb 2018 23:12:14 +0200 (EET) Received: by mail-yb0-f196.google.com with SMTP id b12-v6so89839ybn.8 for ; Tue, 27 Feb 2018 13:12:19 -0800 (PST) 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; bh=uDL1dktQTdekSH8vnLmubYctTfjqMOkyPg5fCWE865M=; b=dQPqxUY46ccHSA9fiu4qR/vqLaTLm9EwO84IPpxP/XNBmgYMbAbGdO5f4DxWAmvE5D 64Qye5cAEBpYxgErODZ2PpsxyNJh8oUabeV/gOd4YQgFFKaNNRSsMmEYdBxTajY7rB+I lBV3hE2tl+4dDnqZP8jBEqm5zSCSqszrw++yWnqBiF150cspAkyO2ddS5yaWrGWuinbW rTRAyNeW0VJqti1kKHjLmT21xhVwnVN6Z9hk9FreSM8BuxJWGyEK4aZVLAKRb7rlNzxW QPKy865+FWtGIYR6JKpvxd5x3FwQVGecFpi7F4vP8kkN8ZJXLHs34GK09jKTLHcht1p8 +tzQ== 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; bh=uDL1dktQTdekSH8vnLmubYctTfjqMOkyPg5fCWE865M=; b=EYYcPqINg1fZM2JZKeRzPcMZP94qjs5an2/2mB83s/OgMTXhtlHloZHH27npK7ZhUS 6Qes3uq3+RetqJPuMD7IhI9J6Y9yCuV8nKTvlu5deWgssJXWAzbGL2RAfPMR9GLE+UA0 ATog4XPQlrgwvhzqAGmNGwAeuMt4VmkbKn5nU= 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; bh=uDL1dktQTdekSH8vnLmubYctTfjqMOkyPg5fCWE865M=; b=Nk+Wuv1Y+JJI1gFbweztZQ/F0wdZd6TWNTWjtW5V9A801yYYoGm1Y7YL5lMNRzfGvr tQdubRy+rWIi9LIafqWO3Szo17H6kTwnxdPdmL2LRH8+4z1IGBRsCwfNa5LrDkfny6MW KPwt4lk3CCgwYcDkz6fZraQbTX5tE4GyHglElE29bZ/NVE+TqKsKPBIGATQSjfC6CpFp ASyO7cJSDePNuZ/btrbY8OtVCTQNiglarBrJZbFJ0MDa3CaN13VjiF0dopGs97xA3+YL Aw63GOG72/dzwzgNS+Nx52d8WaSMTRfm2+CMbRgfjC0Ldy2Ntnsze+1oY1CjCGyL26Mf scNw== X-Gm-Message-State: APf1xPB/R0943L00ffEu/ymmFjLWtGMyKYHVlxD874vc2WDUOym0Jyax v2k1rqfpla7b93yCRWqlcsPhWXU38Bg8Vqu2yEWAsHIQ X-Received: by 2002:a25:848c:: with SMTP id v12-v6mr10410954ybk.111.1519765937122; Tue, 27 Feb 2018 13:12:17 -0800 (PST) MIME-Version: 1.0 Received: by 2002:a25:a022:0:0:0:0:0 with HTTP; Tue, 27 Feb 2018 13:11:56 -0800 (PST) In-Reply-To: <20180227103858.GK23018@michaelspb> References: <20180216203011.GM23018@michaelspb> <20180225030435.GE23018@michaelspb> <20180227103858.GK23018@michaelspb> From: =?UTF-8?B?WGlhb2hhbiBXYW5nICjnjovmtojlr5Ip?= Date: Tue, 27 Feb 2018 13:11:56 -0800 X-Google-Sender-Auth: p38iM5tr9XOSzh729W8gHPVY-j4 Message-ID: To: FFmpeg development discussions and patches X-Content-Filtered-By: Mailman/MimeDel 2.1.20 Subject: Re: [FFmpeg-devel] Fix memset size on ctts_data in mov_read_trun() 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 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Sure. Updated! On Tue, Feb 27, 2018 at 2:38 AM, Michael Niedermayer wrote: > On Mon, Feb 26, 2018 at 10:37:51AM -0800, Xiaohan Wang (王消寒) wrote: > > Thanks! Updated the patch. Please take a look again. > > > > On Sat, Feb 24, 2018 at 7:04 PM, Michael Niedermayer > > > wrote: > > > > > On Fri, Feb 23, 2018 at 05:12:05PM -0800, Xiaohan Wang (王消寒) wrote: > > > > Michael: Dale and I dig into history a bit more and we don't > understand > > > why > > > > the code is changed to the current state around memset. This new > patch > > > > reverted the change back to the previous state which we felt is much > > > > cleaner. Please see the CL description for details and take a look > at the > > > > new patch. Thanks! > > > > > > > > On Wed, Feb 21, 2018 at 1:14 PM, Xiaohan Wang (王消寒) < > xhwang@chromium.org > > > > > > > > wrote: > > > > > > > > > jstebbins: kindly ping! > > > > > > > > > > On Fri, Feb 16, 2018 at 2:42 PM, Xiaohan Wang (王消寒) < > > > xhwang@chromium.org> > > > > > wrote: > > > > > > > > > >> +jstebbins@ who wrote that code. > > > > >> > > > > >> On Fri, Feb 16, 2018 at 12:30 PM, Michael Niedermayer < > > > > >> michael@niedermayer.cc> wrote: > > > > >> > > > > >>> On Thu, Feb 15, 2018 at 12:10:33PM -0800, Xiaohan Wang (王消寒) > wrote: > > > > >>> > > > > > >>> > > > > >>> > mov.c | 3 ++- > > > > >>> > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > >>> > 5597d0b095f8b15eb11503010a51c2bc2c022413 > > > > >>> 0001-ffmpeg-Fix-memset-size-on-ctts_data-in-mov_read_trun.patch > > > > >>> > From 7c1e6b50ebe35b2a38c4f1d0a988e31eccbd0ead Mon Sep 17 > 00:00:00 > > > 2001 > > > > >>> > From: Xiaohan Wang > > > > >>> > Date: Thu, 15 Feb 2018 12:05:53 -0800 > > > > >>> > Subject: [PATCH] ffmpeg: Fix memset size on ctts_data in > > > > >>> mov_read_trun() > > > > >>> > > > > > >>> > The allocated size of sc->ctts_data is > > > > >>> > (st->nb_index_entries + entries) * sizeof(*sc->ctts_data). > > > > >>> > > > > > >>> > The size to memset at offset sc->ctts_data + sc->ctts_count > should > > > be > > > > >>> > (st->nb_index_entries + entries - sc->ctts_count) * > > > > >>> sizeof(*sc->ctts_data)) > > > > >>> > > > > > >>> > The current code missed |entries| I believe. > > > > >>> > > > > >>> shouldnt "entries" be read by this function later and so shouldnt > > > need a > > > > >>> memset? > > > > >>> I didnt write this, but it looks a bit to me as if it was > intended to > > > > >>> only > > > > >>> clear the area that would not be read later > > > > >>> > > > > >> > > > > >> I thought we only had sc->ctts_count entries before > av_fast_realloc, > > > so > > > > >> memset everything starting from sc->ctts_data + sc->ctts_count > > > couldn't > > > > >> go wrong. But I am not familiar with this code and that could > totally > > > be > > > > >> wrong. I added jstebbins@ who wrote the code and hopefully we > can get > > > > >> expert opinion there. > > > > >> > > > > >> > > > > >>> [...] > > > > >>> -- > > > > >>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC7 > > > 87040B0FAB > > > > >>> > > > > >>> No great genius has ever existed without some touch of madness. > -- > > > > >>> Aristotle > > > > >>> > > > > >>> _______________________________________________ > > > > >>> ffmpeg-devel mailing list > > > > >>> ffmpeg-devel@ffmpeg.org > > > > >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > >>> > > > > >>> > > > > >> > > > > > > > > > > > > mov.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > e5bbe55f0b1260f787f431b5c45e6ca49a7d2d1e > 0001-Fix-memset-size-on-ctts_ > > > data-in-mov_read_trun-round-.patch > > > > From f34b35ec5749c17b21f80665a0b8859bce3e84ab Mon Sep 17 00:00:00 > 2001 > > > > From: Xiaohan Wang > > > > Date: Fri, 23 Feb 2018 10:51:30 -0800 > > > > Subject: [PATCH] Fix memset size on ctts_data in mov_read_trun() > (round > > > 2) > > > > > > > > The allocated size of sc->ctts_data is > > > > (st->nb_index_entries + entries) * sizeof(*sc->ctts_data). > > > > > > > > The size to memset at offset sc->ctts_data + sc->ctts_count should be > > > > (st->nb_index_entries + entries - sc->ctts_count) * > > > > sizeof(*sc->ctts_data)) > > > > > > > > The current code missed |entries| I believe, which was introduced in > > > > https://patchwork.ffmpeg.org/patch/5541/. > > > > > > > > However, after offline discussion, it seems the original code is much > > > > more clear to read (before https://patchwork.ffmpeg.org/patch/5541/ > ). > > > > > > > > Hence this CL revert the memset logic to it's previous state by > > > > remembering the |old_ctts_allocated_size|, and only memset the newly > > > > allocated entries. > > > > --- > > > > libavformat/mov.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/libavformat/mov.c b/libavformat/mov.c > > > > index a3725692a7..f8d79c7b02 100644 > > > > --- a/libavformat/mov.c > > > > +++ b/libavformat/mov.c > > > > @@ -4713,6 +4713,7 @@ static int mov_read_trun(MOVContext *c, > > > AVIOContext *pb, MOVAtom atom) > > > > st->index_entries= new_entries; > > > > > > > > requested_size = (st->nb_index_entries + entries) * > > > sizeof(*sc->ctts_data); > > > > + unsigned int old_ctts_allocated_size = sc->ctts_allocated_size; > > > > > > this should possibly be size_t > > > > > > and declaration and statements should not be mixed > > > > > > libavformat/mov.c: In function ‘mov_read_trun’: > > > libavformat/mov.c:4691:5: warning: ISO C90 forbids mixed declarations > and > > > code [-Wdeclaration-after-statement] > > > > > > [...] > > > > > > -- > > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC7 > 87040B0FAB > > > > > > If you fake or manipulate statistics in a paper in physics you will > never > > > get a job again. > > > If you fake or manipulate statistics in a paper in medicin you will get > > > a job for life at the pharma industry. > > > > > > _______________________________________________ > > > ffmpeg-devel mailing list > > > ffmpeg-devel@ffmpeg.org > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > > > > > mov.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > b6b9921ba048a72f86e49a5cbb14238ce5137ebb 0001-ffmpeg-Fix-memset-size- > on-ctts_data-in-mov_read_trun.patch > > From 51fc5d4e25c128f260ce519ac85dc81313b48459 Mon Sep 17 00:00:00 2001 > > From: Xiaohan Wang > > Date: Fri, 23 Feb 2018 17:04:41 -0800 > > Subject: [PATCH] ffmpeg: Fix memset size on ctts_data in mov_read_trun() > > (round 2) > > > > The allocated size of sc->ctts_data is > > (st->nb_index_entries + entries) * sizeof(*sc->ctts_data). > > > > The size to memset at offset sc->ctts_data + sc->ctts_count should be > > (st->nb_index_entries + entries - sc->ctts_count) * > > sizeof(*sc->ctts_data)) > > > > The current code missed |entries| I believe, which was introduced in > > https://patchwork.ffmpeg.org/patch/5541/. > > > > However, after offline discussion, it seems the original code is much > > more clear to read (before https://patchwork.ffmpeg.org/patch/5541/). > > > > Hence this CL revert the memset logic to it's previous state by > > remembering the |old_ctts_allocated_size|, and only memset the newly > > allocated entries. > > > > BUG=812567 > > > > Change-Id: Ibe94c7138e5818bfaae76866bfa6619a9b8a2b6b > > Reviewed-on: https://chromium-review.googlesource.com/934925 > > Reviewed-by: Dale Curtis > > --- > > libavformat/mov.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/libavformat/mov.c b/libavformat/mov.c > > index a3725692a7..c93ad7de67 100644 > > --- a/libavformat/mov.c > > +++ b/libavformat/mov.c > > @@ -4621,6 +4621,7 @@ static int mov_read_trun(MOVContext *c, > AVIOContext *pb, MOVAtom atom) > > int64_t prev_dts = AV_NOPTS_VALUE; > > int next_frag_index = -1, index_entry_pos; > > size_t requested_size; > > + size_t old_ctts_allocated_size = 0; > > the assignment of 0 seems redundant > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > If you fake or manipulate statistics in a paper in physics you will never > get a job again. > If you fake or manipulate statistics in a paper in medicin you will get > a job for life at the pharma industry. > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > From 74b725f0c4f79d83d4ea4498896fcc6c96b97c30 Mon Sep 17 00:00:00 2001 From: Xiaohan Wang Date: Fri, 23 Feb 2018 17:04:41 -0800 Subject: [PATCH] ffmpeg: Fix memset size on ctts_data in mov_read_trun() (round 2) The allocated size of sc->ctts_data is (st->nb_index_entries + entries) * sizeof(*sc->ctts_data). The size to memset at offset sc->ctts_data + sc->ctts_count should be (st->nb_index_entries + entries - sc->ctts_count) * sizeof(*sc->ctts_data)) The current code missed |entries| I believe, which was introduced in https://patchwork.ffmpeg.org/patch/5541/. However, after offline discussion, it seems the original code is much more clear to read (before https://patchwork.ffmpeg.org/patch/5541/). Hence this CL revert the memset logic to it's previous state by remembering the |old_ctts_allocated_size|, and only memset the newly allocated entries. BUG=812567 Change-Id: Ibe94c7138e5818bfaae76866bfa6619a9b8a2b6b Reviewed-on: https://chromium-review.googlesource.com/934925 Reviewed-by: Dale Curtis --- libavformat/mov.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/libavformat/mov.c b/libavformat/mov.c index a3725692a7..556d42bb4a 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -4621,6 +4621,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) int64_t prev_dts = AV_NOPTS_VALUE; int next_frag_index = -1, index_entry_pos; size_t requested_size; + size_t old_ctts_allocated_size; AVIndexEntry *new_entries; MOVFragmentStreamInfo * frag_stream_info; @@ -4713,6 +4714,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) st->index_entries= new_entries; requested_size = (st->nb_index_entries + entries) * sizeof(*sc->ctts_data); + old_ctts_allocated_size = sc->ctts_allocated_size; ctts_data = av_fast_realloc(sc->ctts_data, &sc->ctts_allocated_size, requested_size); if (!ctts_data) @@ -4722,8 +4724,8 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) // In case there were samples without ctts entries, ensure they get // zero valued entries. This ensures clips which mix boxes with and // without ctts entries don't pickup uninitialized data. - memset(sc->ctts_data + sc->ctts_count, 0, - (st->nb_index_entries - sc->ctts_count) * sizeof(*sc->ctts_data)); + memset((uint8_t*)(sc->ctts_data) + old_ctts_allocated_size, 0, + sc->ctts_allocated_size - old_ctts_allocated_size); if (index_entry_pos < st->nb_index_entries) { // Make hole in index_entries and ctts_data for new samples -- 2.16.2.395.g2e18187dfd-goog