* [FFmpeg-devel] [PATCH v3 1/4] avformat/movenc: fix assert failure in get_cluster_duration()
@ 2021-12-31 11:36 Zhao Zhili
2022-02-27 6:34 ` "zhilizhao(赵志立)"
0 siblings, 1 reply; 6+ messages in thread
From: Zhao Zhili @ 2021-12-31 11:36 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Zhao Zhili
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH v3 1/4] avformat/movenc: fix assert failure in get_cluster_duration()
2021-12-31 11:36 [FFmpeg-devel] [PATCH v3 1/4] avformat/movenc: fix assert failure in get_cluster_duration() Zhao Zhili
@ 2022-02-27 6:34 ` "zhilizhao(赵志立)"
2022-02-27 6:49 ` Gyan Doshi
0 siblings, 1 reply; 6+ messages in thread
From: "zhilizhao(赵志立)" @ 2022-02-27 6:34 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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".
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH v3 1/4] avformat/movenc: fix assert failure in get_cluster_duration()
2022-02-27 6:34 ` "zhilizhao(赵志立)"
@ 2022-02-27 6:49 ` Gyan Doshi
2022-02-27 12:42 ` Paul B Mahol
2022-03-01 13:32 ` "zhilizhao(赵志立)"
0 siblings, 2 replies; 6+ messages in thread
From: Gyan Doshi @ 2022-02-27 6:49 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH v3 1/4] avformat/movenc: fix assert failure in get_cluster_duration()
2022-02-27 6:49 ` Gyan Doshi
@ 2022-02-27 12:42 ` Paul B Mahol
2022-03-01 13:32 ` "zhilizhao(赵志立)"
1 sibling, 0 replies; 6+ messages in thread
From: Paul B Mahol @ 2022-02-27 12:42 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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".
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH v3 1/4] avformat/movenc: fix assert failure in get_cluster_duration()
2022-02-27 6:49 ` Gyan Doshi
2022-02-27 12:42 ` Paul B Mahol
@ 2022-03-01 13:32 ` "zhilizhao(赵志立)"
2022-03-29 4:59 ` "zhilizhao(赵志立)"
1 sibling, 1 reply; 6+ messages in thread
From: "zhilizhao(赵志立)" @ 2022-03-01 13:32 UTC (permalink / raw)
To: FFmpeg development discussions and patches
> 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".
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH v3 1/4] avformat/movenc: fix assert failure in get_cluster_duration()
2022-03-01 13:32 ` "zhilizhao(赵志立)"
@ 2022-03-29 4:59 ` "zhilizhao(赵志立)"
0 siblings, 0 replies; 6+ messages in thread
From: "zhilizhao(赵志立)" @ 2022-03-29 4:59 UTC (permalink / raw)
To: FFmpeg development discussions and patches
> 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".
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-29 4:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-31 11:36 [FFmpeg-devel] [PATCH v3 1/4] avformat/movenc: fix assert failure in get_cluster_duration() Zhao Zhili
2022-02-27 6:34 ` "zhilizhao(赵志立)"
2022-02-27 6:49 ` Gyan Doshi
2022-02-27 12:42 ` Paul B Mahol
2022-03-01 13:32 ` "zhilizhao(赵志立)"
2022-03-29 4:59 ` "zhilizhao(赵志立)"
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git