Skip to content

Conversation

bradh
Copy link
Contributor

@bradh bradh commented Jul 22, 2025

I went back-and-forward a bit on whether adding and getting should take the item or the item id. Would appreciate feedback on that.

@bradh bradh force-pushed the text_elng_2025-07-22 branch from 86fb4ca to 18bb58d Compare July 22, 2025 11:22
@farindk
Copy link
Contributor

farindk commented Jul 22, 2025

I went back-and-forward a bit on whether adding and getting should take the item or the item id. Would appreciate feedback on that.

Yes, I can understand that. My first intuition would be to also use heif_text_item* for heif_item_get_property_extended_language(), similar to heif_error heif_text_item_add_extended_language().
That view might change if the elng box can also be assigned to other items (e.g. alternative images in different languages). I don't have the new standard yet, so I do not know whether elng is intended for that use too.

Another point: I would move all logic from the api/heif_*.cc file to the main library. Keep the logic in the core and only have C-type conversions in the api directory. I know that this is a bit of a mess currently, but if we try to separate it like this, it will be easier in the future to find the actual logic.

If you want, I can merge this and do the refactoring. Or you may do it if you prefer. Just let me know.

@bradh
Copy link
Contributor Author

bradh commented Jul 23, 2025

elng is only shown as a text item property in ISO 23008-12:2025. However the detail just comes from 14496-12, where it can be attached to a track.

Outside of specification conformance, I can think of a case where you might want to use it in a layered image, where the base layer is a photograph and there is a derived image using that base plus a selected labelling layer (with alpha) composited over the base layer. @dukesook have you looked at a photomap use case?

@farindk
Copy link
Contributor

farindk commented Jul 23, 2025

Outside of specification conformance, I can think of a case where you might want to use it in a layered image, where the base layer is a photograph and there is a derived image using that base plus a selected labelling layer (with alpha) composited over the base layer.

This may even be useful without a layered composition. I often have to handle software screenshots for different UI languages. These could then be combined into one file. For such a use, it might make sense to use an "alternative group" (altr) for language variants.

@bradh
Copy link
Contributor Author

bradh commented Jul 24, 2025

This may even be useful without a layered composition. I often have to handle software screenshots for different UI languages. These could then be combined into one file. For such a use, it might make sense to use an "alternative group" (altr) for language variants.

So I think we should allow for that. If you agree, I will update the interface to use a heif_item_id for both adding and getting. Potentially setting that property on a non-text item could be behind an experimental compile time flag, but as it would not be essential, then I do not see an issue with allowing it anywhere.

@farindk
Copy link
Contributor

farindk commented Jul 24, 2025

If you agree, I will update the interface to use a heif_item_id for both adding and getting.

Ok.

@dukesook
Copy link
Contributor

dukesook commented Jul 24, 2025

elng is only shown as a text item property in ISO 23008-12:2025. However the detail just comes from 14496-12, where it can be attached to a track.

Outside of specification conformance, I can think of a case where you might want to use it in a layered image, where the base layer is a photograph and there is a derived image using that base plus a selected labelling layer (with alpha) composited over the base layer. @dukesook have you looked at a photomap use case?

Sorry, no. I'm not familiar with this.

if (auto img = text_item->context->get_image(text_item->text_item->get_item_id(), false)) {
auto existing_elng = img->get_property<Box_elng>();
if (existing_elng) {
return {heif_error_Usage_error, heif_suberror_Invalid_parameter_value, "item already has an elng property"};
Copy link
Contributor

Choose a reason for hiding this comment

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

How about replacing the existing elng?
Currently, we do not allow editing existing files, but if we will, replacing the value would be the expected behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally tried removing the existing elng and adding a new one. That turned out to be very messy because it requires updating the property associations.

So I made the choice to update the text. However it might be unexpected that updating the text could affect another item with the same original text (because de-duplication).

@bradh bradh force-pushed the text_elng_2025-07-22 branch from 18bb58d to a1de57a Compare July 30, 2025 11:06
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.

3 participants