Add quirc_end_with_workarea() #96
Conversation
|
btw, i'm not quite happy with the name |
lib/identify.c
Outdated
| { | ||
| struct flood_fill_vars stack[FLOOD_FILL_MAX_DEPTH]; | ||
|
|
||
| quirc_end_with_workarea(q, (void *)stack, sizeof(stack)); |
There was a problem hiding this comment.
nit: casting to void * here seems unnecessary.
lib/identify.c
Outdated
| span_func_t func, void *user_data, | ||
| const struct workarea *wa) | ||
| { | ||
| struct flood_fill_vars *stack = wa->p; |
There was a problem hiding this comment.
wa->p could have been be provided by the caller of quirc_end_with_workarea and thus we have no guarantee that it is compatible with struct flood_fill_vars * (alignment, pointer type).
* add a new variant of quirc_end() to allow users to specify
the working memory for the flood filling logic
* motivation: the dynamic stack usage with recursion is not
easy to deal with. (for me)
eg. malloc/free is easier than launching a temporary thread
with a large stack.
* implementation:
* make flood filling logic iterative. (vs recursive)
i basically tried one-to-one conversions here to avoid mistakes.
probably it has a room for later improvements.
* passes around the user-specified working memory for flood fill
* possible downsides:
* might have a negative impact on architectures with fast stack.
eg. something with register-window
* might have a negative impact on demand-paged stack
|
How big does a workarea need to be for the library to behave well on typical images? I'm guessing it's of the order of a thousand-ish entries? I'm very much in favour of removing the recursion, but I wonder if the workarea could just be allocated with a static fixed (or #define'd) size in struct quirc? We already have a static buffer for grid decoding, and I'm guessing this wouldn't add significantly to the size. |
i was assuming that FLOOD_FILL_MAX_DEPTH was the value for the typical images. wasn't it?
depends on the typical size, it might make sense. |
|
|
ok.
some of my colleagues might have some experiment results about this. let me ask them. |
unfortunately, they don't seem to have much info to share. |
|
reading at the code, i think that the max depth should be something like (image_height_in_pixels / 3 * 2).
as it's a proportion to the image resolution, i guess there's no one-size-fit-all value of the max depth. |
i did quick experiments around this value and it seems working as i expected. i guess it makes sense to make quirc_resize() allocate the buffer with this size automatically using malloc(). |
| { | ||
| /* ensure the alignment */ | ||
|
|
||
| size_t align = sizeof(int); /* alignof(**stackp) */ |
There was a problem hiding this comment.
This doesn't seem to be a safe assumption to me, can you elaborate?
There was a problem hiding this comment.
the structure only contains several int members.
i'm not aware of any arch where this is not safe.
| uintptr_t p = (uintptr_t)wa->p; | ||
| uintptr_t aligned_p = (p + align - 1) / align * align; | ||
|
|
||
| *stackp = (struct flood_fill_vars *)aligned_p; |
There was a problem hiding this comment.
Even assuming proper alignment, formally we don't know whether the pointer is compatible with struct flood_fill_vars * correct?
There was a problem hiding this comment.
if you mean C-language lawyer level of "formal", i guess you are right.
i submitted an alternative version which do this. |
|
i haven't noticed any issues with the alternative (#100) so far. |
the working memory for the flood filling logic
easy to deal with. (for me)
eg. malloc/free is easier than launching a temporary thread
with a large stack.
i basically tried one-to-one conversions here to avoid mistakes.
probably it has a room for later improvements.
eg. something with register-window