image2 frame_pts integer overflow

[Filip]

Summary of the bug:
The image2 muxer has a frame_pts option, which forwards the pts of every frame into the filename. When using microsecond precision (which is the default setting of the drawtext video filter pts template for comparison), the numbers in the filenames of the output images overflow when the timestamps go beyond 35m47s.

These calculated pts numbers are being coerced into a signed int32 variable, used for sequence numbers when frame_pts is off, before being converted into text for the filename. Solutions would be to expand this variable to int64 or to reroute frame_pts to calculate the pts values as doubles and then printf them with a float format.

How to reproduce:
Substitute video.mp4 for any video longer than 35m48s.

% ffmpeg  -ss 35:47  -t 1  -i video.mp4  -copyts  -fps_mode passthrough  -enc_time_base 0.000001  -frame_pts 1  %08d.png

Reproduced on build 2024-09-16-git-76ff97cef5-essentials_build-www.gyan.dev

[Filip]

​https://github.com/FFmpeg/FFmpeg/blob/ceb471cfde901d47e705dd32abba1ea8eab269fa/libavformat/utils.c#L283
I think the best solution is to just change the parameter type of "number", currently int, to int64_t. The function av_get_frame_filename2 is called in many places with an int "number" argument, except for via the frame_pts codepath, where it's a int64_t "number" argument. It seems safer to me to cast int into int64_t rather than the other way round, and I don't foresee any side-effects, though I'm inexperienced with C. Shall I try to submit that as a patch?

This "number" variable just gets passed through into snprintf, with a negative sign if it's <0.

[Filip]

​https://patchwork.ffmpeg.org/project/ffmpeg/patch/20240916182420.188-3-shoutplenty@gmail.com/
Patch is here. One other change that needs to be made is the snprintf format string needs to handle int64_t properly, which I don't know how to do correctly across platforms (the compiler on patchwork is suggesting "%0*ld"). I would also check that the variable length is being passed correctly since I have a vague memory that only single-digit lengths work correctly. Will update if I have any other ideas.

[Filip]

https://ffmpeg.org/pipermail/ffmpeg-devel/2024-September/333602.html
New patch, supersedes the last one. The change is implemented as non–ABI-breaking, with the introduction of av_get_frame_filename3, but it raises the issue of the old implementations kicking around, which should probably be deprecated (with the changes announced in the API changelog).

I decided this is a necessary change to support the frame_pts option properly since the old code had a lossy narrowing type conversion that caused this defect, and making an entirely new codepath would've made the interface of frame_pts (relying on things like a custom parsing routine for the filename template "%d" specifier) incompatible with the rest of img2enc. The new implementation should be totally backwards-compatible but I still can't test it myself since I can't get make to work on WSL lol. Regression tests are passed though: ​https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=12886.

[Filip]

i sent this to ffmpeg-devel but it was rejected probs cos i unsubscribed too fast, so here it is

some feedback from my experience contributing:

i wouldn't have minded to just be told no, you know? or to get help with the things i needed help with, like getting the compiler to include the library i was testing, or how long i was supposed to wait before pinging, or who was meant to take care of my patch. whether/how to make a test for this feature that had never had tests, whether to deprecate the several old apis and redo it properly, or do the non-breaking workaround that was eventually merged. i had my patch reviewed twice and then all those suggested changes and work overridden the third time, my deprecations tossed in favour of technical debt, and this apparently reset the review timeout counter. so it took overall 12 days to merge a very basic bugfix. the maintainer of the part i fixed gave non-answers as to whether it'd be merged and backported ("maybe"), when i could expect the release containing it ("at some point"). i don't know why it wasn't backported, or much else really. i don't know why it was a higher priority to fix fuzzer bugs than stuff that users were affected by.

i'm aware you're aware of these problems, since i read the developer meetings.

"other thing to consider: a patch that no one reviews, is it implicitly accepted after a bunch of pings or not? so far, that seems to be how most developers work"

"Many (most?) patches are unreviewed. We should shepherd patches, especially by new / occasional contributors. Key change needed: avoid leaving patches in limbo or ignored. Proposed process (rough): acknowledging patch(es) on receipt --> identify / designate reviewers --> alert reviewers as required --> ongoing engagement -->"

"It's a self-reinforcing defect: fewer reviews --> fewer new contributors --> fewer reviewers --> fewer reviews"

and it really needn't be said that any gsoc outreach will be undone by potential contributors under 30y/o being unable to work git send-email, or emails at all really, unable to get irc to work, unable to piece together plaintext diffs, or simply emotionally exhausted by feeling like they're burdening people with pings, waiting, forgetting, fretting over the optimal time to ping again. it's that feeling of being a nuisance that means i don't want a reply to this, will unsub and so you can do as you please with this email. my recommendation is that you can solve 2/3 of these problems by switching to gitlab or similar.

thanks for merging my patch, but i do regret asking.

[MasterQuestionable]

͏    Using email to commit doesn't appear mandatory:
͏    ​https://trac.ffmpeg.org/ticket/11190#comment:6

͏    Primary cause is lack of reviewers eligible.
͏    Not something that simply switching to GitLab GitHub whatsoever may solve...

[quinkblack]

The patch has been merged to master branch and cherry-picked to 7.1 release branch. Sorry for the delay. I'm on vacation, and that part of code isn't maintained by me, so I need to wait longer enough. The inefficiency of the development process is a real issue of the project, I don't have an answer for that.

[Filip]

@quinkblack Thanks for backporting the patch; better to wait 1.5 months for the release than 6. And yeah there's not much anyone else can do when the maintainer's response to whether to backport is "maybe".

@MasterQuestionable IMO it is actually that simple, because the thing that stops most interested people in engaging with chores (code review) is the inability to communicate in a linear thread with assignment of reviewers. In my case, not only was it hard to track whether people had replied, but incremental changes I made resulted in new reviewers each time who suggested changes that contradicted the previously suggested ones.

[MasterQuestionable]

͏    The point is probably not communication... but reviewers eligible. (who can direct push)

͏    For the communication part, I think Trac maybe somewhat more fit than GitHub:
͏    ​https://trac.ffmpeg.org/ticket/11055#comment:151
͏    At least no such clipping...
͏    ​https://github.com/MasterInQuestion/TEST/issues/4#issuecomment-1314215587