[FFmpeg-devel,v3,1/4] avformat/movenc: fix assert failure in get_cluster_duration()

When editlist is disabled, the workaournd method of shift dts to
zero and increase the first sample duration doesn't work if the
timestamp is larger than mp4 spec restriction (e.g., sample_delta
in stts entry). Further more, it triggers get_cluster_duration()
assert failure. This patch will drop large offsets between
multiple tracks.
---
 libavformat/movenc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Ping.

> On Dec 31, 2021, at 7:36 PM, Zhao Zhili <quinkblack@foxmail.com> wrote:
> 
> When editlist is disabled, the workaournd method of shift dts to
> zero and increase the first sample duration doesn't work if the
> timestamp is larger than mp4 spec restriction (e.g., sample_delta
> in stts entry). Further more, it triggers get_cluster_duration()
> assert failure. This patch will drop large offsets between
> multiple tracks.
> ---
> libavformat/movenc.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index 0f912dd012..f5bb785b01 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -5917,7 +5917,18 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt)
>          * to signal the difference in starting time without an edit list.
>          * Thus move the timestamp for this first sample to 0, increasing
>          * its duration instead. */
> -        trk->cluster[trk->entry].dts = trk->start_dts = 0;
> +        if (pkt->dts + pkt->duration <= INT32_MAX) {
> +            trk->cluster[trk->entry].dts = trk->start_dts = 0;
> +        } else {
> +            /* Impossible to write a sample duration >= UINT32_MAX.
> +             * Use INT32_MAX as a tight restriction.
> +             */
> +            trk->start_dts = pkt->dts;
> +            av_log(s, AV_LOG_WARNING,
> +                   "Track %d starts with a nonzero dts %" PRId64
> +                   " which will be shifted to zero\n",
> +                   pkt->stream_index, pkt->dts);
> +        }
>     }
>     if (trk->start_dts == AV_NOPTS_VALUE) {
>         trk->start_dts = pkt->dts;
> -- 
> 2.31.1
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

On 2022-02-27 12:04 pm, "zhilizhao(赵志立)" wrote:
> Ping.
>
>> On Dec 31, 2021, at 7:36 PM, Zhao Zhili <quinkblack@foxmail.com> wrote:
>>
>> When editlist is disabled, the workaournd method of shift dts to
>> zero and increase the first sample duration doesn't work if the
>> timestamp is larger than mp4 spec restriction (e.g., sample_delta
>> in stts entry). Further more, it triggers get_cluster_duration()
>> assert failure. This patch will drop large offsets between
>> multiple tracks.
>> ---
>> libavformat/movenc.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>> index 0f912dd012..f5bb785b01 100644
>> --- a/libavformat/movenc.c
>> +++ b/libavformat/movenc.c
>> @@ -5917,7 +5917,18 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt)
>>           * to signal the difference in starting time without an edit list.
>>           * Thus move the timestamp for this first sample to 0, increasing
>>           * its duration instead. */
>> -        trk->cluster[trk->entry].dts = trk->start_dts = 0;
>> +        if (pkt->dts + pkt->duration <= INT32_MAX) {
>> +            trk->cluster[trk->entry].dts = trk->start_dts = 0;
>> +        } else {
>> +            /* Impossible to write a sample duration >= UINT32_MAX.

As per 14496-12 (2005), sample_delta is stored as

unsigned int(32) sample_delta;

The only restriction is that no delta shall be zero except the last 
sample, which means UINT32_MAX  is allowed.
INT32_MAX had been set as max allowed delta in mov.c since inception. 
See my recent changes in mov.c correcting that.

Will this patch break remuxing of files with large deltas?
Let's not clamp if the spec does not call for it.

Regards,
Gyan

On Sun, Feb 27, 2022 at 7:49 AM Gyan Doshi <ffmpeg@gyani.pro> wrote:

>
>
> On 2022-02-27 12:04 pm, "zhilizhao(赵志立)" wrote:
> > Ping.
> >
> >> On Dec 31, 2021, at 7:36 PM, Zhao Zhili <quinkblack@foxmail.com> wrote:
> >>
> >> When editlist is disabled, the workaournd method of shift dts to
> >> zero and increase the first sample duration doesn't work if the
> >> timestamp is larger than mp4 spec restriction (e.g., sample_delta
> >> in stts entry). Further more, it triggers get_cluster_duration()
> >> assert failure. This patch will drop large offsets between
> >> multiple tracks.
> >> ---
> >> libavformat/movenc.c | 13 ++++++++++++-
> >> 1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> >> index 0f912dd012..f5bb785b01 100644
> >> --- a/libavformat/movenc.c
> >> +++ b/libavformat/movenc.c
> >> @@ -5917,7 +5917,18 @@ int ff_mov_write_packet(AVFormatContext *s,
> AVPacket *pkt)
> >>           * to signal the difference in starting time without an edit
> list.
> >>           * Thus move the timestamp for this first sample to 0,
> increasing
> >>           * its duration instead. */
> >> -        trk->cluster[trk->entry].dts = trk->start_dts = 0;
> >> +        if (pkt->dts + pkt->duration <= INT32_MAX) {
> >> +            trk->cluster[trk->entry].dts = trk->start_dts = 0;
> >> +        } else {
> >> +            /* Impossible to write a sample duration >= UINT32_MAX.
>
> As per 14496-12 (2005), sample_delta is stored as
>
> unsigned int(32) sample_delta;
>
> The only restriction is that no delta shall be zero except the last
> sample, which means UINT32_MAX  is allowed.
> INT32_MAX had been set as max allowed delta in mov.c since inception.
> See my recent changes in mov.c correcting that.
>
> Will this patch break remuxing of files with large deltas?
> Let's not clamp if the spec does not call for it.
>

Yes, I agree 100% this is not proper fix for assert.


> Regards,
> Gyan
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>

> On Feb 27, 2022, at 2:49 PM, Gyan Doshi <ffmpeg@gyani.pro> wrote:
> 
> 
> 
> On 2022-02-27 12:04 pm, "zhilizhao(赵志立)" wrote:
>> Ping.
>> 
>>> On Dec 31, 2021, at 7:36 PM, Zhao Zhili <quinkblack@foxmail.com> wrote:
>>> 
>>> When editlist is disabled, the workaournd method of shift dts to
>>> zero and increase the first sample duration doesn't work if the
>>> timestamp is larger than mp4 spec restriction (e.g., sample_delta
>>> in stts entry). Further more, it triggers get_cluster_duration()
>>> assert failure. This patch will drop large offsets between
>>> multiple tracks.
>>> ---
>>> libavformat/movenc.c | 13 ++++++++++++-
>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>> index 0f912dd012..f5bb785b01 100644
>>> --- a/libavformat/movenc.c
>>> +++ b/libavformat/movenc.c
>>> @@ -5917,7 +5917,18 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>          * to signal the difference in starting time without an edit list.
>>>          * Thus move the timestamp for this first sample to 0, increasing
>>>          * its duration instead. */
>>> -        trk->cluster[trk->entry].dts = trk->start_dts = 0;
>>> +        if (pkt->dts + pkt->duration <= INT32_MAX) {
>>> +            trk->cluster[trk->entry].dts = trk->start_dts = 0;
>>> +        } else {
>>> +            /* Impossible to write a sample duration >= UINT32_MAX.
> 
> As per 14496-12 (2005), sample_delta is stored as
> 
> unsigned int(32) sample_delta;
> 
> The only restriction is that no delta shall be zero except the last sample, which means UINT32_MAX  is allowed.
> INT32_MAX had been set as max allowed delta in mov.c since inception. See my recent changes in mov.c correcting that.
> 
> Will this patch break remuxing of files with large deltas?
> Let's not clamp if the spec does not call for it.

Since get_cluster_duration() checks dts against INT_MAX, remove
the clamp at here can still trigger abort() after.

```
    av_assert0(next_dts <= INT_MAX);

```

The prototype of get_cluster_duration() which returns int needs
to be fixed, and a bunch of places.

And then we can get regression issues: movenc.c output files with
delta larger than INT32_MAX can be dangerous, which is not the
case of mov.c demuxer.

It’s a clever trick here, by using a large sample duration to
handle start time difference between multiple tracks. Since it’s
not a standard method like editlist, I think we can turn off the
strategy safely (FFmpeg abort before the patch, so it’s already
turned off in a hard way).

What do you think?

> 
> Regards,
> Gyan
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

> On Mar 1, 2022, at 9:32 PM, zhilizhao(赵志立) <quinkblack@foxmail.com> wrote:
> 
> 
>> On Feb 27, 2022, at 2:49 PM, Gyan Doshi <ffmpeg@gyani.pro> wrote:
>> 
>> 
>> 
>> On 2022-02-27 12:04 pm, "zhilizhao(赵志立)" wrote:
>>> Ping.
>>> 
>>>> On Dec 31, 2021, at 7:36 PM, Zhao Zhili <quinkblack@foxmail.com> wrote:
>>>> 
>>>> When editlist is disabled, the workaournd method of shift dts to
>>>> zero and increase the first sample duration doesn't work if the
>>>> timestamp is larger than mp4 spec restriction (e.g., sample_delta
>>>> in stts entry). Further more, it triggers get_cluster_duration()
>>>> assert failure. This patch will drop large offsets between
>>>> multiple tracks.
>>>> ---
>>>> libavformat/movenc.c | 13 ++++++++++++-
>>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>>> index 0f912dd012..f5bb785b01 100644
>>>> --- a/libavformat/movenc.c
>>>> +++ b/libavformat/movenc.c
>>>> @@ -5917,7 +5917,18 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>>         * to signal the difference in starting time without an edit list.
>>>>         * Thus move the timestamp for this first sample to 0, increasing
>>>>         * its duration instead. */
>>>> -        trk->cluster[trk->entry].dts = trk->start_dts = 0;
>>>> +        if (pkt->dts + pkt->duration <= INT32_MAX) {
>>>> +            trk->cluster[trk->entry].dts = trk->start_dts = 0;
>>>> +        } else {
>>>> +            /* Impossible to write a sample duration >= UINT32_MAX.
>> 
>> As per 14496-12 (2005), sample_delta is stored as
>> 
>> unsigned int(32) sample_delta;
>> 
>> The only restriction is that no delta shall be zero except the last sample, which means UINT32_MAX  is allowed.
>> INT32_MAX had been set as max allowed delta in mov.c since inception. See my recent changes in mov.c correcting that.
>> 
>> Will this patch break remuxing of files with large deltas?
>> Let's not clamp if the spec does not call for it.
> 
> Since get_cluster_duration() checks dts against INT_MAX, remove
> the clamp at here can still trigger abort() after.
> 
> ```
>    av_assert0(next_dts <= INT_MAX);
> 
> ```
> 
> The prototype of get_cluster_duration() which returns int needs
> to be fixed, and a bunch of places.
> 
> And then we can get regression issues: movenc.c output files with
> delta larger than INT32_MAX can be dangerous, which is not the
> case of mov.c demuxer.
> 
> It’s a clever trick here, by using a large sample duration to
> handle start time difference between multiple tracks. Since it’s
> not a standard method like editlist, I think we can turn off the
> strategy safely (FFmpeg abort before the patch, so it’s already
> turned off in a hard way).
> 
> What do you think?

Ping. Another idea is return error instead of abort().

> 
>> 
>> Regards,
>> Gyan
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>