fix/looping-gap-issue#271
Merged
Merged
Conversation
Add LastKeyframeGapMs to MP4 state and update fragment-flush logic to consider the previous keyframe gap before forcing a fragment boundary. Previously any unexpectedly short keyframe gap (< MinNormalGOPMs) would force a flush, which could cascade on streams that legitimately emit short GOPs. Now we only force a flush when the current gap is short and the prior gap was healthy (or unset), and we record the current gap for future checks. Also refactor the check to use a local gap variable and preserve the existing log message.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts MP4 fragment-flush behavior to reduce false-positive “loop/restart seam” detections by incorporating the previous keyframe gap into the decision to force a fragment boundary, and adds/updates test coverage for the seam pattern.
Changes:
- Add
LastKeyframeGapMsto MP4 state to track the prior keyframe interval. - Refine seam detection to only force a flush on a sudden short keyframe gap (current gap short + prior gap healthy/unset).
- Update the loop-seam isolation test scenario to reflect a tighter, more realistic seam gap.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
machinery/src/video/mp4.go |
Tracks the previous keyframe gap and gates forced fragment flushes on “sudden” anomalous gaps. |
machinery/src/video/mp4_loopseam_test.go |
Adjusts the simulated seam timing to align with the updated detection threshold/behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+35
to
+40
| // MinNormalGOPMs is the maximum spacing between two consecutive IDRs that | ||
| // we still consider an anomalous "loop/restart seam". When two keyframes | ||
| // arrive closer than this, we treat the second one as an upstream | ||
| // restart/loop-seam and force a fresh fragment so the seam IDR cannot end | ||
| // up as a mid-fragment sync sample. The check only runs when the current | ||
| // fragment has not yet reached FragmentDurationMs. |
Comment on lines
+54
to
+58
| // Seam: IDR arrives ~150ms after previous (vs normal ~1000ms). | ||
| // This matches the realistic virtual-rtsp / ffmpeg `-stream_loop` | ||
| // loop boundary, where the new clip's first IDR is emitted shortly | ||
| // after the previous clip's final IDR. | ||
| pts -= 820 |
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
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.
Add LastKeyframeGapMs to MP4 state and update fragment-flush logic to consider the previous keyframe gap before forcing a fragment boundary. Previously any unexpectedly short keyframe gap (< MinNormalGOPMs) would force a flush, which could cascade on streams that legitimately emit short GOPs. Now we only force a flush when the current gap is short and the prior gap was healthy (or unset), and we record the current gap for future checks. Also refactor the check to use a local gap variable and preserve the existing log message.