true keyframe cut #1984
true keyframe cut #1984
Conversation
WalkthroughThe recent updates introduce a new option for aligning segment times called "True keyframe cut" across various modules. This involves adding the 'consistent' type to keyframe alignment options, updating default values, and modifying function logic to handle this new alignment type. These changes span the dialogs, ffmpeg utility functions, and segment handling hooks, enhancing the precision of segment alignment in the application. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/renderer/src/dialogs/index.tsx (1 hunks)
- src/renderer/src/ffmpeg.ts (2 hunks)
- src/renderer/src/hooks/useSegments.ts (2 hunks)
Additional comments not posted (4)
src/renderer/src/dialogs/index.tsx (1)
329-331: The addition of the 'consistent' option inaskForAlignSegmentsaligns well with the PR's goal to improve keyframe cut precision. Changing the default to 'consistent' is a significant change that should be highlighted in user documentation or release notes.src/renderer/src/ffmpeg.ts (2)
99-99: The expansion of theFindKeyframeModetype to include 'consistent' supports the new feature introduced in this PR. This change should be well-documented to inform developers of the new functionality available.
112-114: The implementation of the 'consistent' mode infindKeyframecorrectly focuses on finding the next keyframe, which is crucial for the true keyframe cut feature. Ensure that this behavior is covered by unit tests to prevent regressions in future changes.src/renderer/src/hooks/useSegments.ts (1)
9-9: Ensure the newly imported functionsgetStreamFpsandgetDurationare used appropriately within the file.
| @@ -279,18 +279,49 @@ function useSegments({ filePath, workingRef, setWorking, setCutProgress, videoSt | |||
| if (response == null) return; | |||
| setWorking({ text: i18n.t('Aligning segments to keyframes') }); | |||
| const { mode, startOrEnd } = response; | |||
|
|
|||
| if (filePath == null) throw new Error(); | |||
There was a problem hiding this comment.
Explicitly check for filePath being null or undefined at the start of the function to avoid repeated checks and potential runtime errors in deeper nested blocks.
- if (filePath == null) throw new Error();
+ if (!filePath) throw new Error('File path is required.');Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if (filePath == null) throw new Error(); | |
| if (!filePath) throw new Error('File path is required.'); |
|
|
||
| if (filePath == null) throw new Error(); | ||
| const frameTime = 1 / (getStreamFps(videoStream) || 1000); | ||
| const duration = await getDuration(filePath); | ||
|
|
||
| await modifySelectedSegmentTimes(async (segment) => { | ||
| const newSegment = { ...segment }; | ||
|
|
||
| async function align(key) { | ||
| async function align(key: string) { | ||
| const time = newSegment[key]; | ||
| if (filePath == null) throw new Error(); | ||
| const keyframe = await findKeyframeNearTime({ filePath, streamIndex: videoStream.index, time, mode }); | ||
| if (keyframe == null) throw new Error(`Cannot find any keyframe within 60 seconds of frame ${time}`); | ||
| let keyframe = await findKeyframeNearTime({ filePath, streamIndex: videoStream.index, time, mode }); | ||
| if (keyframe == null) { | ||
| if (mode != 'consistent') { | ||
| throw new Error(`Cannot find any keyframe within 60 seconds of frame ${time}`); | ||
| } | ||
| keyframe = duration; | ||
| } | ||
| newSegment[key] = keyframe; | ||
| } | ||
| if (startOrEnd.includes('start')) await align('start'); | ||
| if (startOrEnd.includes('end')) await align('end'); | ||
| if (startOrEnd.includes('start')) { | ||
| if (mode == 'consistent') { | ||
| newSegment.start += frameTime * 0.3; | ||
| } | ||
| await align('start'); | ||
| if (mode == 'consistent') { | ||
| newSegment.start -= frameTime * 0.7; | ||
| } | ||
| } | ||
| if (startOrEnd.includes('end')) { | ||
| await align('end'); | ||
| if (mode == 'consistent' && newSegment.end != duration) { | ||
| newSegment.end -= frameTime * 0.3; | ||
| } | ||
| } | ||
| if (startOrEnd.includes('start')) { | ||
| newSegment.start = Math.min(newSegment.start, newSegment.end - frameTime * 0.99); //don't know how ffmpeg interprets cuts between frames | ||
| } | ||
| else { | ||
| newSegment.end = Math.max(newSegment.start + frameTime * 0.99, newSegment.end); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
There are no unit tests covering the new 'consistent' keyframe alignment mode or the useSegments function. This lack of test coverage could lead to untested edge cases and potential issues in production.
- Ensure that unit tests are added to cover the 'consistent' mode, including edge cases where keyframes might not be found within the expected range.
- Consider adding tests for the
useSegmentsfunction to ensure comprehensive coverage.
Analysis chain
The implementation of the new 'consistent' keyframe alignment mode is complex and involves several conditional checks and adjustments to segment times. Ensure thorough testing, especially edge cases where keyframes might not be found within the expected range.
Would you like assistance in writing additional unit tests for this new feature?
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new 'consistent' mode is covered by unit tests.
# Test: Search for unit tests covering the 'consistent' mode. Expect: Tests that handle various scenarios including edge cases.
rg --type typescript $'describe.*(consistent)'
Length of output: 82
Script:
#!/bin/bash
# Description: Verify the new 'consistent' mode is covered by unit tests.
# Test: Search for unit tests covering the 'consistent' mode. Expect: Tests that handle various scenarios including edge cases.
rg --type ts $'describe.*(consistent)'
Length of output: 39
Script:
#!/bin/bash
# Description: Verify if there are any tests related to the `useSegments` function.
# Test: Search for test cases related to `useSegments`. Expect: Tests that might indirectly cover the 'consistent' mode.
rg --type ts 'useSegments'
Length of output: 880
Script:
#!/bin/bash
# Description: Verify if there are any test files in the repository.
# Test: Search for test files in the repository. Expect: Files that might contain tests for `useSegments`.
fd --type f --extension ts --exec rg 'test|describe|it'
Length of output: 80538
aligns the segments to make keyframe cut more consistent
the segment start will be moved to less than one frame before the first keyframe strictly after the start point, ensuring that the cut will be at the last keyframe at or before the start point.
the endpoint can optionally be aligned to a fraction of a frame before the first keyframe at or after the endpoint. this is useful for splitting a video into segments without repeating or missing any frames.
Summary by CodeRabbit
New Features
Improvements
Enhancements