Possible to pass existing buffer into quirc?

[FoundationKen]

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!

[dlbeer]

On Wed, Apr 29, 2020 at 11:31:37PM -0700, Ken Carpenter wrote:

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)?

Hi Ken,

That sounds like it'd be a useful addition. If you want to think about implementing it yourself, first have a look at the image and pixels fields and note carefully the different behaviour when QUIRC_MAX_REGIONS is >= 255.

I think it's probably fine to require the user to give you a quirc_pixel_t buffer rather than uint8_t in the case where the user is responsible for memory allocation.

Cheers, Daniel

-- Daniel Beer [email protected] http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

[kaworu]

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.

[dlbeer]

On Thu, Apr 30, 2020 at 05:32:12AM -0700, Alexandre Perrin wrote:

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.

Yes, please preserve ABI compatibility!

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).

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.

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.

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 that you don't want to break ABI compatibility by exposing a type that varies in size between different build configurations. Perhaps we could require the user to query the pixel size at runtime option and allocate/convert as requested (or fail)?

Cheers, Daniel

-- Daniel Beer [email protected] http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

[kaworu]

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);
/* ... */

[dlbeer]

On Fri, May 01, 2020 at 06:44:21AM -0700, Alexandre Perrin wrote:

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.

I'd forgotten about reallocation in quirc_resize. In that case, I think you're right and the user does need to supply a destructor function.

[...] 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.

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.

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);
/* ... */

Something like that seems fine to me.

Cheers, Daniel

-- Daniel Beer [email protected] http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

[kaworu]

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.

[FoundationKen]

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.

[FoundationKen]

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_IMAGE is true. 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 by malloc/free anyway.
• 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 before quirc_begin().
• The w and h parameters to quirc_begin() can be NULL.
• 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 after quirc_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.

[dlbeer]

On Tue, May 05, 2020 at 11:28:52PM -0700, Ken Carpenter wrote:

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_IMAGE is true. 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 by malloc/free anyway.
• 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 before quirc_begin().
• The w and h parameters to quirc_begin() can be NULL.
• 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 after quirc_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.

Hi Ken,

Sorry for the late reply. I guess that sounds broadly conistent, but I think perhaps it might be an idea to make it more restrictive to avoid misuse. 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 take it that quirc_init_with_image() is to be use in place of quirc_init()?

While we're at it, I wonder if it's a good idea to expose struct quirc so that the user can avoid allocating memory for that too? Of course it would be a bad idea to depend on this structure's layout if you're dynamically linking, but I doubt that anyone dynamically linking would need to avoid malloc.

Cheers, Daniel

-- Daniel Beer [email protected] http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

[FoundationKen]

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.

[FoundationKen]

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.

[kaworu]

Hi @FoundationKen,

Couple of comments:

• I don't think we need to use void pointers here. If we expose struct quirc and/or quirc_pixel_t though the interface of the library, it will be less confusing to expose them completely.
• When QUIRC_PIXEL_ALIAS_IMAGE is truthy, then q->image and q->pixels point to the same buffer. Thus, there should be no need for the caller to provide both.

[dlbeer]

On Mon, May 11, 2020 at 02:06:37AM -0700, Alexandre Perrin wrote:

Hi @FoundationKen,

Couple of comments:

• I don't think we need to use void pointers here. If we expose struct quirc and/or quirc_pixel_t though the interface of the library, it will be less confusing to expose them completely.
• When QUIRC_PIXEL_ALIAS_IMAGE is truthy, then q->image and q->pixels point to the same buffer. Thus, there should be no need for the caller to provide both.

I'd also suggest returning 0 vs -1 rather than bool, to be consistent with the other functions.

-- Daniel Beer [email protected] http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

[FoundationKen]

@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]

On Mon, May 11, 2020 at 12:14:05PM -0700, Ken Carpenter wrote:

@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.

Yes, I think so -- as long as it's still possible to use the old API and not depend on their structure for binary compatibility.

-- Daniel Beer [email protected] http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

[kaworu]

@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.

[FoundationKen]

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.

[FoundationKen]

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.

[kaworu]

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!

[FoundationKen]

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.

[dlbeer]

On Sun, May 17, 2020 at 05:23:36PM -0700, Ken Carpenter wrote:

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.

I'll just chip in to say that I agree on this point. Most allocation-avoiding library code I've used and developed for embedded systems follows this convention.

-- Daniel Beer [email protected] http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

[kaworu]

Alright then!