Possible to pass existing buffer into quirc?
FoundationKen opened this issue · 22 comments
I got quirc running on a small embedded target quite easily (nice job on the example code!), but at the moment I am only able to scan smaller QR codes because I don't have enough memory to allocate two copies of the image buffer.
My data is already in the same format that quirc requires, and looking at the code it seems like it would be easy to pass an extra (optional) parameter to quirc_resize(q,w,h,data) and use that buffer instead of calling malloc(). The onus is then on the caller to make sure the w, h, data match up, and to not call quirc_resize() again or to call it with a data parameter again, and be responsible for freeing the old one.
Does this seem like something you'd consider adding (or accept a PR for)?
Thanks!
Hello @FoundationKen,
I also believe it would be a useful addition. I would rather see this feature implemented as a new function (as opposed to changing quirc_resize) for simplicity's sake but also to avoid breaking the current API.
How the newly user-provided buffer should be freed is an open question. The usual tricks include giving an additional flag telling whether the pointer should be passed to free(3), or giving an additional free func pointer (useful with custom memory allocators).
Finally, I would advise against exposing quirc_pixel_t (it is currently an implementation detail of quirc) for the reasons listed in #9. My guess is memory constrained systems don't set QUIRC_MAX_REGIONS > UINT8_MAX anyway, and so they avoid the overhead of the pixels buffer.
You could require that the user dispose of it after destroying the quirc
object? If quirc didn't allocate it, then perhaps it shouldn't free it
either.
Yes that is also an option, and the simplest from the library's perspective. The issue I have with it is that the burden is on the user to dispose of the buffer at the right time and in the right way.
The right time is an implementation detail of the library (quirc_destroy is obvious, quirc_resize less so). If the struct quirc pointer is returned (say from a constructor function) the buffer has to be returned as well.
The right way is easy when you have exactly one "object" to track (or always the same storage / allocator) but can quickly become a quagmire.
tl;dr I just like the free func param from a library design's perspective and because I feel it is harder to misuse, but that's only my two cents.
[...] Probably true, but then if QUIRC_MAX_REGIONS >= UINT8_MAX, the feature
becomes useless. That's probably rare, but it would be nice if the two
options didn't interact.
I agree, but keep in mind that in addition the caller must have its pixels bytes actually stored as uint16_t which IMHO is highly unlikely. In the case where the caller has to convert to the quirc_pixel_t representation then malloc(3) is probably involved and the point is moot.
If we're going that route, an option would be an additional parameter to the new function with the caller's sizeof(*data) value. Then, we could fail when it doesn't match the expected sizeof(quirc_pixel_t). Some ideas:
/* returns 0 on success, -1 on failure. */
int quirc_set_pixels(struct quirc *q, int w, int h, void *buffer, size_t elemsiz);if (quirc_set_pixels(q, width, height, buffer, sizeof(*buffer)) < 0) {
/* fallback to the usual quirc_resize() / quirc_begin() workflow,
let quirc handle the memory and representation conversion dance */
}
quirc_end(q);
/* ... */16-bit pixel formats like RGB565 are pretty common in embedded systems.
It's also common for cameras to return 16 bpp YUYV data. You can convert
either of those in-place to a 0-255 level easily.
Thanks I did not know that! Then support for this feature combined with QUIRC_MAX_REGIONS >= UINT8_MAX make sense to me now as well.
Thanks for the feedback. We're considering adding more RAM to the system, but I'd still like to use less memory either way, so I'll try to dig into this in the next week or two.
My proposal is the following:
- Create a new function
quirc_init_with_image(q,w,h,image)(open to name suggestions). - Assert or perform runtime check that
QUIRC_PIXEL_ALIAS_IMAGEistrue. As @kaworu suggested, anyone using this method is probably on a device with limited memory, so this is likely the case. - Put the onus on the caller to ensure that the buffer lasts until
quirc_destroy()is called. In my case, the buffer is allocated at startup and used whenever scanning is required. If we expand the memory on our device, I'll actually bake the buffer address into the memory map, so it wouldn't be managed bymalloc/freeanyway. - It is not possible to "resize" the buffer, however, the user can call
quirc_init_with_image()again with a different buffer and size if desired (even reusing the same buffer pointer if it's big enough for the new size). - For the reasons in the last two bullets, it doesn't make sense to provide or require a destructor function. In the embedded environment you just have the small responsibility of ensuring the buffer outlives
quirc_destroy(). quirc_init_with_image()must be called beforequirc_begin().- The
wandhparameters toquirc_begin()can beNULL. - The buffer returned by
quirc_begin()can be ignored (you already have it). - If you want to mix and match, it's OK to call
quirc_resize()before or afterquirc_init_with_image()and it will handle the allocation/freeing correctly (free only buffers allocated by quirc and never free a user owned buffer).
I found some time to implement this tonight and it's working, so if we can agree on the above and on a name, then I can make a PR tomorrow.
In my sample code, it's called instead of quirc_resize(), which is the first time you specify the image size in the current way of things.
If we want to allow users to avoid malloc() entirely, then we could change it so that instead of calling quirc_new(), you pass buffers for all the dynamically allocated bits to a new function:
void quirc_init(quirc* quirc, int w, int h, void* image, quirc_pixel_t* pixels);
To keep it simple, you would need to pass pointers for all three buffers (otherwise we would have a weird mix of some buffers from the user and some from malloc()). A side effect of this would unfortunately mean that you would have to #include quirc_internal.h in your code so you could use sizeof(quirc) and sizeof(quirc_pixel_t) to ensure the buffers you supply for them are large enough (or include those definitions in quirc.h).
The quirc struct could have a flag like quirc_owns_buffers or something to indicate which allocation mode we used, so we could avoid calling free() anywhere and could trigger an error in quirc_resize() if the user called it.
As a partial review so we're not talking in abstracts anymore, here is the main code that I wrote:
/* You will call this instead of quirc_new() */
bool quirc_init(void* quirc, int w, int h, void* image, void* pixels) {
struct quirc* q = (struct quirc*)quirc;
if (!q || !image || q->quirc_owns_buffers || !QUIRC_PIXEL_ALIAS_IMAGE) {
return false;
}
memset(q, 0, sizeof(*q));
q->w = w;
q->h = h;
q->image = image;
q->pixels = (quirc_pixel_t*)pixels;
q->quirc_owns_buffers = false;
return true;
}
/* Expose sizes without revealing the internal structure */
int get_quirc_size() {
return sizeof(struct quirc);
}
int get_quirc_pixel_size() {
return sizeof(quirc_pixel_t);
}
There are a couple of other changes to set and check the q->quirc_owns_buffers flag.
Hi @FoundationKen,
Couple of comments:
- I don't think we need to use void pointers here. If we expose
struct quircand/orquirc_pixel_tthough the interface of the library, it will be less confusing to expose them completely. - When
QUIRC_PIXEL_ALIAS_IMAGEis truthy, thenq->imageandq->pixelspoint to the same buffer. Thus, there should be no need for the caller to provide both.
@dlbeer Are you OK with exposing struct quirc and quirc_pixel_t? If so, I'll revise and submit a PR that we can iterate on tonight.
@dlbeer @FoundationKen in your embeded project, do you ever only use one struct quirc object?
If yes, what do you think of having one global struct quirc object in the library exposed to the user? That way the library could provide a struct quirc without dynamic memory allocation nor exposing the internals. Users for which a single quirc object is not enough probably don't mind calling malloc nor the overhead of a global struct quirc in the library.
Yes, I allocate a single struct quirc once at startup (and a single image buffer that gets reused).
So if I understand your suggestion, I would remove the quirc parameter from quirc_init(), and just return the internal static quirc instance.
This would be preferable to me since it would remove the requirement of including quirc_internal.h in my code and allocating that memory.
The only downside I can think of is if struct quirc ever needs any dynamic allocation in the future, then that would still need to be done on the global instance.
So if I understand your suggestion, I would remove the quirc parameter from quirc_init(), and just return the internal static quirc instance.
That is not what I had in mind. I think one should be able to pass the static instance to quirc_init() or a dynamically allocated one.
More generally, the vibe I am feeling in this thread about the library usage is all-in dynamic memory allocation using quirc_new() then quirc_resize() versus no malloc at all with quirc_init(). This is fair: the first case is the "workstation" and the second case is embeded. From a practical stand-point there are probably few cases in-between.
With that said, I don't think the library should artificially have the two "paths" incompatible with each other. In other words, I don't see why quirc_new() then quirc_init() should not be allowed. Also using quirc_resize with the global instance should be fine too. This is not only about the few cases in-between. This is about exposing a robust API and avoiding adding new error conditions.
It seems to me that freeing the member buffers is only about remembering if we were initialized with quirc_init() vs quirc_resize(). Freeing the struct quirc itself looks straightforward: we call free(3) unless we're the global instance.
Feel free to correct me if I am missing something and thanks again for the hard work!
Maybe I'm still missing something, but the API for quirc_init() in this PR doesn't care if you statically allocated the memory or dynamically allocated it. I don't think it makes sense to add the overhead of a static quirc instance to all users of quirc just for the use of embedded developers, who can just as easily allocate a static instance themselves. Whether static or dynamic, it's up to the caller (not quirc) to decide whether/when to free it. In quirc_init(), it just sets a flag to tell quirc that it doesn't own the memory, so that quirc leaves it alone.
Regarding being able to use quirc_new(), quirc_init(), quirc_destroy() and quirc_resize(), that was what I originally implemented. You even could quirc_resize() as long as your buffer was big enough.
In response, @dlbeer suggested, "I think if the buffer is going to be managed by the user, perhaps it would be a better idea to forbid the use of quirc_resize() and have it return an error."
I can go back to the previous way and put up those changes if we can agree on being able to call all methods. I think it was a "no surprises" API. If you started with quirc_init(), you can call all quirc methods although quirc_destroy() would be a no-op. In fact, you can also call quirc_new() and get a dynamic instance alongside your "embedded" instance if you like.
Alright then!