[FFmpeg-devel] Fix ctts_index calculation

Submitted by Xiaohan Wang (王消寒) on Feb. 9, 2018, 9:34 p.m.

Details

Message ID CAF1j9YPigN+Oz6LbmH8MZPSWCTsxmGgk9KtcfS6ordkHp1MxZg@mail.gmail.com
State New
Headers show

Commit Message

Xiaohan Wang (王消寒) Feb. 9, 2018, 9:34 p.m.
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: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > 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
>

Comments

Sasi Inguva Feb. 9, 2018, 10:32 p.m.
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
>
>
Sasi Inguva Feb. 9, 2018, 10:51 p.m.
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
>
>
Michael Niedermayer Feb. 10, 2018, 2:39 a.m.
On Fri, Feb 09, 2018 at 02:51:25PM -0800, Sasi Inguva wrote:
> The new patch looks good to me.

will apply

thanks

[...]

Patch hide | download patch | download mbox

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