Skip to content

refactor: use sam3_graph_compute in sam2_encode_image_hiera - #31

Merged
PABannier merged 1 commit into
mainfrom
cleanup/use-graph-compute-helper
Sep 16, 2026
Merged

PABannier merged 1 commit into
mainfrom
cleanup/use-graph-compute-helper

Conversation

@PABannier

@PABannier PABannier commented Sep 11, 2026 •

Copy link
Copy Markdown
Owner

sam2_encode_image_hiera open-coded exactly what sam3_graph_compute does: set the CPU thread count when the backend is CPU, run ggml_backend_graph_compute, and check the status. Every other stage in sam3.cpp already goes through the helper. The helper also logs graph compute failed: <status>, so the caller's own error print at this call site is dropped.

Rebased onto main. The earlier conflict was with #27 (RAII contexts/allocators), which had already removed the manual ggml_gallocr_free/ggml_free lines this diff sat on; the failure path is now just return false;.

Verification: a parity tool runs encode, PVS point + box, and a 3-frame visual track, then compares the output byte for byte with main. The SAM2.1 runs go through this function.

PARITY OK   edgetam_f16 gpu0
PARITY OK   edgetam_f16 gpu1
PARITY OK   sam2.1_hiera_tiny_q8_0 gpu0
PARITY OK   sam2.1_hiera_tiny_q8_0 gpu1

A Release build with -DSAM3_BUILD_TESTS=ON has no errors or new warnings.

git diff --shortstat main: 1 file changed, 1 insertion(+), 5 deletions(-).

Not fixed here, to keep the diff as reviewed: sam3_encode_vit_from_preprocessed_selective (~sam3.cpp:7128) still open-codes the same pattern, inside the test/debug region that #27 skipped.

Part of an over-engineering cleanup series.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M2qLmQ9ag49hT61V6i9qAq

Comment thread sam3.cpp
@@ -5938,11 +5938,7 @@ static bool sam2_encode_image_hiera(sam3_state& state,
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the merge conflicts

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased onto main; conflict resolved and force-pushed. GitHub now reports the PR as mergeable.

The conflict was with #27 (RAII contexts/allocators), which had already deleted the manual ggml_gallocr_free(galloc); ggml_free(ctx0); lines this diff sat on top of. Resolution keeps #27's version, so the failure path is just return false;:

    // Compute
    if (!sam3_graph_compute(model.backend, graph, state.n_threads)) {
        return false;
    }

The diff is now 1 insertion, 5 deletions in sam2_encode_image_hiera — the thread-count setup, the status check and the duplicate error print, all of which sam3_graph_compute already does (it also logs the status string).

Verified: encode, PVS point + box, and a 3-frame visual track match main byte for byte on EdgeTAM f16 and SAM2.1-tiny q8_0, CPU and Metal (PARITY OK ×4); the SAM2.1 runs go through this function. A Release build with -DSAM3_BUILD_TESTS=ON has no errors or new warnings.

One note for later, left out to keep this diff as reviewed: sam3_encode_vit_from_preprocessed_selective (~sam3.cpp:7128) still open-codes the same pattern. It's in the test/debug region that #27 skipped, so it kept its manual frees and needs a slightly different edit.

sam2_encode_image_hiera hand-rolled the CPU thread-count setup and
ggml_backend_graph_compute status check that sam3_graph_compute already
does. The helper also logs the status string, so the caller's
duplicate error print goes too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M2qLmQ9ag49hT61V6i9qAq
@PABannier
PABannier force-pushed the cleanup/use-graph-compute-helper branch from de3bbb5 to e49a2c1 Compare September 16, 2026 19:09
@PABannier
PABannier merged commit 416186c into main Sep 16, 2026
3 checks passed
@PABannier
PABannier deleted the cleanup/use-graph-compute-helper branch September 16, 2026 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant