Fix vxMinMaxLoc minCount/maxCount scalar type to VX_TYPE_SIZE#68
Merged
Conversation
The OpenVX 1.3 specification requires the minCount/maxCount parameters of vxMinMaxLocNode to be VX_TYPE_SIZE scalars, but the sample implementation produced them as vx_uint32 and its output validator declared them as VX_TYPE_UINT32. On LP64 platforms vx_size is 8 bytes while vx_uint32 is 4, so when a spec-conformant client supplies a VX_TYPE_SIZE count scalar, vxCopyScalar performs an 8-byte read from a 4-byte source, corrupting the returned count with adjacent stack bytes. This makes MinMaxLoc.OnRandom conformance tests fail against the impl. Change the count locals/helpers in c_model and venum statistics kernels from vx_uint32 to vx_size, and set the MinMaxLoc output validator (params 5/6) to VX_TYPE_SIZE in both the c_model and venum targets. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
This PR aligns the sample implementation of vxMinMaxLocNode with the OpenVX 1.3 spec by making the {min,max}Count scalar outputs use VX_TYPE_SIZE end-to-end, preventing corrupted results on LP64 platforms when clients supply spec-correct VX_TYPE_SIZE scalars.
Changes:
- Updated
MinMaxLocoutput validators (c_model + venum targets) to declare params 5/6 asVX_TYPE_SIZEinstead ofVX_TYPE_UINT32. - Updated c_model and venum MinMaxLoc implementations to compute and store
minCount/maxCountinvx_size(and to passvx_size*through helper functions), matching whatvxCopyScalarexpects forVX_TYPE_SIZE. - Kept NEON/count-accumulation internals in
uint32where appropriate, only widening at the boundary where results are written out.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| sample/targets/venum/vx_minmaxloc.c | Output validator now enforces VX_TYPE_SIZE for min/max count scalar outputs. |
| sample/targets/c_model/vx_minmaxloc.c | Output validator now enforces VX_TYPE_SIZE for min/max count scalar outputs. |
| kernels/venum/venum_statistics.c | Min/max count computation and plumbing widened to vx_size to match VX_TYPE_SIZE scalars. |
| kernels/c_model/c_statistics.c | Min/max count computation and plumbing widened to vx_size to match VX_TYPE_SIZE scalars. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Point the bundled conformance suite at the OpenVX-cts branch that creates the MinMaxLoc minCount/maxCount scalars (and the MatchTemplate test's internal MinMaxLoc usage) as VX_TYPE_SIZE, matching this implementation's spec-conformant kernel change. Without this bump the pinned CTS still supplies VX_TYPE_UINT32 count scalars, which now fail graph verification against the corrected validator. Co-authored-by: Cursor <cursoragent@cursor.com>
ownDestructGraph removed the graph's nodes but never dereferenced virtual objects (images, tensors, etc.) whose scope is the dying graph, leaking memory in loops that repeatedly create graphs and virtual data. After removing the nodes, iterate the context reference table and release any virtual reference scoped to this graph. Ports the fix from PR #60 to openvx_1.3.2's ownDestructGraph, which was the only PR #60 code change missing from tip. Co-authored-by: Cursor <cursoragent@cursor.com>
)" This reverts commit ea6db34.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The OpenVX 1.3 specification requires the
minCountandmaxCountparameters ofvxMinMaxLocNodeto beVX_TYPE_SIZEscalars:The sample implementation instead:
vx_uint32locals in thec_modelandvenumstatistics kernels, andVX_TYPE_UINT32in theMinMaxLocoutput validators.On LP64 platforms (Linux/macOS/ARM64)
vx_sizeis 8 bytes whilevx_uint32is 4. When a spec-conformant client supplies aVX_TYPE_SIZEcount scalar,vxCopyScalardispatches on the scalar's declared type and performs an 8-byte read from a 4-byte source, so the returned count is corrupted with adjacent stack bytes. This makes theMinMaxLoc.OnRandomconformance tests fail against the implementation once the CTS is updated to use the spec-correctVX_TYPE_SIZEscalar.Changes
kernels/c_model/c_statistics.c:analyzeMinMaxValuecount params andiMinCount/iMaxCountlocalsvx_uint32->vx_size.kernels/venum/venum_statistics.c: same foranalyzeMinMaxValue,calcMinMaxLocu8,calcMinMaxLocs16and the outervxMinMaxLoc(internal NEON accumulators stayuint32; only the value carried tovxCopyScalarwidens).sample/targets/c_model/vx_minmaxloc.candsample/targets/venum/vx_minmaxloc.c:MinMaxLocoutput validator for params 5/6VX_TYPE_UINT32->VX_TYPE_SIZE.The framework node/vxu wrappers only pass the user-provided scalars through, so no other changes are required.
Test plan
*MinMaxLoc*conformance tests from a CTS checkout that creates the count scalars asVX_TYPE_SIZE(see Fix vxMinMaxLocNode minCount/maxCount scalar type to VX_TYPE_SIZE (#29) OpenVX-cts#39), on x86_64 and ARM64.Made with Cursor