feat: add Landing AI integration for cloud auto-labeling#129
feat: add Landing AI integration for cloud auto-labeling#129vusallyv wants to merge 13 commits intodeveloper0hye:masterfrom
Conversation
e466750 to
ba12606
Compare
developer0hye
left a comment
There was a problem hiding this comment.
Review Summary
Thanks for adding Landing AI support! However, this PR has a critical structural issue that needs to be resolved before it can be merged.
Root Cause: Branch not rebased on merged #128
This branch was forked from feat/cloud-auto-label before PR #128 went through its 4 review rounds. The first commit (514ac52) contains the pre-review version of #128's code, re-introducing bugs that were already fixed during #128's review process. Since #128 is now merged into master, this branch needs to be rebased on current master so only the Landing AI delta remains.
Once rebased, the three BLOCKER regressions below will disappear automatically.
BLOCKER — #128 review regressions (will be fixed by rebase)
| # | Issue | Detail |
|---|---|---|
| 1 | Session restore deleted | saveSession() / restoreLastSession() are removed entirely. App restart loses previous session. (PR #127 feature, fixed in #128 round 3) |
| 2 | Progress display 0-based | set_label_progress changed from fileIndex+1 / size to fileIndex / size-1 — shows "0 / 4" instead of "1 / 5". (Fixed in #128 round 3) |
| 3 | Homebrew ONNX include path removed | YoloLabel.pro drops include/onnxruntime subdirectory support — breaks macOS Homebrew builds. (Fixed in #128 round 3) |
CRITICAL — Landing AI code issues
4. #include <QDir> removed from mainwindow.cpp
The PR removes #include <QDir> but QDir::currentPath() is still used in on_loadModel_clicked(). This relies on transitive includes and is fragile.
5. No structural validation of Landing AI response
dataVal.toArray().first().toArray()— no check thatfirst()is actually an array- No range check on normalized coordinates (cx, cy, nw, nh could be negative or > 1)
- No validation that
bounding_boxvalues are numeric
HIGH
6. No Cancel button for Landing AI batch
landingAIAutoLabelAll() never calls m_btnCancelAutoLabel->setVisible(true). Users cannot cancel a Landing AI batch operation. Compare with cloudAutoLabelAll() which shows it.
7. No upload consent for Landing AI
The yololabel.com path has a one-time privacy notice (checkUploadConsent()). Landing AI uploads images to api.va.landing.ai without any consent prompt. Users should be informed before their images are sent to a third-party service.
8. No backup before overwrite
CloudAutoLabeler calls backupLabelFile() before writing labels. doLandingAIJob() directly opens the file with WriteOnly and overwrites without creating a .bak backup.
9. No retry logic
doLandingAIJob() shows a QMessageBox and clears the queue on the first network failure. CloudAutoLabeler retries up to 3 times (MAX_RETRIES).
10. MIME type only covers PNG/JPEG
doLandingAIJob() only checks .png and defaults to image/jpeg. CloudAutoLabeler::mimeForImage() also handles BMP, TIFF, and WebP. Consider reusing that helper.
11. No saveState() call
Landing AI single-image labeling doesn't call saveState(), so Ctrl+Z (undo) won't work after a cloud label. cloudAutoLabelAll() calls save_label_data() — Landing AI should match.
MEDIUM
12. Dead member m_landingPendingImagePath — Set in doLandingAIJob() but never read anywhere.
13. Shared prompt semantics — Both providers share m_cloudPrompt, but yololabel.com sends it as a single semicolon-separated string while Landing AI splits it into separate prompts multipart fields. Switching providers with the same prompt could produce unexpected results.
14. Progress off-by-one — In landingAIProcessNextInQueue(), done = m_imgList.size() - m_landingQueue.size() counts the current (in-flight) image as "done" before it finishes.
Architecture suggestion
The Landing AI logic is implemented directly in MainWindow (~200 lines of networking code), which is the same pattern that was refactored out of MainWindow into CloudAutoLabeler during #128's first review round. Consider either:
- Extending
CloudAutoLabelerwith a provider abstraction (enum or strategy) so both yololabel.com and Landing AI share the same cancel/retry/backup/consent infrastructure, or - Creating a separate
LandingAILabelerclass following the same QObject + signals pattern asCloudAutoLabeler
Either approach would eliminate the duplicate button-management code in MainWindow and automatically give Landing AI the same robustness (retry, cancel, backup, consent) that CloudAutoLabeler already has.
Recommended next steps
- Rebase on current
master— this resolves all 3 BLOCKERs - Fix the CRITICAL and HIGH issues listed above
- Consider the architecture suggestion to reduce duplication
Happy to re-review once rebased!
Extends the AI Settings tab with a model selector (YoloLabel AI / Landing AI) so users can choose between the yololabel.com API and the Landing AI text-to-object-detection endpoint. Key changes: - QComboBox model selector added to AI Settings tab - syncAiSettingsTab() / saveAiSettings() are model-aware - submitLandingAIJob() / doLandingAIJob(): POST to Landing AI endpoint with image + prompt; parse bbox response and write YOLO .txt files - QImageReader used to get image dimensions for coordinate normalization (falls back to QImage if size() returns invalid) - Full JSON + bounds validation on Landing AI responses - landingAIAutoLabelAll() / landingAIProcessNextInQueue() for batch flow - m_landingNet: dedicated QNetworkAccessManager for Landing AI requests - Saves/restores landingApiKey under "YoloLabel"/"CloudAI" QSettings key Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
Rebased onto current master (resolves the 3 BLOCKERs: session restore, progress display, and Homebrew ONNX path — all fixed in the upstream merge of PR developer0hye#128). CRITICAL: - QDir include already present after rebase (no change needed) - Validate that data[0] is an array before indexing (firstVal.isArray()) - Check bounding_box values are numeric (isDouble()) - Skip boxes with coords outside [0,1] or non-positive size HIGH: - Add checkLandingConsent() — one-time privacy notice for Landing AI (separate from checkUploadConsent() used by yololabel.com) - Show m_btnCancelAutoLabel in both submitLandingAIJob() and landingAIAutoLabelAll(); cancelAutoLabel() now also sets m_landingCancelled=true and clears m_landingQueue - Call CloudAutoLabeler::backupLabelFile() before overwriting label file - Add retry logic: up to MAX_LANDING_RETRIES=3 on network failure - Use CloudAutoLabeler::mimeForImage() for PNG/BMP/TIFF/WebP/JPEG - Call save_label_data() and ui->label_image->saveState() in submitLandingAIJob() so Ctrl+Z undo works after single-image labeling MEDIUM: - Remove dead member m_landingPendingImagePath (set but never read) - Fix progress off-by-one: show done-1/total while image is in-flight Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
ba12606 to
9767268
Compare
Both static helpers are called from mainwindow.cpp but were declared private, causing C2248 access errors on MSVC and equivalent errors on GCC/Clang. Move them to the public section. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
developer0hye
left a comment
There was a problem hiding this comment.
Review Summary
Great work addressing all the issues from the previous review! The rebase is clean, JSON validation is solid, and the consent/retry/backup/undo flows are all in place. Two remaining issues need fixing before merge.
HIGH — Must fix before merge
1. Image slider not disabled during Landing AI batch
cloudAutoLabelAll() disables the slider via CloudAutoLabeler::busyChanged:
connect(m_cloudLabeler, &CloudAutoLabeler::busyChanged, this, [this](bool busy) {
ui->horizontalSlider_images->setEnabled(!busy);
});Landing AI bypasses CloudAutoLabeler, so the slider (and A/D key navigation) remains active during batch processing. This is the same race condition fixed in PR #128 round 2 ("Batch + A/D navigation race"), now reintroduced for the Landing AI path.
Fix: Disable the slider in submitLandingAIJob() and landingAIAutoLabelAll(), re-enable in resetCloudButtons() (or wherever the Landing AI flow completes).
2. File write failure silently ignored
In doLandingAIJob():
if (lf.open(QIODevice::WriteOnly | QIODevice::Text)) {
// ... write labels ...
lf.close();
}
// if open() fails → no error, silent continueThis was flagged as CRITICAL in PR #128 round 4 ("File write failure silent success") and fixed in CloudAutoLabeler. The Landing AI path doesn't have the same fix.
Fix: Show an error (e.g., QMessageBox::warning or statusBar()->showMessage) when the file can't be opened for writing.
MEDIUM — Strongly recommended
3. Label matching is case-sensitive and exact
int classId = m_objList.indexOf(label);
if (classId < 0) continue; // silently skippedIf Landing AI returns "Person" but the class file has "person", all detections are silently dropped. The user sees "0 detections" with no explanation.
Suggestion: Use case-insensitive matching, or at minimum log skipped labels to the status bar (e.g., "Landing AI: 3 detections skipped (unknown labels: Person, Car)").
4. No upload size warning for Landing AI batch
cloudAutoLabelAll() warns when estimated upload exceeds 500 MB. landingAIAutoLabelAll() doesn't have this check. Since Landing AI processes one image at a time (vs. batches of 20), the bandwidth impact is actually higher for large datasets.
LOW / Nit
5. Magic number 1 for Landing AI combo index — Used in 4+ places (currentIndex() == 1). An enum or named constant would improve readability.
6. QNetworkAccessManager transitive include — mainwindow.h declares QNetworkAccessManager *m_landingNet but relies on the include from cloud_labeler.h. A forward declaration would be safer.
7. setBody() vs setBodyDevice() — PR #128 used setBodyDevice() to halve peak memory. Landing AI uses f.readAll() + setBody(). Minor since it's one image at a time, but worth aligning for consistency.
What looks good
- Clean rebase — no conflicts with current master
QImageReader.size()for dimensions without decoding pixels- Full JSON validation (
firstVal.isArray(),isDouble(), coordinate range checks) - Separate consent dialog for Landing AI privacy notice
- 3-retry logic matching
CloudAutoLabeler::MAX_RETRIES backupLabelFile()before overwritingsave_label_data()+saveState()for Ctrl+Z undo support- Progress counter correctly accounts for in-flight image (
done = total - remaining - 1)
Recommended next steps
… round 2) Fix the two HIGH issues flagged in the second review round: 1. Disable image slider during Landing AI operations to prevent the A/D navigation race condition that was already fixed for the yololabel.com path via CloudAutoLabeler::busyChanged. Slider is disabled in submitLandingAIJob() and landingAIAutoLabelAll(), and re-enabled in resetCloudButtons(). 2. Handle file write failure in doLandingAIJob(): when lf.open() fails, show a status bar message instead of silently continuing. Also address MEDIUM developer0hye#3: when Landing AI returns labels not found in the class list, collect the unknown label names and report them to the status bar (e.g. "detections skipped (unknown labels: Person, Car)") so users understand why detections were dropped. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
developer0hye
left a comment
There was a problem hiding this comment.
Strict Review of PR #129 — Landing AI Integration
CI passes (all 3 platforms + DCO). The rebase is clean and the round 2 fixes look good. However, comparing this PR with the robustness standards established in PR #128 reveals several remaining issues.
CRITICAL — Must fix before merge
1. A/D keyboard navigation race condition during Landing AI operations
next_img() and prev_img() only guard against CloudAutoLabeler:
void MainWindow::next_img(bool bSavePrev)
{
if (m_cloudLabeler && m_cloudLabeler->isBusy()) return; // ← Landing AI not checked
if(bSavePrev && ui->label_image->isOpened()) save_label_data();
goto_img(m_imgIndex + 1);
}During a Landing AI batch, m_cloudLabeler->isBusy() is false. The slider is correctly disabled, but A/D keyboard shortcuts bypass the slider and call next_img()/prev_img() directly. This means:
- User presses A/D during batch →
save_label_data()fires for the current on-screen image - The Landing AI callback is simultaneously writing labels for a different queued image
- If the user navigates to an image that's currently being processed, both paths can write to the same
.txtfile
This is the exact same race condition fixed in PR #128 round 2 ("Batch + A/D navigation race"). The slider disable covers part of it, but the keyboard shortcut path needs a Landing AI busy guard.
Fix: Add a check for Landing AI busy state in next_img()/prev_img():
void MainWindow::next_img(bool bSavePrev)
{
if (m_cloudLabeler && m_cloudLabeler->isBusy()) return;
if (!m_landingQueue.isEmpty() || m_landingCancelled == false /* during in-flight job */) return;
...
}Or better: introduce a single m_landingBusy bool that is set/cleared alongside button state.
2. No stale callback protection (generation counter missing)
CloudAutoLabeler uses m_generation to invalidate in-flight callbacks when the user cancels:
// CloudAutoLabeler pattern:
int gen = ++m_generation;
connect(reply, &QNetworkReply::finished, this, [this, gen, ...]() {
if (gen != m_generation) return; // stale → discard
...
});The Landing AI path has no equivalent. Scenario:
- User clicks "Auto Label AI" →
doLandingAIJob()sends request, connects lambda - User clicks Cancel →
m_landingCancelled = true - User immediately clicks "Auto Label AI" again →
m_landingCancelled = false, new request sent - Old reply's lambda fires →
m_landingCancelledis nowfalse→ lambda proceeds and writes label file for the old image with stale data
This was explicitly fixed in PR #128 round 2 ("CRITICAL: Stale callbacks after cancel") with a generation counter. The Landing AI path needs the same protection.
HIGH — Should fix before merge
3. saveState() not called in landingAIAutoLabelAll()
Single-image path correctly calls both:
void MainWindow::submitLandingAIJob() {
save_label_data();
ui->label_image->saveState(); // ✓ Ctrl+Z will work
...
}Batch path only calls one:
void MainWindow::landingAIAutoLabelAll() {
save_label_data();
// ← missing ui->label_image->saveState()
...
}Without saveState(), Ctrl+Z cannot undo the batch labeling. This was flagged as HIGH in PR #128 round 3 ("HIGH 1: No undo after cloud label").
4. No early validation when class file is not loaded
If m_objList is empty (user hasn't loaded a class file), Landing AI will process all images, receive detections with label names like "person", "car", etc., and then silently drop ALL of them because m_objList.indexOf(label) returns -1 for every label.
The user sees "Landing AI: 0 detections" or "Landing AI: detections skipped (unknown labels: ...)" after waiting for the network round-trip. This is confusing.
For YoloLabel AI, CloudAutoLabeler::setClasses() is called before submission and the server uses the class list directly. For Landing AI, the class list is critical for mapping label names → class IDs.
Fix: Add an early check in submitLandingAIJob() and landingAIAutoLabelAll():
if (m_objList.isEmpty()) {
QMessageBox::information(this, "Landing AI",
"Please load a class file first — Landing AI returns label names\n"
"that must be mapped to your class list.");
return;
}5. Missing forward declaration for QNetworkAccessManager in mainwindow.h
mainwindow.h declares QNetworkAccessManager *m_landingNet but relies on the transitive include from cloud_labeler.h. If cloud_labeler.h ever changes its includes, mainwindow.h would fail to compile. Add either #include <QNetworkAccessManager> or a forward declaration class QNetworkAccessManager;.
MEDIUM — Strongly recommended
6. Inconsistent behavior for empty vs. filtered-out detections
- Server returns empty
dataarray → old label file preserved (early return at line 1527, no file write) - Server returns detections that are ALL filtered out (bad coords, unknown labels) → backup created, file truncated to 0 bytes (file opened at line 1564, nothing written)
This inconsistency means a model returning "no detections" preserves labels, but a model returning detections that all fail validation destroys them. Both should behave the same way — either both preserve or both overwrite.
7. Case-sensitive label matching still not addressed
Previous review (round 2, MEDIUM #3) recommended case-insensitive matching. Current code:
int classId = m_objList.indexOf(label); // exact, case-sensitiveIf Landing AI returns "Person" but the class file has "person", the detection is silently dropped. The skippedLabels reporting (added in round 2 fix) mitigates the UX, but a case-insensitive fallback would be more robust:
int classId = m_objList.indexOf(label);
if (classId < 0) {
// Case-insensitive fallback
for (int i = 0; i < m_objList.size(); ++i) {
if (m_objList[i].compare(label, Qt::CaseInsensitive) == 0) {
classId = i; break;
}
}
}8. Model selection not persisted across restarts
The combo box always starts at index 0 ("YoloLabel AI"). If a user primarily uses Landing AI, they have to reselect it every launch. Save/restore via QSettings like the API keys.
9. Magic number 1 for Landing AI combo index
Used in 4+ places:
if (m_modelCombo->currentIndex() == 1) submitLandingAIJob();
if (m_modelCombo->currentIndex() == 1) landingAIAutoLabelAll();
bool isLanding = (m_modelCombo->currentIndex() == 1); // x2An enum would improve readability:
enum CloudProvider { ProviderYoloLabel = 0, ProviderLandingAI = 1 };LOW / Nit
10. Missing explicit includes in mainwindow.cpp — QHttpMultiPart, QHttpPart, QJsonDocument, QJsonObject, QJsonArray are used but not included (they come transitively from cloud_labeler.h). Adding them makes the dependency explicit and protects against future refactors.
11. setBody() vs setBodyDevice() — PR #128 used setBodyDevice() to halve peak memory per image. Landing AI uses f.readAll() + setBody(). Minor since it's one image at a time, but worth aligning for consistency.
12. Architecture note (reiteration from round 1) — ~300 lines of networking code directly in MainWindow duplicates the cancel/retry/backup/consent infrastructure already in CloudAutoLabeler. This is the same pattern refactored out during PR #128's first review round. A provider abstraction in CloudAutoLabeler or a separate LandingAILabeler class would eliminate the duplication and automatically inherit the robustness of the existing infrastructure (generation counter, busy state, etc.). Not blocking, but every new bug above (stale callbacks, navigation race, saveState) exists precisely because this code reimplements what CloudAutoLabeler already does correctly.
What looks good
- Clean rebase on current master — no regressions from PR #128
QImageReader.size()for dimensions without decoding pixels — efficient- Full JSON validation:
firstVal.isArray(),isDouble()checks, coordinate range validation - Separate consent dialog for Landing AI (distinct from yololabel.com notice)
- 3-retry logic matching
CloudAutoLabeler::MAX_RETRIES backupLabelFile()before overwriting label filessave_label_data()+saveState()for single-image undo support- Progress counter correctly accounts for in-flight image (
done = total - remaining - 1) - Unknown label reporting to status bar (
skippedLabels) - File write failure detection with status bar message
- Slider disabled during Landing AI operations
Summary
| Severity | Count | Blocking? |
|---|---|---|
| CRITICAL | 2 | Yes |
| HIGH | 3 | Yes (3, 4); Recommended (5) |
| MEDIUM | 4 | No |
| LOW | 3 | No |
The two CRITICAL issues (#1 navigation race, #2 stale callbacks) are bugs that were explicitly fixed in PR #128 and are re-introduced here because the Landing AI path reimplements networking outside of CloudAutoLabeler. Fix those plus the HIGH items, and this is ready to merge.
CRITICAL: - Add m_landingBusy flag; check it in next_img()/prev_img() to block A/D keyboard navigation during Landing AI batch operations (same race condition fixed in PR developer0hye#128 round 2 for CloudAutoLabeler) - Add m_landingGeneration counter; increment on cancel and new job start; capture gen in doLandingAIJob() lambda and discard stale callbacks when gen != m_landingGeneration HIGH: - Add ui->label_image->saveState() in landingAIAutoLabelAll() so Ctrl+Z undo works after batch labeling - Add early m_objList.isEmpty() guard in submitLandingAIJob() and landingAIAutoLabelAll() to prevent silent drop of all detections when no class file is loaded - Add #include <QNetworkAccessManager> to mainwindow.h to remove reliance on transitive include from cloud_labeler.h MEDIUM: - Collect valid detections into outputLines before opening the label file; only write (and backup) if non-empty — makes "all detections filtered" behave consistently with "no detections" (both preserve the existing file) - Add case-insensitive fallback in label matching so Landing AI returning "Person" correctly maps to class "person" - Persist model combo selection to QSettings (key "modelCombo") in saveAiSettings(); restore in initSideTabWidget() after combo init - Replace magic number 1 with CloudProvider enum (ProviderYoloLabel, ProviderLandingAI) in all four usage sites LOW: - Add explicit includes: QHttpMultiPart, QHttpPart, QJsonArray, QJsonDocument, QJsonObject, QNetworkReply, QNetworkRequest - Switch doLandingAIJob() image upload from readAll()+setBody() to setBodyDevice() with QFile parented to multiPart (consistent with CloudAutoLabeler, avoids redundant in-memory copy) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
…loper0hye#4) Mirrors the ≥500MB upload warning already present in cloudAutoLabelAll(): compute total byte size across all images before the confirmation dialog; if it exceeds 500 MB, append the estimated size and a bandwidth notice to the confirmation message. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
developer0hye
left a comment
There was a problem hiding this comment.
Strict Review — PR #129: Landing AI Integration (Round 4)
The latest two commits (301520a, 80ef2f4) addressed most of the round 3 issues well — generation counter, m_landingBusy guard, saveState() in batch, m_objList.isEmpty() guard, explicit includes, setBodyDevice(), case-insensitive matching, CloudProvider enum, model combo persistence, and upload size warning. CI passes on all 3 platforms.
However, after cross-referencing with the robustness standards established in PR #128 (which went through 6 rounds of review), there are still issues that need attention. The root cause is architectural: ~350 lines of networking code reimplemented directly in MainWindow, duplicating infrastructure that CloudAutoLabeler already provides. Every bug below exists because the Landing AI path re-implements what CloudAutoLabeler handles correctly.
CRITICAL 1: cancelAutoLabel() fails to reset buttons when only Landing AI is running
mainwindow.cpp — cancelAutoLabel(), cloud_labeler.cpp:81-86
cancelAutoLabel() does not call resetCloudButtons() directly. It relies on m_cloudLabeler->cancel() emitting busyChanged(false), which triggers the connected lambda:
// Line 155-159: busyChanged handler
connect(m_cloudLabeler, &CloudAutoLabeler::busyChanged, this, [this](bool busy) {
ui->horizontalSlider_images->setEnabled(!busy);
if (!busy) resetCloudButtons();
});But CloudAutoLabeler::setBusy() has a guard:
// cloud_labeler.cpp:81-86
void CloudAutoLabeler::setBusy(bool busy) {
if (m_busy == busy) return; // ← Already false? No signal emitted!
m_busy = busy;
emit busyChanged(busy);
}Scenario:
- User selects Landing AI, clicks "Auto Label All" →
m_landingBusy = true, buttons disabled, slider disabled. CloudAutoLabeler is not busy (m_busy == false). - User clicks Cancel →
cancelAutoLabel()callsm_cloudLabeler->cancel()→setBusy(false)→ guard fires (false == false) → no signal emitted →resetCloudButtons()never called. - The in-flight Landing AI callback fires →
gen != m_landingGeneration→ returns immediately without callingresetCloudButtons(). - Result: Buttons stuck in disabled/cancel state permanently. The user must quit and restart the app.
Required fix: Call resetCloudButtons() directly in cancelAutoLabel():
void MainWindow::cancelAutoLabel()
{
++m_landingGeneration;
m_cloudLabeler->cancel();
m_landingCancelled = true;
m_landingQueue.clear();
resetCloudButtons(); // ← Ensure buttons always reset
}Note: the busyChanged(false) handler also calls resetCloudButtons(), so when CloudAutoLabeler IS busy, it will be called twice. resetCloudButtons() is idempotent (sets fixed states), so double-calling is safe.
CRITICAL 2: doLandingAIJob() stale-callback path calls resetCloudButtons() on cancel but NOT on generation mismatch — inconsistent cleanup
mainwindow.cpp — doLandingAIJob() lambda
connect(reply, &QNetworkReply::finished, this, [this, reply, imagePath, retryCount, gen]() {
reply->deleteLater();
if (gen != m_landingGeneration) return; // ← Path A: just return, no cleanup
if (m_landingCancelled) {
resetCloudButtons(); // ← Path B: cleanup
return;
}
...
});And at the top of doLandingAIJob():
void MainWindow::doLandingAIJob(const QString &imagePath, int retryCount, int gen)
{
if (gen != m_landingGeneration || m_landingCancelled) {
resetCloudButtons(); // ← Path C: cleanup (synchronous call)
return;
}
...
}Path A (gen mismatch in async lambda) just returns without any cleanup. This is correct IF the caller of cancel/new-job has already reset the buttons. But as shown in CRITICAL 1, cancelAutoLabel() does NOT reset buttons directly. So:
- Cancel →
++m_landingGeneration→ stale lambda fires → Path A → no cleanup → buttons stuck
Path C (synchronous check) is only reached when landingAIProcessNextInQueue() calls doLandingAIJob() and the generation was bumped in between. This correctly cleans up. But Path A (the more common async case) does not.
This is the same issue as CRITICAL 1 — fixing CRITICAL 1 by adding resetCloudButtons() to cancelAutoLabel() will also fix this path. But it's worth noting the inconsistency for maintainability.
HIGH 1: Division by zero when image dimensions cannot be determined
mainwindow.cpp — doLandingAIJob() lambda
QImageReader reader(imagePath);
QSize imgSize = reader.size();
if (!imgSize.isValid() || imgSize.isEmpty())
imgSize = QImage(imagePath).size();
double imgW = imgSize.width();
double imgH = imgSize.height();
// ...
double cx = ((x1 + x2) / 2.0) / imgW; // imgW could be 0If both QImageReader and QImage fail (corrupted file, unsupported format, permission denied), imgSize remains invalid/empty. imgW and imgH would be 0, causing division by zero in the coordinate normalization. While IEEE 754 would produce infinity (filtered by the range check), this is technically undefined behavior in C++.
Required fix: Guard against zero dimensions:
if (imgW <= 0 || imgH <= 0) {
statusBar()->showMessage("Landing AI: could not read image dimensions: " + imagePath, 5000);
if (m_landingQueue.isEmpty()) resetCloudButtons();
else landingAIProcessNextInQueue();
return;
}HIGH 2: Stale callback from single-image can interfere with subsequent batch
The generation counter is incremented in landingAIAutoLabelAll() and submitLandingAIJob(), but consider this edge case:
- User clicks "Auto Label AI" (single image) →
gen = ++m_landingGeneration(gen=1) - Network is slow, reply hasn't arrived yet
- User clicks "Auto Label All AI" →
++m_landingGeneration(gen=2), batch starts processing - Old single-image reply arrives →
gen(1) != m_landingGeneration(2)→ returns immediately ✓
This case IS correctly handled by the generation counter. Good.
But: submitLandingAIJob() sets m_landingBusy = true and m_landingCancelled = false. If the old reply from step 2 is still in flight, and the user starts a new single-image job (not a batch), the old callback sees gen != m_landingGeneration and returns. Fine.
Actually, on closer inspection, there's a subtler issue: submitLandingAIJob() does not clear m_landingQueue. If a batch was cancelled (queue cleared by cancelAutoLabel) and then a single image is submitted, this is fine. But if the user rapidly clicks "Auto Label AI" multiple times, each call creates a new callback. The generation counter handles this correctly. OK, this one is fine.
Downgrading to informational — generation counter correctly protects this case.
HIGH 3: MAX_LANDING_RETRIES scoped as function-local static — inconsistent with CloudAutoLabeler
void MainWindow::doLandingAIJob(const QString &imagePath, int retryCount, int gen)
{
...
static constexpr int MAX_LANDING_RETRIES = 3;
...
}MAX_LANDING_RETRIES is defined inside the function body. It works correctly but:
- Not accessible from other functions (e.g.,
submitLandingAIJob()if retry logic is ever needed at the submission level) - Inconsistent with
CloudAutoLabeler::MAX_RETRIESwhich is a class-level constant - A static constexpr inside a function is fine semantically, but it looks like it was meant to be a class-level constant matching the CloudAutoLabeler pattern
Suggested fix: Move to a private constant in MainWindow or match CloudAutoLabeler::MAX_RETRIES directly.
MEDIUM 1: Architecture concern — networking code in MainWindow (reiteration, non-blocking)
The initial review of PR #128 identified "636 lines added directly to mainwindow.cpp" as the first issue and extracted all cloud logic into CloudAutoLabeler. PR #129 adds ~350 lines of networking code back into MainWindow. The result:
- Cancel/retry/backup/consent/generation counter logic is duplicated rather than shared
- 5 of the 6 commits in this PR are bug fixes for issues that
CloudAutoLabeleralready handles correctly - Future providers would each add another ~300 lines to MainWindow
A provider abstraction in CloudAutoLabeler (or a separate LandingAILabeler class following the same QObject+signals pattern) would:
- Eliminate CRITICAL 1 (cancel would go through the same
setBusypath) - Eliminate the class of bugs where Landing AI path misses safety mechanisms
- Make future providers trivial to add
Not blocking merge, but strongly recommended as a follow-up refactor before adding any more providers.
MEDIUM 2: Batch "done" count can confuse users
landingAIProcessNextInQueue():
int done = m_imgList.size() - m_landingQueue.size() - 1;For a 5-image batch:
- Processing image 1: button shows "☁ Auto Label All (0/5)…"
- Processing image 2: button shows "☁ Auto Label All (1/5)…"
CloudAutoLabeler shows "1/N" for the first image being processed (progress signal emits done=1). The Landing AI path shows "0/N". This is a minor UX inconsistency between the two providers.
MEDIUM 3: m_landingNet is never deleted explicitly
m_landingNet = new QNetworkAccessManager(this);This is actually fine — it's parented to this (MainWindow), so Qt's parent-child ownership handles cleanup. Just noting it's correct.
Non-issue on inspection.
Summary
| Severity | # | Issue | Blocking? |
|---|---|---|---|
| CRITICAL | 1 | cancelAutoLabel() doesn't reset buttons when only Landing AI is running |
Yes |
| CRITICAL | 2 | Inconsistent cleanup in stale callback paths (same root cause as #1) | Yes |
| HIGH | 1 | Division by zero when image dimensions cannot be determined | Yes |
| HIGH | 3 | MAX_LANDING_RETRIES function-local scope |
No (style) |
| MEDIUM | 1 | Architecture: ~350 lines of networking reimplemented in MainWindow | No (follow-up) |
| MEDIUM | 2 | Batch progress counter inconsistency | No |
What looks good
- Clean rebase on current master — no regressions from PR #128
- Generation counter (
m_landingGeneration) correctly handles stale callbacks in the async lambda m_landingBusyflag properly blocks A/D navigation during Landing AI operationsQImageReader.size()for efficient dimension queries without full decode- Full JSON validation:
firstVal.isArray(),isDouble()checks, coordinate range validation - Case-insensitive label matching with fallback
- Separate consent dialog for Landing AI (distinct from yololabel.com)
- 3-retry logic with parameter-based retry counting (matches CloudAutoLabeler pattern)
backupLabelFile()before overwriting label filessave_label_data()+saveState()for both single-image and batch undo supportsetBodyDevice()for memory-efficient image uploadCloudProviderenum replacing magic numbers- Model selection persisted to QSettings
- Upload size warning for large batches
m_objList.isEmpty()early guard preventing silent detection drops- Unknown label reporting to status bar (
skippedLabels) - File write failure detection with status bar message
- Slider disabled during Landing AI operations
- Only writes file when detections are non-empty (consistent with "no detections" path)
Recommended action
Fix CRITICAL 1 (one-line fix in cancelAutoLabel()) and HIGH 1 (image dimension guard). Both are small, targeted changes. CRITICAL 2 resolves automatically when CRITICAL 1 is fixed. After those, this is ready to merge.
developer0hye
left a comment
There was a problem hiding this comment.
Strict Review — PR #129: Landing AI Integration (Round 4)
The latest two commits (301520a, 80ef2f4) addressed most round 3 issues well — generation counter, m_landingBusy guard, saveState() in batch, m_objList.isEmpty() guard, explicit includes, setBodyDevice(), case-insensitive matching, CloudProvider enum, model combo persistence, and upload size warning. CI passes on all 3 platforms.
However, after cross-referencing with the robustness standards established in PR #128 (which went through 6 rounds of review), there are still issues that need attention.
CRITICAL: cancelAutoLabel() fails to reset buttons when only Landing AI is running
mainwindow.cpp cancelAutoLabel(), cloud_labeler.cpp:81-86
cancelAutoLabel() does not call resetCloudButtons() directly. It relies on m_cloudLabeler->cancel() emitting busyChanged(false), which triggers the connected lambda (line 155-159):
connect(m_cloudLabeler, &CloudAutoLabeler::busyChanged, this, [this](bool busy) {
ui->horizontalSlider_images->setEnabled(!busy);
if (!busy) resetCloudButtons();
});
But CloudAutoLabeler::setBusy() has a guard:
// cloud_labeler.cpp:81-86
void CloudAutoLabeler::setBusy(bool busy) {
if (m_busy == busy) return; // Already false? No signal emitted!
m_busy = busy;
emit busyChanged(busy);
}
Scenario:
- User selects Landing AI, clicks "Auto Label All" -> m_landingBusy = true, buttons disabled, slider disabled. CloudAutoLabeler is NOT busy (m_busy == false).
- User clicks Cancel -> cancelAutoLabel() calls m_cloudLabeler->cancel() -> setBusy(false) -> guard fires (false == false) -> no signal emitted -> resetCloudButtons() never called.
- The in-flight Landing AI callback fires -> gen != m_landingGeneration -> returns immediately without calling resetCloudButtons().
- Result: Buttons stuck in disabled/cancel state permanently. The user must quit and restart the app.
Additionally, in the doLandingAIJob() lambda, the stale-callback path (gen mismatch) just returns without cleanup:
if (gen != m_landingGeneration) return; // No resetCloudButtons()!
While the cancel path does clean up:
if (m_landingCancelled) {
resetCloudButtons(); // Cleanup
return;
}
But since cancelAutoLabel() increments m_landingGeneration, the lambda always hits the gen-mismatch path (Path A, no cleanup) rather than the m_landingCancelled path (Path B, with cleanup).
Required fix: Call resetCloudButtons() directly in cancelAutoLabel():
void MainWindow::cancelAutoLabel()
{
++m_landingGeneration;
m_cloudLabeler->cancel();
m_landingCancelled = true;
m_landingQueue.clear();
resetCloudButtons(); // Ensure buttons always reset
}
resetCloudButtons() is idempotent, so double-calling (when CloudAutoLabeler IS busy and busyChanged fires too) is safe.
HIGH 1: Division by zero when image dimensions cannot be determined
mainwindow.cpp doLandingAIJob() lambda
QImageReader reader(imagePath);
QSize imgSize = reader.size();
if (!imgSize.isValid() || imgSize.isEmpty())
imgSize = QImage(imagePath).size();
double imgW = imgSize.width();
double imgH = imgSize.height();
// ...
double cx = ((x1 + x2) / 2.0) / imgW; // imgW could be 0!
If both QImageReader and QImage fail (corrupted file, unsupported format, permission denied), imgSize remains invalid. imgW and imgH would be 0, causing division by zero in the coordinate normalization.
Required fix: Guard against zero dimensions:
if (imgW <= 0 || imgH <= 0) {
statusBar()->showMessage(
"Landing AI: could not read image dimensions: " + imagePath, 5000);
if (m_landingQueue.isEmpty()) resetCloudButtons();
else landingAIProcessNextInQueue();
return;
}
HIGH 2: MAX_LANDING_RETRIES function-local scope
void MainWindow::doLandingAIJob(const QString &imagePath, int retryCount, int gen)
{
...
static constexpr int MAX_LANDING_RETRIES = 3;
...
}
MAX_LANDING_RETRIES is defined inside the function body as a static constexpr. It works correctly but is inconsistent with CloudAutoLabeler::MAX_RETRIES which is a class-level constant. Consider moving to a class-level constant.
Not blocking, but noted for consistency.
MEDIUM 1: Architecture concern (reiteration, non-blocking)
The initial review of PR #128 identified "636 lines added directly to mainwindow.cpp" as the first issue and extracted all cloud logic into CloudAutoLabeler. PR #129 adds ~350 lines of networking code back into MainWindow. The CRITICAL bug above exists precisely because the Landing AI path bypasses CloudAutoLabeler's cancel/busy infrastructure.
A provider abstraction in CloudAutoLabeler (or a separate LandingAILabeler class following the same QObject+signals pattern) would eliminate CRITICAL 1 entirely — cancel would go through the same setBusy path. Strongly recommended as a follow-up refactor before adding more providers.
MEDIUM 2: Batch progress counter inconsistency
landingAIProcessNextInQueue():
int done = m_imgList.size() - m_landingQueue.size() - 1;
For a 5-image batch, processing image 1 shows "Auto Label All (0/5)..." while CloudAutoLabeler shows "1/N" for the first image. Minor UX inconsistency between providers.
Summary
| Severity | Issue | Blocking? |
|---|---|---|
| CRITICAL | cancelAutoLabel() doesn't reset buttons when only Landing AI is running | Yes |
| HIGH | Division by zero when image dimensions cannot be determined | Yes |
| HIGH | MAX_LANDING_RETRIES function-local scope | No (style) |
| MEDIUM | Architecture: ~350 lines of networking reimplemented in MainWindow | No (follow-up) |
| MEDIUM | Batch progress counter inconsistency | No |
What looks good
- Clean rebase on current master — no regressions from PR #128
- Generation counter (m_landingGeneration) correctly handles stale callbacks in the async lambda
- m_landingBusy flag properly blocks A/D navigation during Landing AI operations
- QImageReader.size() for efficient dimension queries without full decode
- Full JSON validation: firstVal.isArray(), isDouble() checks, coordinate range validation
- Case-insensitive label matching with fallback
- Separate consent dialog for Landing AI (distinct from yololabel.com)
- 3-retry logic with parameter-based retry counting (matches CloudAutoLabeler pattern)
- backupLabelFile() before overwriting label files
- save_label_data() + saveState() for both single-image and batch undo support
- setBodyDevice() for memory-efficient image upload
- CloudProvider enum replacing magic numbers
- Model selection persisted to QSettings
- Upload size warning for large batches
- m_objList.isEmpty() early guard preventing silent detection drops
- Unknown label reporting to status bar (skippedLabels)
- File write failure detection with status bar message
- Slider disabled during Landing AI operations
- Only writes file when detections are non-empty (consistent with "no detections" path)
Recommended action
Fix the CRITICAL (one-line fix in cancelAutoLabel()) and HIGH 1 (image dimension guard). Both are small, targeted changes. After those, this is ready to merge.
- cancelAutoLabel(): call resetCloudButtons() directly so buttons are always re-enabled even when CloudAutoLabeler is already idle (m_busy == false prevents busyChanged signal from firing) - doLandingAIJob(): guard against zero image dimensions to avoid division by zero when QImageReader and QImage both fail - Move MAX_LANDING_RETRIES from function-local static to class-level constexpr, consistent with CloudAutoLabeler::MAX_RETRIES Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
vusallyv
left a comment
There was a problem hiding this comment.
Addressed all blocking issues in ea558dc:
CRITICAL 1 fixed — cancelAutoLabel() now calls resetCloudButtons() directly, so buttons always re-enable regardless of whether CloudAutoLabeler is busy. The comment explains why: busyChanged is only emitted when m_busy actually changes, so it's a no-op when CloudAutoLabeler is already idle.
CRITICAL 2 — Resolved automatically by the CRITICAL 1 fix. The gen-mismatch path in the stale lambda no longer needs to call resetCloudButtons() itself, since cancelAutoLabel() handles it synchronously before any callbacks can fire.
HIGH 1 fixed — Added a imgW <= 0 || imgH <= 0 guard in doLandingAIJob() after the QImageReader/QImage fallback. On failure, shows a status bar message and either resets buttons (last image) or advances the queue.
HIGH 3 (style) fixed — MAX_LANDING_RETRIES moved from function-local static constexpr to a class-level static constexpr in mainwindow.h, consistent with CloudAutoLabeler::MAX_RETRIES.
developer0hye
left a comment
There was a problem hiding this comment.
Strict review findings
- CRITICAL: Landing batch uses mutable UI state (
m_imgList) as its job source, so mid-run dataset mutations can mislabel wrong files or hit invalid indices.
- Queue is built from indices of current list:
mainwindow.cpp:1712,mainwindow.cpp:1714 - Each step dereferences current
m_imgList[idx]:mainwindow.cpp:1745 m_imgListcan still change during Landing run:- open new dataset has no busy guard:
mainwindow.cpp:284 - delete image has no busy guard:
mainwindow.cpp:472 - delete shortcuts remain active:
mainwindow.cpp:48,mainwindow.cpp:49 - only prev/next are guarded by
m_landingBusy:mainwindow.cpp:417,mainwindow.cpp:425
- open new dataset has no busy guard:
- MEDIUM: Unknown-label diagnostics are immediately overwritten, so users lose the reason detections were dropped.
- unknown-label message is shown:
mainwindow.cpp:1652 - then immediately overwritten by generic detection count:
mainwindow.cpp:1660
I could not run build/tests locally in this environment because qmake is not installed; this review is static analysis of branch feat/landing-ai.
…ages - on_pushButton_open_files_clicked() and remove_img() now early-return when CloudAutoLabeler or Landing AI batch is running, preventing m_imgList mutations that would invalidate queued indices (CRITICAL developer0hye#1) - Merged skipped-labels status message with detection-count message on final/single image so the unknown-label info is never overwritten (MEDIUM developer0hye#2 from round 5 review) Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
|
Addressed both round 5 issues in 6ec7644: CRITICAL fixed — MEDIUM fixed — Removed the separate |
developer0hye
left a comment
There was a problem hiding this comment.
Strict review findings
- CRITICAL:
Canceldoes not stop in-flight Landing AI network activity.
cancelAutoLabel()only flips flags/clears queue/reset UI:mainwindow.cpp:1307- Active request is created in
doLandingAIJob()and not retained for abort:mainwindow.cpp:1517 - Stale callback path only ignores completion (
genmismatch) instead of stopping upload:mainwindow.cpp:1524
Impact: after clicking Cancel, the upload/request may still continue until completion on the wire; only callback effects are suppressed.
- HIGH: Landing batch reads mutable settings (
m_landingApiKey,m_cloudPrompt) per-image, so one batch can run with mixed settings.
- Settings can change at runtime via Save:
mainwindow.cpp:1275 - Each request recomputes prompt/auth from current members:
mainwindow.cpp:1494,mainwindow.cpp:1514
Impact: if user saves settings mid-batch, later images can run with different key/prompt than earlier images, producing inconsistent behavior and hard-to-debug failures.
- MEDIUM: No automated test coverage for the new Landing AI flow in MainWindow.
- Existing tests focus on
CloudAutoLabelerbehavior (e.g.tests/test_cloud_labeler.cpp:2) - New paths (
submitLandingAIJob,landingAIAutoLabelAll,doLandingAIJob,cancelAutoLabel) have no regression tests for cancel, queue progression, and response edge cases.
I could not run build/tests locally because qmake is not installed in this environment; this review is static analysis of branch feat/landing-ai.
1. CRITICAL: abort in-flight QNetworkReply on cancel
- Store active reply in m_activeLandingReply
- cancelAutoLabel() calls reply->abort() before incrementing generation,
stopping the upload on the wire rather than only suppressing the callback
2. HIGH: snapshot settings at batch start
- Add m_batchLandingApiKey / m_batchLandingPrompt, captured in
submitLandingAIJob() and landingAIAutoLabelAll() before the first request
- doLandingAIJob() uses the snapshot; mid-batch Save no longer changes
auth or prompt for images already in the queue
3. MEDIUM: add unit tests for Landing AI JSON response parsing
- Move parseLandingAIDetections() from MainWindow to CloudAutoLabeler
so it can be tested without instantiating the full widget stack
- Add tests/test_landing_ai_parser.{cpp,pro} with 15 cases covering:
exact label match, case-insensitive fallback, unknown/empty labels,
missing/short/non-numeric bounding_box, out-of-range and zero-size
boxes, multiple detections with mixed validity, coordinate math
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
developer0hye
left a comment
There was a problem hiding this comment.
Strict Review — PR #129: Landing AI Integration (Round 8)
CI passes on all 3 platforms + DCO. The PR has improved significantly through 8 rounds — the latest commits address cancel-abort, settings snapshot, open/delete guards, and unit tests. Cross-referencing with CloudAutoLabeler's robustness patterns reveals the following remaining issues.
CRITICAL: effectivePrompt reads mutable m_objList at request time, not at batch start
mainwindow.cpp — doLandingAIJob()
QString effectivePrompt = m_batchLandingPrompt.isEmpty()
? m_objList.join(" ; ") // ← reads mutable m_objList at request time
: m_batchLandingPrompt;The API key and prompt were correctly snapshotted into m_batchLandingApiKey / m_batchLandingPrompt (round 5 fix). But when the user-set prompt is empty (the most common case — Landing AI prompt defaults to class names), m_objList is read per-request, not snapshotted. If the user opens a different class file mid-batch (not blocked — open_obj_file has no m_landingBusy guard), later images would use different prompts than earlier ones.
Fix: Snapshot the effective prompt at batch start:
// In submitLandingAIJob() / landingAIAutoLabelAll():
m_batchLandingPrompt = m_cloudPrompt.isEmpty()
? m_objList.join(" ; ")
: m_cloudPrompt;
// In doLandingAIJob(): always use m_batchLandingPrompt (remove the fallback)HIGH 1: No mutual exclusion between YoloLabel AI and Landing AI operations
There is no guard preventing both providers from running concurrently. Scenario:
- User starts a YoloLabel AI batch → CloudAutoLabeler busy, buttons disabled
- CloudAutoLabeler finishes →
busyChanged(false)→resetCloudButtons()re-enables all buttons and setsm_landingBusy = false - User switches combo to Landing AI, clicks "Auto Label AI"
- Meanwhile, if timing is close, both providers could run concurrently sharing the same buttons and cancel infrastructure
Fix: The button-clicked lambdas should check both providers before proceeding:
connect(m_btnCloudAutoLabel, &QPushButton::clicked, this, [this](){
if (m_landingBusy || (m_cloudLabeler && m_cloudLabeler->isBusy())) return;
if (m_modelCombo->currentIndex() == ProviderLandingAI) submitLandingAIJob();
else submitCloudJob();
});Same for m_btnCloudAutoLabelAll.
HIGH 2: cancelAutoLabel() ordering — landing state set after CloudAutoLabeler cancel
void MainWindow::cancelAutoLabel()
{
++m_landingGeneration;
if (m_activeLandingReply) {
m_activeLandingReply->abort();
m_activeLandingReply = nullptr;
}
m_cloudLabeler->cancel(); // ← triggers busyChanged(false) → resetCloudButtons() → m_landingBusy=false
m_landingCancelled = true; // ← set AFTER resetCloudButtons() already ran
m_landingQueue.clear(); // ← cleared AFTER resetCloudButtons() already ran
resetCloudButtons();
}m_cloudLabeler->cancel() triggers setBusy(false) → busyChanged(false) → resetCloudButtons(), which sets m_landingBusy = false. Between that moment and the explicit assignments below, the landing state is inconsistent (busy=false but cancelled/queue not yet cleared). Safe in practice (single-threaded Qt event loop), but fragile and confusing.
Fix: Set landing state before m_cloudLabeler->cancel():
void MainWindow::cancelAutoLabel()
{
++m_landingGeneration;
if (m_activeLandingReply) {
m_activeLandingReply->abort();
m_activeLandingReply = nullptr;
}
m_landingCancelled = true;
m_landingQueue.clear();
m_cloudLabeler->cancel();
resetCloudButtons();
}MEDIUM 1: Batch skipped-labels message is immediately overwritten
if (!skippedLabels.isEmpty()) {
statusBar()->showMessage(
QString("Landing AI: detections skipped (unknown labels: %1)")
.arg(skippedLabels.join(", ")), 5000);
}
landingAIProcessNextInQueue(); // immediately fires next requestThe skipped-labels message is set with 5s timeout, but the next request completes and overwrites it immediately. During a batch, the user never sees which labels were skipped for intermediate images.
Suggestion: Accumulate skipped labels across the batch and report them in the final summary message.
MEDIUM 2: Architecture — ~350 lines of networking in MainWindow (reiteration, non-blocking)
Every round has found bugs that CloudAutoLabeler already handles correctly (cancel, stale callbacks, busy guards, retry, backup, settings snapshot). The pattern is clear: reimplemented infrastructure drifts from the reference implementation. A provider abstraction would eliminate this entire class of bugs. Not blocking, but strongly recommended before adding a third provider.
MEDIUM 3: LANDING_AI_ENDPOINT should be constexpr
static const char *LANDING_AI_ENDPOINT = ...;File-scope static const char* — static constexpr const char* would be more idiomatic C++17 and consistent with CloudAutoLabeler's constants.
LOW / Nits
- Double blank line between
doLandingAIJob()andlandingAIAutoLabelAll()— style inconsistency m_modelCombonot initialized tonullptrin the member list — initialized ininitSideTabWidget()but a default= nullptrwould be safer- 30s transfer timeout (
req.setTransferTimeout(30000)) may be too short for large images on slow connections —CloudAutoLabelerdoesn't set a transfer timeout
Summary
| Severity | # | Issue | Blocking? |
|---|---|---|---|
| CRITICAL | 1 | effectivePrompt reads mutable m_objList per-request |
Yes |
| HIGH | 1 | No mutual exclusion between providers | Yes |
| HIGH | 2 | cancelAutoLabel() ordering |
No (fragile, not unsafe) |
| MEDIUM | 1 | Batch skipped-labels message overwritten | No (UX) |
| MEDIUM | 2 | Architecture: networking reimplemented in MainWindow | No (follow-up) |
| MEDIUM | 3 | const char* vs constexpr |
No (style) |
| LOW | 3 | Style nits | No |
What looks good
- Clean rebase — no regressions from PR #128
- Generation counter correctly handles stale callbacks
m_landingBusyguards A/D navigation and open/delete during batchm_activeLandingReply->abort()stops in-flight uploads on cancel- Settings snapshot (
m_batchLandingApiKey/m_batchLandingPrompt) prevents mid-batch key/prompt drift QImageReader.size()for efficient dimension queries without full decode- Full JSON validation:
firstVal.isArray(),isDouble()checks, coordinate range validation, division-by-zero guard - Case-insensitive label matching with fallback
- Separate consent dialog for Landing AI (distinct from yololabel.com)
- 3-retry logic,
backupLabelFile(),save_label_data()+saveState()for undo setBodyDevice()for memory-efficient uploadCloudProviderenum replacing magic numbers, model combo persisted to QSettingsparseLandingAIDetections()extracted as pure static function with 15 unit tests- Upload size warning for large batches,
m_objList.isEmpty()early guard - Unknown label reporting to status bar, file write failure detection
Recommended action
- Fix CRITICAL — snapshot effective prompt at batch start (small change in 2 functions)
- Fix HIGH 1 — add mutual exclusion guards in button lambdas (2 lines each)
- Fix HIGH 2 — reorder
cancelAutoLabel()(just move 2 lines up) - Everything else can be follow-up PRs
CRITICAL: snapshot effective prompt at batch start in submitLandingAIJob() and landingAIAutoLabelAll() using m_objList.join() when m_cloudPrompt is empty, preventing mid-batch class file changes from affecting later images. doLandingAIJob() now always uses m_batchLandingPrompt directly. HIGH 1: add mutual exclusion guards in Auto Label / Auto Label All button lambdas — both check m_landingBusy and m_cloudLabeler->isBusy() before proceeding, preventing concurrent YoloLabel AI + Landing AI operations. HIGH 2: reorder cancelAutoLabel() to set m_landingCancelled=true and clear m_landingQueue before calling m_cloudLabeler->cancel(), eliminating the window where landing state is inconsistent with busy state. Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
|
@vusallyv Why did you choose Landing AI? |
|
Landing AI uses the same prompt-to-detections approach as YoloLabel AI, so the integration fits naturally without changing the UX. That said, in my testing YoloLabel AI gives more accurate detections and is less expensive — Landing AI is there as an alternative for users who already have a Landing AI API key. |
developer0hye
left a comment
There was a problem hiding this comment.
Strict review findings (blocking)
1) HIGH — Landing AI write-failure is reported as success
- In
doLandingAIJob(), file open failure is detected and a status error is shown:mainwindow.cpp:1613(if (lf.open(...)))mainwindow.cpp:1619(could not write label file)
- But execution falls through and still shows a success-style message based on detection count:
mainwindow.cpp:1626("Landing AI: %1 detection(s)")
Impact: users can be told labeling succeeded even when no label file was written.
2) HIGH — Landing label matching breaks with CRLF/trailing-space class names
- Class labels are loaded without trimming:
mainwindow.cpp:535(m_objList << qstrLabel)
- Landing parser compares raw label strings:
cloud_labeler.cpp:722(objList.indexOf(label))cloud_labeler.cpp:726(case-insensitive compare, still untrimmed)
I reproduced this quickly with objList={"cat\r"} and detection label "cat": parser returned no output lines and treated the label as skipped.
Impact: detections are silently dropped for common Windows-formatted class files.
3) MEDIUM — New parser test is not wired into CI
- PR adds parser tests:
tests/test_landing_ai_parser.cpp
- But CI currently runs only:
test_cloud_labeler,test_label_img,test_yolo_detector.github/workflows/ci-release.yml:130-140and:150-163
Impact: parser regressions can merge undetected.
Verification performed locally
qmake YoloLabel.pro && make(build passed)tests/test_landing_ai_parser(16 passed)tests/test_cloud_labeler(37 passed)
- fix: write-failure in doLandingAIJob() no longer falls through to success message; early return after status bar error, with proper queue continuation or button reset - fix: class names trimmed on load (CRLF/trailing-space in Windows class files no longer silently drops Landing AI detections) - fix: parseLandingAIDetections trims both objList entries and the incoming label before case-insensitive comparison - test: add crlfObjList_matchesTrimmed and trailingSpaceLabel_matchesTrimmed cases to test_landing_ai_parser - ci: wire test_landing_ai_parser into CI for Unix and Windows Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
developer0hye
left a comment
There was a problem hiding this comment.
-
HIGH:
parseLandingAIDetections()still accepts partially off-image boxes. It only validates normalized center/size, not the raw corners, so a box like[-10, 10, 90, 90]on a100x100image passes and gets written as a YOLO annotation even though part of the box lies outside the image. That will poison label files instead of rejecting or clamping invalid detections. Seecloud_labeler.cpp:734,cloud_labeler.cpp:741,cloud_labeler.cpp:746. -
HIGH: malformed top-level Landing responses are still treated as "no detections". The
!dataVal.isArray()case is routed through the same branch as an empty detection list, so a200 OKpayload like{"error":"quota exceeded"}or{"data":{}}will silently preserve labels and, in batch mode, continue processing as if nothing went wrong. That hides protocol or service failures behind a false "0 detections" outcome. Seemainwindow.cpp:1564,mainwindow.cpp:1565,mainwindow.cpp:1571. -
MEDIUM: batch completion reporting is inaccurate after recoverable per-image failures. If dimension lookup fails or the label file cannot be opened, the code skips to the next image, but the terminal branch still reports
Landing AI: all %1 images done.Earlier status-bar errors are transient and get overwritten, so users can be told the batch completed successfully even when some images were never labeled. Seemainwindow.cpp:1594,mainwindow.cpp:1619,mainwindow.cpp:1713.
Verification performed locally: built and ran tests/test_landing_ai_parser (18 passed) and tests/test_cloud_labeler (37 passed) in /tmp/pr129-review/tests.
…sponse handling, batch failure count 1. HIGH: clamp partially off-image corners to image bounds in parseLandingAIDetections() instead of discarding the entire detection; boxes entirely outside the image still produce zero size and are skipped. Add partiallyOffImageBox_clamped test (19 pass, 0 fail). 2. HIGH: split !dataVal.isArray() (malformed/error response) from dataVal.toArray().isEmpty() (no detections) in the Landing AI reply handler. A non-array "data" field now shows a warning and stops processing instead of silently falling through as zero detections. 3. MEDIUM: track m_landingFailCount across a batch run; reset to 0 at batch start, increment on dimension-read or file-write failure, and emit "X/N images labeled (Y failed)" in the completion message so per-image errors are visible to the user. Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
developer0hye
left a comment
There was a problem hiding this comment.
I found 3 issues in the Landing AI integration.
-
Blocker: the request builder always explodes the effective prompt into multiple
promptsform fields while hardcodingmodel=agentic(mainwindow.cpp:1463,mainwindow.cpp:1504,mainwindow.cpp:1514). The current Landing AI OpenAPI says theagenticmodel allows only one prompt for/v1/tools/text-to-object-detection, so the default “use class file labels” path will 422 on any normal multi-class dataset instead of labeling it. -
High: the reply handler rejects any non-array
datavalue as malformed (mainwindow.cpp:1564), but the currentODResponseschema allowsdatato benull. A contract-compliant empty-result response would therefore be surfaced as “Unexpected response from server” and abort the run instead of behaving like “no detections.” -
Low: canceling a Landing AI run still calls
CloudAutoLabeler::cancel()unconditionally (mainwindow.cpp:1309), and that method always emitsCloud auto-label cancelled.even when no cloud job is active (cloud_labeler.cpp:69). The shared cancel button added in this PR will therefore show the wrong provider/status message on every Landing cancel.
I built the PR in a temporary worktree at /tmp/yolo-pr129, built YoloLabel.pro, and ran tests/test_landing_ai_parser successfully (19 passed). I did not run live Landing AI requests because there is no API key in the workspace; the first two findings come from the current official provider schema plus the PR code path.
1. Send all class names as a single `prompts` field instead of splitting into multiple fields. The agentic model only accepts one prompt per request; multi-class datasets would previously 422. 2. Accept `data: null` in the Landing AI response as empty detections instead of treating it as a malformed response. The ODResponse schema allows null. 3. Only call CloudAutoLabeler::cancel() when the cloud labeler is actually busy, preventing a spurious "Cloud auto-label cancelled." message when cancelling a Landing AI run. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Vusal Abdullayev <85983771+vusallyv@users.noreply.github.com>
Summary
Extends the cloud auto-label feature with a second provider: Landing AI. Users can select the model from a combo box in the AI Settings tab.
Key changes (delta over #128)
YoloLabel AI(default) andLanding AIsyncAiSettingsTab()andsaveAiSettings()switch between providers' keys based on selection; Landing AI key stored as"landingApiKey"in QSettingscheckLandingConsent()— one-time privacy notice before images are uploaded toapi.va.landing.ai(separate from the yololabel.com consent)submitLandingAIJob(),doLandingAIJob(),landingAIAutoLabelAll(),landingAIProcessNextInQueue()— full single-image and all-images flowscancelAutoLabel()setsm_landingCancelledand clears the queueCloudAutoLabeler::MAX_RETRIES)CloudAutoLabeler::mimeForImage()reused for PNG/BMP/TIFF/WebP/JPEG Content-TypeCloudAutoLabeler::backupLabelFile()called before overwriting any label filesave_label_data()+ui->label_image->saveState()called before labeling so Ctrl+Z undo worksQImageReader.size()used to get image dimensions without decoding pixels; falls back toQImageonly if size query failsdata[0]checked as array,bounding_boxvalues checked as numeric, coordinates validated in[0,1]rangedone/totalwhere done = completed images (in-flight not counted as done)Test plan
🤖 Generated with Claude Code