[FFmpeg-devel] [PATCH] mov: fix DTS calculation for samples with negative stts duration

[Michael Niedermayer]

On Thu, May 28, 2015 at 06:49:02PM +0200, Andreas Cadhalpun wrote:
> On 28.05.2015 04:29, Michael Niedermayer wrote:
> > On Thu, May 28, 2015 at 12:11:00AM +0200, Andreas Cadhalpun wrote:
> >> A negative sample duration is invalid according to the spec, but there
> >> are samples that use it for the DTS calculation, e.g.:
> >> http://files.1f0.de/samples/mp4-negative-stts-problem.mp4
> >>
> >> These currently get out of A/V sync.
> >>
> >> Also change the logging type to AV_LOG_WARNING, because decoding the
> >> sample can continue.
> >>
> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> >> ---
> >>  libavformat/mov.c | 20 ++++++++++++++------
> >>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >> index 51cdd21..730f097 100644
> >> --- a/libavformat/mov.c
> >> +++ b/libavformat/mov.c
> >> @@ -2234,12 +2234,6 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>          sample_count=avio_rb32(pb);
> >>          sample_duration = avio_rb32(pb);
> >>  
> >> -        /* sample_duration < 0 is invalid based on the spec */
> >> -        if (sample_duration < 0) {
> >> -            av_log(c->fc, AV_LOG_ERROR, "Invalid SampleDelta %d in STTS, at %d st:%d\n",
> >> -                   sample_duration, i, c->fc->nb_streams-1);
> >> -            sample_duration = 1;
> >> -        }
> >>          if (sample_count < 0) {
> >>              av_log(c->fc, AV_LOG_ERROR, "Invalid sample_count=%d\n", sample_count);
> >>              return AVERROR_INVALIDDATA;
> >> @@ -2439,6 +2433,7 @@ static void mov_build_index(MOVContext *mov, AVStream *st)
> >>          unsigned int distance = 0;
> >>          unsigned int rap_group_index = 0;
> >>          unsigned int rap_group_sample = 0;
> >> +        int64_t dts_correction = 0;
> >>          int rap_group_present = sc->rap_group_count && sc->rap_group;
> >>          int key_off = (sc->keyframe_count && sc->keyframes[0] > 0) || (sc->stps_count && sc->stps_data[0] > 0);
> >>  
> >> @@ -2522,6 +2517,19 @@ static void mov_build_index(MOVContext *mov, AVStream *st)
> >>  
> >>                  current_offset += sample_size;
> >>                  stream_size += sample_size;
> >> +                current_dts += dts_correction;
> > 
> > i think this could depending on the next stts value end up making
> > DTS non monotone which seems wrong/unintended
> 
> Indeed. Fixed patch attached.
> 
> Best regards,
> Andreas
> 

>  mov.c |   29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 24c730d9588822043175600b1bcde8f0fe0719c5  0001-mov-fix-DTS-calculation-for-samples-with-negative-st.patch
> From 1ecae2a3b588c878f91b1c5c1cf93c2d9bc81812 Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> Date: Wed, 27 May 2015 23:57:50 +0200
> Subject: [PATCH] mov: fix DTS calculation for samples with negative stts
>  duration
> 
> A negative sample duration is invalid according to the spec, but there
> are samples that use it for the DTS calculation, e.g.:
> http://files.1f0.de/samples/mp4-negative-stts-problem.mp4
> 
> These currently get out of A/V sync.
> 
> Also change the logging type to AV_LOG_WARNING, because decoding the
> sample can continue.
> 
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> ---
>  libavformat/mov.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 51cdd21..7ce5a86 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -2234,12 +2234,6 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>          sample_count=avio_rb32(pb);
>          sample_duration = avio_rb32(pb);
>  
> -        /* sample_duration < 0 is invalid based on the spec */
> -        if (sample_duration < 0) {
> -            av_log(c->fc, AV_LOG_ERROR, "Invalid SampleDelta %d in STTS, at %d st:%d\n",
> -                   sample_duration, i, c->fc->nb_streams-1);
> -            sample_duration = 1;
> -        }
>          if (sample_count < 0) {
>              av_log(c->fc, AV_LOG_ERROR, "Invalid sample_count=%d\n", sample_count);
>              return AVERROR_INVALIDDATA;
> @@ -2439,10 +2433,13 @@ static void mov_build_index(MOVContext *mov, AVStream *st)
>          unsigned int distance = 0;
>          unsigned int rap_group_index = 0;
>          unsigned int rap_group_sample = 0;
> +        int64_t last_dts = 0;
> +        int64_t dts_correction = 0;
>          int rap_group_present = sc->rap_group_count && sc->rap_group;
>          int key_off = (sc->keyframe_count && sc->keyframes[0] > 0) || (sc->stps_count && sc->stps_data[0] > 0);
>  
>          current_dts -= sc->dts_shift;
> +        last_dts     = current_dts;
>  
>          if (!sc->sample_count || st->nb_index_entries)
>              return;
> @@ -2522,7 +2519,27 @@ static void mov_build_index(MOVContext *mov, AVStream *st)
>  
>                  current_offset += sample_size;
>                  stream_size += sample_size;
> +
> +                /* A negative sample duration is invalid based on the spec,
> +                 * but some samples need it to correct the DTS. */
> +                if (sc->stts_data[stts_index].duration < 0) {
> +                    av_log(mov->fc, AV_LOG_WARNING,
> +                           "Invalid SampleDelta %d in STTS, at %d st:%d\n",
> +                           sc->stts_data[stts_index].duration, stts_index,
> +                           st->index);
> +                    dts_correction += sc->stts_data[stts_index].duration - 1;
> +                    sc->stts_data[stts_index].duration = 1;
> +                }
>                  current_dts += sc->stts_data[stts_index].duration;
> +                if (current_dts + dts_correction > last_dts) {
> +                    current_dts += dts_correction;
> +                    dts_correction = 0;
> +                } else {
> +                    /* Avoid creating non-monotonous DTS */
> +                    dts_correction += current_dts - last_dts - 1;
> +                    current_dts = last_dts + 1;

this would enfore strict monotonicity not just monotoicity
dts[i-1] == dts[i] was previously left as is
is there a reason to change this ?
Ive not checked what the spec says about it but i suspect there are
files which have such dts

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150529/225b7400/attachment.asc>