I do see some similarities in the way how the frame rate is accessed in the rBf2b24272dd7c and in this patch, but there are still enough of open questions:

• The description of a fix in the rBf2b24272dd7c mentions that the issue is caused by an integer overflow, and does not go into details of why v_st->r_frame_rate got replaced with av_guess_frame_rate. This lack of information raises a question: what is the proper source of truth for the frame rate. Should it be r_frame_rate or the av_guess_frame_rate.
• I do not see how one can have proper review of a fix which does not even mention how to reproduce the issue.

Please pay more attention to presentation of your work so others can understand why things got changed, as well as make others possible to reproduce the issues which are being solved.

Not to mention that in an ideal world we'll need to have those things covered with regression tests, so next time the area is changed we know that we do not break duration detection in some existing files.

In D15839#430589, @Sergey Sharybin (sergey) wrote:

I do see some similarities in the way how the frame rate is accessed in the rBf2b24272dd7c and in this patch, but there are still enough of open questions:

• The description of a fix in the rBf2b24272dd7c mentions that the issue is caused by an integer overflow, and does not go into details of why v_st->r_frame_rate got replaced with av_guess_frame_rate. This lack of information raises a question: what is the proper source of truth for the frame rate. Should it be r_frame_rate or the av_guess_frame_rate.
• I do not see how one can have proper review of a fix which does not even mention how to reproduce the issue.

I have created bug report T100771 for this, so you can reproduce.
As for rBf2b24272dd7c, first I have noticed, that last working version is 2.92. Lot of ffmpeg changes have been backported to 2.93, but this commit is not listed. I did not test if this commit did cause the issue, but given how it is fixed, I assumed it must have been the cause. But will have to double check.

Looking at report linked to commit, I think I have mixed in completely different fix for incorrect FPS with provided file and did not mention it in commit message. Now I wonder myself why I did that. I try to break up commits usually. Perhaps I investigated incorrect fps issue and forgot about it. But in any case will try to avoid such situation in future.
av_guess_frame_rate is currently providing best results, and this function should be used normally. Neither source gives good value all the time unfortunately.

Not to mention that in an ideal world we'll need to have those things covered with regression tests, so next time the area is changed we know that we do not break duration detection in some existing files.

I can prepare tests for strip length when using timecodes, but some type of timecodes I have never found practical use-case for and not sure if they even influence strip length. But I think it could be determined from code itself.
Also a note, that me and @Sebastian Parborg (zeddb) have conspired to remove timecodes completely once we believe our seeking is working perfectly in practice. But so far we did not propose such change in official capacity.

Thanks for the test file.
Is it expected that the duration changes from 150 frames to 152?

In D15839#430659, @Sergey Sharybin (sergey) wrote:

Thanks for the test file.
Is it expected that the duration changes from 150 frames to 152?

Well according to ffprobe, file has 153 frames. Here, using Record Run timecode will change length to 151 and Record Run No Gaps will change length to 153. This is likely correct.

Not sure if I have answer to what should be the correct number for "normal" case

• Overall stream length is 5.1s
• Overall average FPS is 30.61
• Audio stream length is 5.013s
• Video stream length is 4.998s
• Video stream FPS is 29.97
• Video stream starts a 0.052s later than audio

Here we use video stream length and fps, because it is available. This gives 150 frames, because there is ugly (int)((duration * fps) + 0.5f) formula for that. In result, video is 5s long exactly.
There is one more frame available past that point, but there is no sound to be played. 2 other "missing" frames I would expect to find somewhere in the middle with odd time spacing. To seek to last frame overall FPS would have to be used.

My judgement on whether this is good or bad is, that it's acceptable. This is because FPS is returned by av_guess_frame_rate. If video stream was used, file from T93328 would have incorrect FPS instead. So even if I consider both files somehow bad, currently both can be used with user likely not noticing any issue. Usually files have same FPS value in all fields. Especially from devices that produce files for further editing.

Now back to timecodes:
Record Run No Gaps timecode ignores timecodes and serves frames as they are encoded, so this is correct.
Recorrd Run timecode stores frame number in its entries which is calculated from frame PTS and FPS. It causes strip to have length of 151, because it includes this "normally" unreachable frame too. Therefore this is technically working as expected.

I have also checked if this issue is caused by rBf2b24272dd7c, and it is not which is strange. It must be very old issue then, just nobody has noticed. Will update description. I don't think I will bisect exact cause unless you want me to do it. I thought I have broken this with VFR patch originally, so I wanted mainly to know what have I done wrong and reference it in commit. But this issue seems to be older than my work on ffmpeg stuff.