[PATCH 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(-) 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
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:
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
On 2022-02-27 12:04 pm, "zhilizhao(赵志立)" wrote: 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".
participants (4)
-
"zhilizhao(赵志立)" -
Gyan Doshi -
Paul B Mahol -
Zhao Zhili