true keyframe cut #1984
true keyframe cut #1984
Conversation
WalkthroughThe latest update introduces a new option, "True keyframe cut," for aligning segment times in the video editing dialog. This mode replaces the former default setting "before" with "consistent." Supporting changes were made across different modules, including updating function parameters, handling logic, and error management to integrate this new mode seamlessly. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User Interface
participant Dialog as Align Segments Dialog
participant Hook as useSegments Hook
participant FFMPEG as FFMPEG Module
UI->>Dialog: Choose 'True keyframe cut'
Dialog->>Hook: Trigger segment alignment
Hook->>FFMPEG: getStreamFps(videoStream)
FFMPEG->>Hook: Return fps
Hook->>FFMPEG: getDuration(filePath)
FFMPEG->>Hook: Return duration
Hook->>FFMPEG: findKeyframe(mode: 'consistent')
FFMPEG->>Hook: Return keyframe time
Hook->>Dialog: Update segment times
Dialog->>UI: Display updated segments
Poem
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.
|
|
||
| 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
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)
Files skipped from review due to trivial changes (1)
- src/renderer/src/dialogs/index.tsx
Additional context used
Biome
src/renderer/src/ffmpeg.ts
[error] 591-591: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 592-592: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 651-651: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (7)
src/renderer/src/ffmpeg.ts (5)
109-109: Added 'consistent' mode toFindKeyframeMode.This extension allows the
findKeyframefunction to handle the new 'consistent' mode, aligning with the PR objectives to enhance keyframe cut consistency.
Line range hint
591-591: Consider using an optional chain to simplify the code.
[REFACTOR_SUGGESTion]
This can make the code cleaner and potentially safer by avoiding undefined errors.- const makeTag = format.tags && format.tags['com.apple.quicktime.make']; + const makeTag = format.tags?.['com.apple.quicktime.make'];
Line range hint
592-592: Consider using an optional chain for accessing the model tag.
[REFACTOR_SUGGESTion]
This refactoring enhances code readability and robustness.- const modelTag = format.tags && format.tags['com.apple.quicktime.model']; + const modelTag = format.tags?.['com.apple.quicktime.model'];
Line range hint
651-651: Apply an optional chain for accessing the codec name.
[REFACTOR_SUGGESTion]
Using optional chaining here can prevent potential runtime errors if thestreamobject or itscodec_nameproperty is undefined.- if (stream.codec_type === 'audio') { + if (stream.codec_type === 'audio' && stream?.codec_name) {
122-124: Review the 'consistent' mode logic infindKeyframe.The 'consistent' mode currently mirrors the 'after' mode. If this is intentional, consider adding a comment explaining why this logic is reused. Otherwise, review if additional adjustments are needed for the 'consistent' mode to meet its intended behavior.
src/renderer/src/hooks/useSegments.ts (2)
8-8: Ensure that the newly imported functionsgetStreamFpsandgetDurationare used appropriately throughout the file.
285-286: Calculation offrameTimeand fetchingdurationare critical for the new alignment logic. Ensure that these values are accurately retrieved and used.
| 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.
This block introduces complex logic for aligning segment times based on the new 'consistent' keyframe mode. Consider breaking down this large anonymous function into smaller, named functions to improve readability and maintainability.
+ async function adjustSegmentTime(segment, key, mode, frameTime, duration) {
+ const time = segment[key];
+ if (filePath == null) throw new Error('File path is required to find keyframes.');
+ 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;
+ }
+ segment[key] = keyframe;
+ return segment;
+ }
- await modifySelectedSegmentTimes(async (segment) => {
+ await modifySelectedSegmentTime(segment => adjustSegmentTime(segment, 'start', mode, frameTime, duration));
+ await modifySelectedSegmentTime(segment => adjustSegmentTime(segment, 'end', mode, frameTime, duration));Committable suggestion was skipped due to low confidence.
| @@ -280,18 +280,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.
The error handling should provide more descriptive messages to help with debugging.
- if (filePath == null) throw new Error();
+ if (filePath == null) throw new Error('File path is required for alignment.');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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (filePath == null) throw new Error(); | |
| if (filePath == null) throw new Error('File path is required for alignment.'); |
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.