* [FFmpeg-devel] [PATCH] avformat/movenc: Check that cts fits in 32bit @ 2024-03-31 11:40 Michael Niedermayer 2024-03-31 16:30 ` James Almer 0 siblings, 1 reply; 5+ messages in thread From: Michael Niedermayer @ 2024-03-31 11:40 UTC (permalink / raw) To: FFmpeg development discussions and patches Fixes: Assertion av_rescale_rnd(start_dts, mov->movie_timescale, track->timescale, AV_ROUND_DOWN) <= 0 failed at libavformat/movenc.c:3694 Fixes: poc2 Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/movenc.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/libavformat/movenc.c b/libavformat/movenc.c index ae94d8d5959..5617a2620c5 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -6194,6 +6194,12 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt) if (ret < 0) return ret; + if (pkt->pts != AV_NOPTS_VALUE && + (uint64_t)pkt->dts - pkt->pts != (int32_t)((uint64_t)pkt->dts - pkt->pts)) { + av_log(s, AV_LOG_WARNING, "pts/dts pair unsupported\n"); + return AVERROR_PATCHWELCOME; + } + if (mov->flags & FF_MOV_FLAG_FRAGMENT || mov->mode == MODE_AVIF) { int ret; if (mov->moov_written || mov->flags & FF_MOV_FLAG_EMPTY_MOOV) { -- 2.17.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] 5+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat/movenc: Check that cts fits in 32bit 2024-03-31 11:40 [FFmpeg-devel] [PATCH] avformat/movenc: Check that cts fits in 32bit Michael Niedermayer @ 2024-03-31 16:30 ` James Almer 2024-04-01 15:38 ` Michael Niedermayer 0 siblings, 1 reply; 5+ messages in thread From: James Almer @ 2024-03-31 16:30 UTC (permalink / raw) To: ffmpeg-devel On 3/31/2024 8:40 AM, Michael Niedermayer wrote: > Fixes: Assertion av_rescale_rnd(start_dts, mov->movie_timescale, track->timescale, AV_ROUND_DOWN) <= 0 failed at libavformat/movenc.c:3694 > Fixes: poc2 > > Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/movenc.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c > index ae94d8d5959..5617a2620c5 100644 > --- a/libavformat/movenc.c > +++ b/libavformat/movenc.c > @@ -6194,6 +6194,12 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt) > if (ret < 0) > return ret; > > + if (pkt->pts != AV_NOPTS_VALUE && > + (uint64_t)pkt->dts - pkt->pts != (int32_t)((uint64_t)pkt->dts - pkt->pts)) { > + av_log(s, AV_LOG_WARNING, "pts/dts pair unsupported\n"); > + return AVERROR_PATCHWELCOME; > + } Any such check should happen in check_pkt(), called directly above. And afaict there's no reason to not support 64bit cts. Even in mov_write_edts_tag() we check for it and write a version 1 of the box that supports 64bit values. Maybe the problem is that MOVIentry.cts is an int, when it should be an int64_t like start_cts? Can you test the following? > diff --git a/libavformat/movenc.h b/libavformat/movenc.h > index 08d580594d..e736c92750 100644 > --- a/libavformat/movenc.h > +++ b/libavformat/movenc.h > @@ -49,11 +49,11 @@ typedef struct MOVIentry { > uint64_t pos; > int64_t dts; > int64_t pts; > + int64_t cts; > unsigned int size; > unsigned int samples_in_chunk; > unsigned int chunkNum; ///< Chunk number if the current entry is a chunk start otherwise 0 > unsigned int entries; > - int cts; > #define MOV_SYNC_SAMPLE 0x0001 > #define MOV_PARTIAL_SYNC_SAMPLE 0x0002 > #define MOV_DISPOSABLE_SAMPLE 0x0004 _______________________________________________ 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] 5+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat/movenc: Check that cts fits in 32bit 2024-03-31 16:30 ` James Almer @ 2024-04-01 15:38 ` Michael Niedermayer 2024-04-01 15:45 ` James Almer 0 siblings, 1 reply; 5+ messages in thread From: Michael Niedermayer @ 2024-04-01 15:38 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1.1: Type: text/plain, Size: 2306 bytes --] On Sun, Mar 31, 2024 at 01:30:26PM -0300, James Almer wrote: > On 3/31/2024 8:40 AM, Michael Niedermayer wrote: > > Fixes: Assertion av_rescale_rnd(start_dts, mov->movie_timescale, track->timescale, AV_ROUND_DOWN) <= 0 failed at libavformat/movenc.c:3694 > > Fixes: poc2 > > > > Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/movenc.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c > > index ae94d8d5959..5617a2620c5 100644 > > --- a/libavformat/movenc.c > > +++ b/libavformat/movenc.c > > @@ -6194,6 +6194,12 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt) > > if (ret < 0) > > return ret; > > + if (pkt->pts != AV_NOPTS_VALUE && > > + (uint64_t)pkt->dts - pkt->pts != (int32_t)((uint64_t)pkt->dts - pkt->pts)) { > > + av_log(s, AV_LOG_WARNING, "pts/dts pair unsupported\n"); > > + return AVERROR_PATCHWELCOME; > > + } > > Any such check should happen in check_pkt(), called directly above. And > afaict there's no reason to not support 64bit cts. Even in > mov_write_edts_tag() we check for it and write a version 1 of the box that > supports 64bit values. > > Maybe the problem is that MOVIentry.cts is an int, when it should be an > int64_t like start_cts? Can you test the following? changing cts to 64bit does avoid the assert with the test sample but If you chaneg cts to 64bit consider cts is assigned to MOVCtts duration (32bit) in mov_write_ctts_tag() and also compared to it. its also written with avio_wb32() later theres also avio_wb32(pb, track->cluster[i].cts); in mov_write_trun_tag() so the suggestion would avoid the assert but the code is not correct, now one can maybe add support for this by switching to 64bit variants. But this needs to be backported too. and "64bit cts support" seems not ideal both because of complexity and beacause its not a bugfix I would prefer to keep a simple (easy backportable) bugfix for the releases. thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB What does censorship reveal? It reveals fear. -- Julian Assange [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] [-- Attachment #2: Type: text/plain, Size: 251 bytes --] _______________________________________________ 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] 5+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat/movenc: Check that cts fits in 32bit 2024-04-01 15:38 ` Michael Niedermayer @ 2024-04-01 15:45 ` James Almer 2024-04-01 16:53 ` Michael Niedermayer 0 siblings, 1 reply; 5+ messages in thread From: James Almer @ 2024-04-01 15:45 UTC (permalink / raw) To: ffmpeg-devel On 4/1/2024 12:38 PM, Michael Niedermayer wrote: > On Sun, Mar 31, 2024 at 01:30:26PM -0300, James Almer wrote: >> On 3/31/2024 8:40 AM, Michael Niedermayer wrote: >>> Fixes: Assertion av_rescale_rnd(start_dts, mov->movie_timescale, track->timescale, AV_ROUND_DOWN) <= 0 failed at libavformat/movenc.c:3694 >>> Fixes: poc2 >>> >>> Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavformat/movenc.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c >>> index ae94d8d5959..5617a2620c5 100644 >>> --- a/libavformat/movenc.c >>> +++ b/libavformat/movenc.c >>> @@ -6194,6 +6194,12 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt) >>> if (ret < 0) >>> return ret; >>> + if (pkt->pts != AV_NOPTS_VALUE && >>> + (uint64_t)pkt->dts - pkt->pts != (int32_t)((uint64_t)pkt->dts - pkt->pts)) { >>> + av_log(s, AV_LOG_WARNING, "pts/dts pair unsupported\n"); >>> + return AVERROR_PATCHWELCOME; >>> + } >> >> Any such check should happen in check_pkt(), called directly above. And >> afaict there's no reason to not support 64bit cts. Even in >> mov_write_edts_tag() we check for it and write a version 1 of the box that >> supports 64bit values. >> >> Maybe the problem is that MOVIentry.cts is an int, when it should be an >> int64_t like start_cts? Can you test the following? > > changing cts to 64bit does avoid the assert with the test sample but > > If you chaneg cts to 64bit consider cts is assigned to MOVCtts duration (32bit) > in mov_write_ctts_tag() and also compared to it. > its also written with avio_wb32() later > > theres also > avio_wb32(pb, track->cluster[i].cts); > in mov_write_trun_tag() You're right, there's no 64bit version for these, i guess they will not define one either in a future revision of the spec. So your patch should be fine. > > so the suggestion would avoid the assert but the code is not correct, now one can > maybe add support for this by switching to 64bit variants. But this needs to be > backported too. > and "64bit cts support" seems not ideal both because of complexity and beacause > its not a bugfix > > I would prefer to keep a simple (easy backportable) bugfix for the releases. > > thx > > [...] > > > _______________________________________________ > 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] 5+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat/movenc: Check that cts fits in 32bit 2024-04-01 15:45 ` James Almer @ 2024-04-01 16:53 ` Michael Niedermayer 0 siblings, 0 replies; 5+ messages in thread From: Michael Niedermayer @ 2024-04-01 16:53 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1.1: Type: text/plain, Size: 2458 bytes --] On Mon, Apr 01, 2024 at 12:45:36PM -0300, James Almer wrote: > On 4/1/2024 12:38 PM, Michael Niedermayer wrote: > > On Sun, Mar 31, 2024 at 01:30:26PM -0300, James Almer wrote: > > > On 3/31/2024 8:40 AM, Michael Niedermayer wrote: > > > > Fixes: Assertion av_rescale_rnd(start_dts, mov->movie_timescale, track->timescale, AV_ROUND_DOWN) <= 0 failed at libavformat/movenc.c:3694 > > > > Fixes: poc2 > > > > > > > > Found-by: Wang Dawei and Zhou Geng, from Zhongguancun Laboratory > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavformat/movenc.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c > > > > index ae94d8d5959..5617a2620c5 100644 > > > > --- a/libavformat/movenc.c > > > > +++ b/libavformat/movenc.c > > > > @@ -6194,6 +6194,12 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt) > > > > if (ret < 0) > > > > return ret; > > > > + if (pkt->pts != AV_NOPTS_VALUE && > > > > + (uint64_t)pkt->dts - pkt->pts != (int32_t)((uint64_t)pkt->dts - pkt->pts)) { > > > > + av_log(s, AV_LOG_WARNING, "pts/dts pair unsupported\n"); > > > > + return AVERROR_PATCHWELCOME; > > > > + } > > > > > > Any such check should happen in check_pkt(), called directly above. And > > > afaict there's no reason to not support 64bit cts. Even in > > > mov_write_edts_tag() we check for it and write a version 1 of the box that > > > supports 64bit values. > > > > > > Maybe the problem is that MOVIentry.cts is an int, when it should be an > > > int64_t like start_cts? Can you test the following? > > > > changing cts to 64bit does avoid the assert with the test sample but > > > > If you chaneg cts to 64bit consider cts is assigned to MOVCtts duration (32bit) > > in mov_write_ctts_tag() and also compared to it. > > its also written with avio_wb32() later > > > > theres also > > avio_wb32(pb, track->cluster[i].cts); > > in mov_write_trun_tag() > > You're right, there's no 64bit version for these, i guess they will not > define one either in a future revision of the spec. So your patch should be > fine. ok, will apply thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB While the State exists there can be no freedom; when there is freedom there will be no State. -- Vladimir Lenin [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] [-- Attachment #2: Type: text/plain, Size: 251 bytes --] _______________________________________________ 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] 5+ messages in thread
end of thread, other threads:[~2024-04-01 16:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-31 11:40 [FFmpeg-devel] [PATCH] avformat/movenc: Check that cts fits in 32bit Michael Niedermayer 2024-03-31 16:30 ` James Almer 2024-04-01 15:38 ` Michael Niedermayer 2024-04-01 15:45 ` James Almer 2024-04-01 16:53 ` Michael Niedermayer
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