Testing OpenCV TrackerMIL 'Tracking error' on high resolution #8934
Conversation
archibald1418
requested review from
azhavoro,
bsekachev and
nmanovic
as code owners
|
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 WalkthroughThis pull request introduces several configuration and testing enhancements across multiple files. The changes primarily focus on debugging configurations in VS Code, improving image generation capabilities for Cypress tests, and adding error handling in the OpenCV tracker implementation. The modifications span across development tools, testing utilities, and utility functions, with a particular emphasis on supporting more complex testing scenarios, especially for high-resolution image tracking. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/cypress/plugins/imageGenerator/imageGeneratorCommand.js (1)
18-29: LGTM with minor naming suggestionThe new command follows good patterns from the existing implementation. Consider renaming
posXsandposYstopositionsand accepting an array of{x, y}objects for better maintainability.tests/cypress/e2e/issues_prs2/issue_8894_OpenCV_TrackerMIL_tracking_error_with_big_resolution.js (2)
28-35: Consider adding test cases with varying shape sizes.While the current test case is valid, consider adding variations of shape sizes to ensure the tracking error isn't size-dependent. This would provide better coverage of potential failure scenarios.
45-74: Enhance test assertions and error handling.While the test correctly verifies the absence of tracking errors, consider:
- Adding assertions for shape position/size after tracking
- Implementing error handling for potential OpenCV initialization failures
- Adding timeout handling for high-resolution operations
Example assertion addition:
cy.get(`#cvat_canvas_shape_${shapeNumber}`) .then(() => { cy.get('.cvat-tracking-notice').should('not.exist'); cy.get(`#cvat-objects-sidebar-state-item-${shapeNumber}`) .should('contain', 'RECTANGLE TRACK'); + // Verify shape position after tracking + cy.get(`#cvat_canvas_shape_${shapeNumber}`) + .should('have.attr', 'x') + .and('not.equal', '0'); });.vscode/settings.json (1)
52-58: Remove incomplete comment.The commented-out partial word "eslin" appears to be an incomplete or leftover comment.
"[javascript]": { "editor.maxTokenizationLineLength": 2500, "editor.codeActionsOnSave": { - "source.fixAll.eslint": "always", - // "eslin" + "source.fixAll.eslint": "always" } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
.vscode/launch.json(2 hunks).vscode/settings.json(1 hunks).vscode/tasks.json(1 hunks)cvat-ui/src/utils/opencv-wrapper/tracker-mil.ts(1 hunks)tests/cypress/e2e/issues_prs2/issue_8894_OpenCV_TrackerMIL_tracking_error_with_big_resolution.js(1 hunks)tests/cypress/plugins/imageGenerator/addPlugin.js(2 hunks)tests/cypress/plugins/imageGenerator/imageGeneratorCommand.js(1 hunks)tests/cypress/plugins/index.js(2 hunks)tests/cypress/support/commands_canvas3d.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .vscode/tasks.json
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build
- GitHub Check: testing
- GitHub Check: generate_github_pages
- GitHub Check: Analyze (python)
- GitHub Check: Linter
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
tests/cypress/plugins/index.js (1)
10-10: LGTM!Clean integration of the new task following existing patterns.
Also applies to: 19-19
tests/cypress/support/commands_canvas3d.js (1)
39-42: Verify if debug logging should be permanentThe added logging statements appear to be for debugging purposes. Consider:
- Making them conditional based on a debug flag
- Removing them if they were only meant for temporary debugging
- Adding more context to the log messages if they should be permanent
tests/cypress/e2e/issues_prs2/issue_8894_OpenCV_TrackerMIL_tracking_error_with_big_resolution.js (2)
1-26: LGTM! Test setup is well-structured.The constants and configuration are appropriate for testing high-resolution tracking, with proper image dimensions (5000x5000) and spacing for object positions.
37-43: LGTM! Helper function is well-implemented.The function properly handles OpenCV initialization and ensures loading is complete before proceeding.
tests/cypress/plugins/imageGenerator/addPlugin.js (1)
Line range hint
1-10: LGTM! Export structure is clean.The new export follows the existing pattern and maintains consistency.
.vscode/launch.json (1)
Line range hint
129-158: LGTM! Debug configuration is properly updated.The changes correctly update the Chrome debugger type and add the specific test file pattern for the new test.
| } catch (error) { | ||
| // eslint-disable-next-line | ||
| const test = error; |
There was a problem hiding this comment.
Critical: Improve error handling in tracker initialization
The current implementation silently swallows errors during tracker initialization, which:
- Makes it difficult to diagnose issues
- Could hide critical problems from users
- May complicate debugging of high-resolution tracking errors (issue "Tracking error" in OpenCV TrackerMIL tracker when tracking many concurrently moving objects #8894)
Consider this improvement:
try {
matImage = this.cv.matFromImageData(src);
const rect = new this.cv.Rect(x1, y1, width, height);
this.trackerMIL.init(matImage, rect);
- } catch (error) {
- // eslint-disable-next-line
- const test = error;
+ } catch (error: unknown) {
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error during tracker initialization';
+ throw new Error(`Failed to initialize TrackerMIL: ${errorMessage}`);
} finally {📝 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.
| } catch (error) { | |
| // eslint-disable-next-line | |
| const test = error; | |
| try { | |
| matImage = this.cv.matFromImageData(src); | |
| const rect = new this.cv.Rect(x1, y1, width, height); | |
| this.trackerMIL.init(matImage, rect); | |
| } catch (error: unknown) { | |
| const errorMessage = error instanceof Error ? error.message : 'Unknown error during tracker initialization'; | |
| throw new Error(`Failed to initialize TrackerMIL: ${errorMessage}`); | |
| } finally { |
| // FIXME: try invoking assertions from here | ||
| // function validateArrays(a, b) { | ||
| // assert(Array.isArray(a), `${a} should be an array`); | ||
| // assert(Array.isArray(b), `${b} should be an array`); | ||
|
|
||
| // assert(a.length === b.length, 'arrays should have same length'); | ||
| // } | ||
|
|
||
| // validateArrays(posXs, posYs); |
There was a problem hiding this comment.
Implement the commented array validation logic.
The commented validation code is essential for ensuring proper input handling. Without it, the function might fail silently with mismatched arrays.
Uncomment and implement the validation:
- // FIXME: try invoking assertions from here
- // function validateArrays(a, b) {
- // assert(Array.isArray(a), `${a} should be an array`);
- // assert(Array.isArray(b), `${b} should be an array`);
-
- // assert(a.length === b.length, 'arrays should have same length');
- // }
-
- // validateArrays(posXs, posYs);
+ function validateArrays(a, b) {
+ if (!Array.isArray(a) || !Array.isArray(b)) {
+ throw new Error('Inputs must be arrays');
+ }
+ if (a.length !== b.length) {
+ throw new Error('Arrays must have the same length');
+ }
+ }
+
+ validateArrays(posXs, posYs);📝 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.
| // FIXME: try invoking assertions from here | |
| // function validateArrays(a, b) { | |
| // assert(Array.isArray(a), `${a} should be an array`); | |
| // assert(Array.isArray(b), `${b} should be an array`); | |
| // assert(a.length === b.length, 'arrays should have same length'); | |
| // } | |
| // validateArrays(posXs, posYs); | |
| function validateArrays(a, b) { | |
| if (!Array.isArray(a) || !Array.isArray(b)) { | |
| throw new Error('Inputs must be arrays'); | |
| } | |
| if (a.length !== b.length) { | |
| throw new Error('Arrays must have the same length'); | |
| } | |
| } | |
| validateArrays(posXs, posYs); |
| try { | ||
| const arraysLength = posXs.length; | ||
| for (let i = 1; i <= imagesCount; i++) { | ||
| let image = await createImage(width, height, color); | ||
| for (let j = 0; j < arraysLength; j++) { | ||
| image = await appendText(image, posXs[j], posYs[j], message, j + 1); | ||
| // ws.push(image); | ||
| } | ||
| await image.write(`${file}.${extension}`); // NOTE: where this line should be? | ||
| } | ||
| // eslint-disable-next-line no-empty | ||
| } catch (e) {} |
There was a problem hiding this comment.
Add proper error handling.
The empty catch block silently suppresses errors, which could hide important issues during image generation.
Implement proper error handling:
try {
const arraysLength = posXs.length;
for (let i = 1; i <= imagesCount; i++) {
let image = await createImage(width, height, color);
for (let j = 0; j < arraysLength; j++) {
image = await appendText(image, posXs[j], posYs[j], message, j + 1);
}
await image.write(`${file}.${extension}`);
}
- // eslint-disable-next-line no-empty
- } catch (e) {}
+ } catch (e) {
+ console.error('Failed to generate images:', e);
+ throw e;
+ }📝 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.
| try { | |
| const arraysLength = posXs.length; | |
| for (let i = 1; i <= imagesCount; i++) { | |
| let image = await createImage(width, height, color); | |
| for (let j = 0; j < arraysLength; j++) { | |
| image = await appendText(image, posXs[j], posYs[j], message, j + 1); | |
| // ws.push(image); | |
| } | |
| await image.write(`${file}.${extension}`); // NOTE: where this line should be? | |
| } | |
| // eslint-disable-next-line no-empty | |
| } catch (e) {} | |
| try { | |
| const arraysLength = posXs.length; | |
| for (let i = 1; i <= imagesCount; i++) { | |
| let image = await createImage(width, height, color); | |
| for (let j = 0; j < arraysLength; j++) { | |
| image = await appendText(image, posXs[j], posYs[j], message, j + 1); | |
| // ws.push(image); | |
| } | |
| await image.write(`${file}.${extension}`); // NOTE: where this line should be? | |
| } | |
| } catch (e) { | |
| console.error('Failed to generate images:', e); | |
| throw e; | |
| } |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8934 +/- ##
========================================
Coverage 73.84% 73.84%
========================================
Files 412 412
Lines 44261 44262 +1
Branches 3993 3993
========================================
+ Hits 32684 32687 +3
+ Misses 11577 11575 -2
|
.vscode/tasks.json
Outdated
There was a problem hiding this comment.
Here the pattern didn't work - the vscode task would go into an endless loop
|
|
The test will not be relevant after the fix #8942. But anyway, my general comment is that we do not need two tests doing basically the same thing (but with different resolution). |
This contributes a regression test reproducing a bug from #8894
Motivation and context
A 'Tracking error' message notification popping up when tracking shapes with OpenCV's TrackerMIL.
Its cause was shown to be memory issues when using an OpenCV tracker on frames with high resolution (ca. 5000x5000)
How has this been tested?
The test is marked as skipped for now since the fix is still to be implemented.
WARN: Cypress crashes if this test is enabled (this is expected for now)
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.