| Message ID | CAF1j9YPigN+Oz6LbmH8MZPSWCTsxmGgk9KtcfS6ordkHp1MxZg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Feb 9, 2018 at 1:34 PM, Xiaohan Wang (王消寒) <xhwang@chromium.org> wrote: > I uploaded a new patch. Is this what you meant? > > I am confused with the code though. In the case *ctts_index == ctts_count, > The problem is when *ctts_index == (ctts_count -1) . if ( (ctts_count- 1) < ctts_count) { while ( ctts_sample < ctts_data[ctts_count - 1].count ) { increment(ctts_sample ) } } But if you check for (ctts_count - 1) < (ctts_count -1) , the above iterations won't take place. > this while loop will do nothing. Is this what we want? Also, the "wrong" > |ctts_index| will be available outside of this function. How do we make > sure it won't cause trouble later? > > On Fri, Feb 9, 2018 at 12:26 PM, Sasi Inguva <isasi-at-google.com@ffmpeg. > org > > wrote: > > > > > > > diff --git a/libavformat/mov.c b/libavformat/mov.c > > > index 5adba52e08..f0bd3e3623 100644 > > > --- a/libavformat/mov.c > > > +++ b/libavformat/mov.c > > > @@ -3148,7 +3148,7 @@ static int find_prev_closest_index(AVStream *st, > > > *ctts_index = 0; > > > *ctts_sample = 0; > > > for (index_ctts_count = 0; index_ctts_count < *index; > > > index_ctts_count++) { > > > - if (*ctts_index < ctts_count) { > > > > + if (*ctts_index < ctts_count - 1) { > > > > > If you do this, then we are skipping, looking at ctts_samples in the last > > ctts entry where (*ctts_index == ctts_count -1), which is not desired. > > > > (*ctts_sample)++; > > > if (ctts_data[*ctts_index].count == *ctts_sample) { > > > (*ctts_index)++; > > > > > > > Here *ctts_index == ctts_count only iff the *ctts_index is the last > CTTS > > entry, and we still didn't reach *index. That means that it's file with > > CTTS truncated i.e. no corresponding CTTS entries for some STTS entries. > > This issue can be fixed by checking for *ctts_index < ctts_count, in > the > > line after this loop. > > while (*index > > <https://cs.corp.google.com/piper///depot/google3/third_ > > party/ffmpeg/next/libavformat/mov.c?l=3123&ct=xref_jump_to_ > > def&gsn=index&rcl=184932376> > > >= 0 && (*ctts_index > > <https://cs.corp.google.com/piper///depot/google3/third_ > > party/ffmpeg/next/libavformat/mov.c?l=3124&ct=xref_jump_to_ > > def&gsn=ctts_index&rcl=184932376>) > > >= 0) { > > we already check for (*ctts_index) >=0 so we can check for the upper > bound > > too. > > > > On Sat, Feb 3, 2018 at 12:36 PM, Michael Niedermayer > > <michael@niedermayer.cc > > > wrote: > > > > > On Fri, Feb 02, 2018 at 05:55:38PM -0800, Xiaohan Wang (王消寒) wrote: > > > > > > > > > > > mov.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > 87157b4053de0e044e78db72ef13e8058075c235 > > 0001-Fix-ctts_index-calculatio > > > n.patch > > > > From bb376fd2de5da5f9ecdef41621a579252b899d7d Mon Sep 17 00:00:00 > 2001 > > > > From: Xiaohan Wang <xhwang@chromium.org> > > > > Date: Fri, 2 Feb 2018 17:33:56 -0800 > > > > Subject: [PATCH] Fix ctts_index calculation > > > > > > > > An index should never be equal to the count. Hence we must make sure > > > > *ctts_index < ctts_count. > > > > --- > > > > libavformat/mov.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > should be reviewed by sasi idealy, he wrote this > > > > > > thx > > > > > > > > > [...] > > > > > > -- > > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC7 > 87040B0FAB > > > > > > Democracy is the form of government in which you can choose your > dictator > > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
The new patch looks good to me. On Fri, Feb 9, 2018 at 1:34 PM, Xiaohan Wang (王消寒) <xhwang@chromium.org> wrote: > I uploaded a new patch. Is this what you meant? > > I am confused with the code though. In the case *ctts_index == ctts_count, > this while loop will do nothing. Is this what we want? Also, the "wrong" > |ctts_index| will be available outside of this function. How do we make > sure it won't cause trouble later? > > On Fri, Feb 9, 2018 at 12:26 PM, Sasi Inguva <isasi-at-google.com@ffmpeg. > org > > wrote: > > > > > > > diff --git a/libavformat/mov.c b/libavformat/mov.c > > > index 5adba52e08..f0bd3e3623 100644 > > > --- a/libavformat/mov.c > > > +++ b/libavformat/mov.c > > > @@ -3148,7 +3148,7 @@ static int find_prev_closest_index(AVStream *st, > > > *ctts_index = 0; > > > *ctts_sample = 0; > > > for (index_ctts_count = 0; index_ctts_count < *index; > > > index_ctts_count++) { > > > - if (*ctts_index < ctts_count) { > > > > + if (*ctts_index < ctts_count - 1) { > > > > > If you do this, then we are skipping, looking at ctts_samples in the last > > ctts entry where (*ctts_index == ctts_count -1), which is not desired. > > > > (*ctts_sample)++; > > > if (ctts_data[*ctts_index].count == *ctts_sample) { > > > (*ctts_index)++; > > > > > > > Here *ctts_index == ctts_count only iff the *ctts_index is the last > CTTS > > entry, and we still didn't reach *index. That means that it's file with > > CTTS truncated i.e. no corresponding CTTS entries for some STTS entries. > > This issue can be fixed by checking for *ctts_index < ctts_count, in > the > > line after this loop. > > while (*index > > <https://cs.corp.google.com/piper///depot/google3/third_ > > party/ffmpeg/next/libavformat/mov.c?l=3123&ct=xref_jump_to_ > > def&gsn=index&rcl=184932376> > > >= 0 && (*ctts_index > > <https://cs.corp.google.com/piper///depot/google3/third_ > > party/ffmpeg/next/libavformat/mov.c?l=3124&ct=xref_jump_to_ > > def&gsn=ctts_index&rcl=184932376>) > > >= 0) { > > we already check for (*ctts_index) >=0 so we can check for the upper > bound > > too. > > > > On Sat, Feb 3, 2018 at 12:36 PM, Michael Niedermayer > > <michael@niedermayer.cc > > > wrote: > > > > > On Fri, Feb 02, 2018 at 05:55:38PM -0800, Xiaohan Wang (王消寒) wrote: > > > > > > > > > > > mov.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > 87157b4053de0e044e78db72ef13e8058075c235 > > 0001-Fix-ctts_index-calculatio > > > n.patch > > > > From bb376fd2de5da5f9ecdef41621a579252b899d7d Mon Sep 17 00:00:00 > 2001 > > > > From: Xiaohan Wang <xhwang@chromium.org> > > > > Date: Fri, 2 Feb 2018 17:33:56 -0800 > > > > Subject: [PATCH] Fix ctts_index calculation > > > > > > > > An index should never be equal to the count. Hence we must make sure > > > > *ctts_index < ctts_count. > > > > --- > > > > libavformat/mov.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > should be reviewed by sasi idealy, he wrote this > > > > > > thx > > > > > > > > > [...] > > > > > > -- > > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC7 > 87040B0FAB > > > > > > Democracy is the form of government in which you can choose your > dictator > > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
On Fri, Feb 09, 2018 at 02:51:25PM -0800, Sasi Inguva wrote:
> The new patch looks good to me.
will apply
thanks
[...]
From cedc04aec7703ce63a7e2b78ee394fcb0999b016 Mon Sep 17 00:00:00 2001 From: Xiaohan Wang <xhwang@chromium.org> Date: Fri, 2 Feb 2018 17:33:56 -0800 Subject: [PATCH] Fix ctts_index calculation An index should never be equal to the count. Hence we must make sure *ctts_index < ctts_count. --- libavformat/mov.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavformat/mov.c b/libavformat/mov.c index 5adba52e08..c7f70f141e 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -3157,7 +3157,7 @@ static int find_prev_closest_index(AVStream *st, } } - while (*index >= 0 && (*ctts_index) >= 0) { + while (*index >= 0 && (*ctts_index) >= 0 && (*ctts_index) < ctts_count) { // Find a "key frame" with PTS <= timestamp_pts (So that we can decode B-frames correctly). // No need to add dts_shift to the timestamp here becase timestamp_pts has already been // compensated by dts_shift above. -- 2.16.0.rc1.238.g530d649a79-goog