Incorrectly defined frame number and frame index in GT jobs when startFrame != 0 #8507
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request primarily involve modifications to the Changes
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
bsekachev
changed the title
bsekachev
changed the title
![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 (1)
- cvat-core/src/frames.ts (5 hunks)
🔇 Additional comments (1)
cvat-core/src/frames.ts (1)
187-187: Update toframeIndexcalculation is correctThe adjustment accounts for the
startFrameoffset, ensuring correct alignment with theincludedFramesarray. This change improves the accuracy of frame indexing within the job.
| @@ -713,7 +717,7 @@ export async function getFrame( | |||
| // TODO: migrate to local frame numbers | |||
| const dataStartFrame = getDataStartFrame(meta, startFrame); | |||
| const dataFrameNumberGetter = (frameNumber: number): number => ( | |||
| getDataFrameNumber(frameNumber, dataStartFrame, meta.frameStep) | |||
| getDataFrameNumber(meta, frameNumber, dataStartFrame, meta.frameStep) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider caching getDataFrameNumber computations
The calls to getDataFrameNumber at lines 720 and 817 use the same parameters. Refactoring to compute the data frame number once and reuse it could enhance performance and readability.
Also applies to: 817-817
cvat-core/src/frames.ts
Outdated
| function getDataFrameNumber(meta: FramesMetaData, frameNumber: number, dataStartFrame: number, step: number): number { | ||
| if (meta.includedFrames.length) { | ||
| return meta.includedFrames[frameNumber] + dataStartFrame; | ||
| } | ||
|
|
There was a problem hiding this comment.
Add bounds check for meta.includedFrames access
In getDataFrameNumber, accessing meta.includedFrames[frameNumber] without verifying that frameNumber is within valid bounds may lead to an index out of bounds error. Please add a check to ensure frameNumber is less than meta.includedFrames.length to prevent potential runtime exceptions.
Apply this diff to add bounds checking:
function getDataFrameNumber(meta: FramesMetaData, frameNumber: number, dataStartFrame: number, step: number): number {
if (meta.includedFrames.length) {
+ if (frameNumber >= meta.includedFrames.length || frameNumber < 0) {
+ throw new ArgumentError(`Frame number ${frameNumber} is out of bounds`);
+ }
return meta.includedFrames[frameNumber] + dataStartFrame;
}
return frameNumber * step + dataStartFrame;
}📝 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.
| function getDataFrameNumber(meta: FramesMetaData, frameNumber: number, dataStartFrame: number, step: number): number { | |
| if (meta.includedFrames.length) { | |
| return meta.includedFrames[frameNumber] + dataStartFrame; | |
| } | |
| function getDataFrameNumber(meta: FramesMetaData, frameNumber: number, dataStartFrame: number, step: number): number { | |
| if (meta.includedFrames.length) { | |
| if (frameNumber >= meta.includedFrames.length || frameNumber < 0) { | |
| throw new ArgumentError(`Frame number ${frameNumber} is out of bounds`); | |
| } | |
| return meta.includedFrames[frameNumber] + dataStartFrame; | |
| } | |
bsekachev
changed the base branch from
develop
to
zm/validation-management-core
bsekachev
changed the title
bsekachev
force-pushed
the
bs/fixed_chunk_index_in_gt
branch
from
4ba8608
to
2722a79
Compare
|
Motivation and context
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes