Re: [FFmpeg-devel] [PATCH] avformat/movenc: Check that cts fits in 32bit

[James Almer <jamrial@gmail.com>]

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".