Stop fabricating a 30 fps fallback in seek()#62
Merged
Conversation
#35 made getFPS() report 0.0 when the container declares no frame rate, rather than silently claiming 30. seek() kept its own fallback and still assumed {30, 1} to compute a target timestamp, so the class gave two different answers to "what is this file's frame rate". Since #51 the fabricated rate could no longer mis-position the reader -- the position-recovery code recomputes the duration from the real rate, finds it unusable and fails -- so the fallback was only ever producing a target timestamp that was then guaranteed to be discarded. Keyframe seeking needs the rate twice: once to turn the requested index into a timestamp, once to turn the landing timestamp back into an index. Neither is possible without a declared rate, so the rate is now selected once, up front, and seek() fails cleanly if there is none. That also removes the duplicated avg/r_frame_rate selection, which had drifted -- the first copy checked num and den, the second only num. No test: constructing a fixture whose container declares no frame rate is awkward, since ffmpeg infers one for essentially every format it writes. The change is exercised indirectly by the existing seek tests, which all take the rate-available path. Fixes #52 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #52.
#35 made
getFPS()report0.0when the container declares no frame rate, instead of silently claiming 30.seek()kept its own fallback and still assumed{30, 1}to compute a target timestamp — so the class gave two different answers to "what is this file's frame rate".Scope
Smaller than it looks. Since #51 the fabricated rate could no longer mis-position the reader: the position-recovery code recomputes the duration from the real rate, finds it unusable, and fails the seek. So the fallback was only ever producing a target timestamp that was then guaranteed to be thrown away. This is a consistency and clarity fix, not a behaviour bug.
Change
Keyframe seeking needs the rate twice — once to turn the requested index into a timestamp, once to turn the landing timestamp back into an index. Neither is possible without a declared rate, so the rate is selected once up front and
seek()fails cleanly if there is none.That also collapses the duplicated
avg_frame_rate/r_frame_rateselection, which had already drifted between its two copies: the first checked bothnumandden, the second onlynum. A stream withnum != 0, den == 0would have divided by zero in one path and not the other.Behaviour change worth noting
seek()on a rate-less file now returns false immediately rather than seeking to a fabricated timestamp and failing later. Both outcomes are a failed seek; the new one is faster and says why.No test
Constructing a fixture whose container declares no frame rate is awkward — ffmpeg infers one for essentially every format it writes, which is why the original #29/#35 work had the same gap. The existing seek tests all take the rate-available path and continue to pass (39 cases, 1118 assertions locally).
If someone wants this covered properly it would need a hand-assembled container or a mocked
AVStream, which is a bigger change than this fix warrants.🤖 Generated with Claude Code