* [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