wip: item API suggestions #1027
Conversation
bradh
force-pushed
the
develop-v1.18.0
branch
from
f880057 to
164ca61
Compare
libheif/heif_items.cc
Outdated
| @@ -32,30 +32,30 @@ | |||
|
|
|||
| // ------------------------- reading ------------------------- | |||
|
|
|||
| int heif_context_get_number_of_items(const struct heif_context* ctx) | |||
| size_t heif_context_get_number_of_items(const struct heif_context* ctx) | |||
There was a problem hiding this comment.
I'd like to avoid size_t as much as possible in the interface because that one bit of extra range is not needed here and size_t makes detecting errors much more complicated:
See Scott Meyer
I'm probably fine with size_t when specifying the size of memory areas that may be so large to hit the 2GB boundary on 32bit systems.
There was a problem hiding this comment.
I'm not sure I agree with the analysis. Even in the quoted article, there are plenty of other cases where signed int won't save you (e.g. the off-by-one case).
Internally we're just casting this to size_t anyway, and a common case is to pass in the std::vector size, so I'm casting to int to match the API, then its being cast back...
Possibly we can use size_t and then just check the number isn't greater than some limit, similar to the way we do security checks.
There was a problem hiding this comment.
To get this moving, I've removed the size_t changes.
The unit test still fails.
bradh
force-pushed
the
develop-v1.18.0
branch
from
164ca61 to
08afe5a
Compare
| @@ -216,6 +216,7 @@ struct heif_error heif_context_add_item(struct heif_context* ctx, | |||
|
|
|||
| if (result && out_item_id) { | |||
| *out_item_id = result.value; | |||
| ctx->context->get_heif_file()->set_primary_item_id(*out_item_id); | |||
There was a problem hiding this comment.
But that will overwrite any other primary item id that was set before whenever a new item is added, right?
There was a problem hiding this comment.
And only if out_item_id is not NULL, which is quite unexpected.
|
The core issue is probably that libheif was designed to write standard image files and not to build arbitrary files (like your new test) from scratch. This probably has to be refactored from the ground up to get a clean solution. |
farindk
force-pushed
the
develop-v1.18.0
branch
from
2b0c2ab to
f7cb6f5
Compare
I think that is a valid design choice. Will close this. |
The unit test currently fails - see #1002 (comment)