Thanks! Updated the patch. Please take a look again. On Sat, Feb 24, 2018 at 7:04 PM, Michael Niedermayer <mich...@niedermayer.cc > 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 (王消寒) <xhw...@chromium.org > > > > wrote: > > > > > jstebbins: kindly ping! > > > > > > On Fri, Feb 16, 2018 at 2:42 PM, Xiaohan Wang (王消寒) < > xhw...@chromium.org> > > > wrote: > > > > > >> +jstebbins@ who wrote that code. > > >> > > >> On Fri, Feb 16, 2018 at 12:30 PM, Michael Niedermayer < > > >> mich...@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 <xhw...@chromium.org> > > >>> > 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 <xhw...@chromium.org> > > 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: 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 51fc5d4e25c128f260ce519ac85dc81313b48459 Mon Sep 17 00:00:00 2001 From: Xiaohan Wang <xhw...@chromium.org> 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 <dalecur...@chromium.org> --- 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; 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.1.291.g4437f3f132-goog
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
