Skip to content

Add NCLX item property reader - #1892

Open
gregbenz wants to merge 1 commit into
strukturag:v1.24.xfrom
gregbenz:codex/add-item-nclx-property-reader
Open

Add NCLX item property reader#1892
gregbenz wants to merge 1 commit into
strukturag:v1.24.xfrom
gregbenz:codex/add-item-nclx-property-reader

Conversation

@gregbenz

@gregbenz gregbenz commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Problem

The generic raw-property getter cannot read a recognized colr property after libheif parses it
into a typed box. The existing image-profile getters also require an image handle, so callers
cannot retrieve NCLX signaling attached to a non-image derived item.

Fix

Add heif_item_get_property_nclx_color_profile() to read an NCLX profile from a specific item
property. The returned profile uses the existing allocated-profile ownership convention and must
be released with heif_nclx_color_profile_free(). Invalid properties return an error and clear the
output pointer. As with the existing typed-property getters, property ID 0 selects the first
matching colr property.

Tests

Locate the parsed colr property on the existing AVIF fixture without assuming a property index,
verify that the raw-property getter rejects it, and check the exact NCLX values including full-range
signaling. Also discover an ispe property and verify that the new accessor rejects it while
resetting the output pointer. Finally, verify that an out-of-range property ID returns
heif_error_Usage_error / heif_suberror_Invalid_property and clears the output pointer.

@gregbenz
gregbenz force-pushed the codex/add-item-nclx-property-reader branch from 0c601d0 to 149d924 Compare August 27, 2026 02:03
@gregbenz
gregbenz marked this pull request as ready for review August 27, 2026 02:10

@fallenmi fallenmi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed exact head 149d9249b39616193bff9820eff60566143cfaf0 against exact base/current master 2bc82b493dd8896fab3226f01977c7ac9d2ea3b8; I found no blocking issue.

The new accessor follows the existing allocated-NCLX ownership path, clears the caller's pointer on every tested error, and the added return fixes the shared typed-property bounds check before indexing. An independent public-ABI oracle on examples/example.avif read the expected NCLX values (cp=2, tc=2, mc=6, full range) by both the explicit property ID and property ID 0. It also verified exact error behavior for a non-colr property, UINT32_MAX, an invalid item, and null arguments. On the existing ICC-only tests/data/rainbow-451x461.heic, both access routes return Color_profile_does_not_exist and leave the output null.

The causal base comparison is clean: the new exported symbol is absent there, and the same out-of-range call through the pre-existing raw-property API reaches the missing-return bug and terminates with SIGBUS; exact head and GitHub's synthetic merge 345f3b433fdcc323d3b53264fc98b79678f30906 instead return Usage_error / Invalid_property. The submitted item_properties test passed ten consecutive runs on both head and merge. The related image_description_metadata and entity_groups tests passed three runs on base, head, and merge; extended_type was skipped under the deliberately codec-minimal local build. git diff --check is clean, and all 41 published checks are green.

This review was prepared with OpenAI Codex assistance under the account owner's authorization; I built and exercised the exact revisions and verified the code paths, outputs, policy, live head/base/merge state, CI, and interaction state.

@gregbenz
gregbenz force-pushed the codex/add-item-nclx-property-reader branch from 149d924 to 078e7ce Compare September 9, 2026 04:13
@gregbenz
gregbenz changed the base branch from master to v1.24.x September 9, 2026 04:13
@gregbenz
gregbenz force-pushed the codex/add-item-nclx-property-reader branch from 078e7ce to a5b216d Compare September 9, 2026 04: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.

2 participants