Improve stack usage for flood filling #100

yamt · 2021-04-05T00:53:21Z

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 optimizations.
Use explicit malloc (vs variables on stack) to allocate the work area.
Estimate the amount of memory for the work area dynamically
from the image size, instead of using a constant FLOOD_FILL_MAX_DEPTH,
which is too big in the most cases.

kaworu

Hello @yamt and thanks for the PR. Honestly I'm not smart enough to understand the goto flow into various loops, so I'll defer to @dlbeer here.

kaworu · 2021-04-16T18:21:22Z

lib/identify.c

-	if (depth >= FLOOD_FILL_MAX_DEPTH)
-		return;
+	/* Set up the first context  */
+	next_vars = &stack[0];


nitpicking:

Suggested change

next_vars = &stack[0];

next_vars = stack;

kaworu · 2021-04-16T18:24:23Z

lib/quirc.c

+	 */
+
+	num_vars = h * 2 / 3;
+	vars = malloc(sizeof(*vars) * num_vars);


any reason to use malloc instead of calloc here?

just because it doesn't need to be initialized.

uninitialized is fine, I meant for the overflow check safety net.

i added a few checks. is this what you meant?

dlbeer · 2021-04-18T22:34:37Z

Apologies for the delay in getting to this. I think you might end up with a better result by implementing the span-based flood-fill directly with a simpler stack structure (you probably only need to keep track of fixed y, and incrementing x for each span) and a loop that examines the top of the stack looking for new seeds to push. I understand that this is a direct translation of the recursive procedure, and I think that's probably led to a lot of unnecessary book-keeping.

yamt · 2021-04-18T23:09:52Z

Apologies for the delay in getting to this. I think you might end up with a better result by implementing the span-based flood-fill directly with a simpler stack structure (you probably only need to keep track of fixed y, and incrementing x for each span) and a loop that examines the top of the stack looking for new seeds to push. I understand that this is a direct translation of the recursive procedure, and I think that's probably led to a lot of unnecessary book-keeping.

i completely agree.
i don't want to do it in this PR though.

yamt · 2021-05-11T01:34:36Z

Apologies for the delay in getting to this. I think you might end up with a better result by implementing the span-based flood-fill directly with a simpler stack structure (you probably only need to keep track of fixed y, and incrementing x for each span) and a loop that examines the top of the stack looking for new seeds to push. I understand that this is a direct translation of the recursive procedure, and I think that's probably led to a lot of unnecessary book-keeping.

i completely agree.
i don't want to do it in this PR though.

@dlbeer
do you want me to do it within this PR?
generally, i don't think it's a good idea to do too many things in a single PR.
but i can do that if it's considered as a blocker of this PR.

dlbeer · 2021-05-11T01:41:04Z

On Mon, May 10, 2021 at 06:34:53PM -0700, YAMAMOTO Takashi wrote: > > Apologies for the delay in getting to this. I think you might end up with a better result by implementing the span-based flood-fill directly with a simpler stack structure (you probably only need to keep track of fixed y, and incrementing x for each span) and a loop that examines the top of the stack looking for new seeds to push. I understand that this is a direct translation of the recursive procedure, and I think that's probably led to a lot of unnecessary book-keeping. > > i completely agree. > i don't want to do it in this PR though. @dlbeer do you want me to do it within this PR? generally, i don't think it's a good idea to do too many things in a single PR. but i can do that if it's considered as a blocker of this PR.

Well, unless you wanted to create a new one. I like what you're trying to do here, but I think the mechanical translation from a recursive algorithm is a bit too hard to follow and modify.

…

-- Daniel Beer ***@***.***> http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

yamt · 2021-05-11T02:03:43Z

On Mon, May 10, 2021 at 06:34:53PM -0700, YAMAMOTO Takashi wrote: > > Apologies for the delay in getting to this. I think you might end up with a better result by implementing the span-based flood-fill directly with a simpler stack structure (you probably only need to keep track of fixed y, and incrementing x for each span) and a loop that examines the top of the stack looking for new seeds to push. I understand that this is a direct translation of the recursive procedure, and I think that's probably led to a lot of unnecessary book-keeping. > > i completely agree. > i don't want to do it in this PR though. @dlbeer do you want me to do it within this PR? generally, i don't think it's a good idea to do too many things in a single PR. but i can do that if it's considered as a blocker of this PR.
Well, unless you wanted to create a new one. I like what you're trying to do here, but I think the mechanical translation from a recursive algorithm is a bit too hard to follow and modify.

i want to work on it. (just expressing honest intention. no promise)
i want to do it in a separate PR.
if this PR will not likely be merged anytime soon, i can add it as separate commits to this PR.

* 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 optimizations. * Use explicit malloc (vs variables on stack) to allocate the work area. * Estimate the amount of memory for the work area dynamically from the image size, instead of using a constant FLOOD_FILL_MAX_DEPTH, which is too big in the most cases.

Also, avoid malloc(0), which is not too portable.

yamt · 2021-05-11T02:08:49Z

btw,

you probably only need to keep track of fixed y, and incrementing x for each span

do you mean to have a single "y" var?
i don't think it works for complex shapes.

dlbeer · 2021-05-11T02:30:32Z

On Mon, May 10, 2021 at 07:09:07PM -0700, YAMAMOTO Takashi wrote: btw, > you probably only need to keep track of fixed y, and incrementing x for each span do you mean to have a single "y" var? i don't think it works for complex shapes.

No, I meant that each span would need a y value and a next x-value (in other words, each stack level).

…

-- Daniel Beer ***@***.***> http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

dlbeer · 2021-05-11T02:32:47Z

On Mon, May 10, 2021 at 07:04:01PM -0700, YAMAMOTO Takashi wrote: > On Mon, May 10, 2021 at 06:34:53PM -0700, YAMAMOTO Takashi wrote: > > Apologies for the delay in getting to this. I think you might end up with a better result by implementing the span-based flood-fill directly with a simpler stack structure (you probably only need to keep track of fixed y, and incrementing x for each span) and a loop that examines the top of the stack looking for new seeds to push. I understand that this is a direct translation of the recursive procedure, and I think that's probably led to a lot of unnecessary book-keeping. > > i completely agree. > i don't want to do it in this PR though. @dlbeer do you want me to do it within this PR? generally, i don't think it's a good idea to do too many things in a single PR. but i can do that if it's considered as a blocker of this PR. > Well, unless you wanted to create a new one. I like what you're trying to do here, but I think the mechanical translation from a recursive algorithm is a bit too hard to follow and modify. * i want to work on it. (just expressing honest intention. no promise) * i want to do it in a separate PR. * if this PR will not likely be merged anytime soon, i can add it as separate commits to this PR.

Are you talking about doing a separate PR with just a direct replacement of the existing code with a simpler iterative implementation? If that's what you mean, then I'd definitely be happy to merge such a PR.

…

-- Daniel Beer ***@***.***> http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

yamt · 2021-05-11T02:59:02Z

On Mon, May 10, 2021 at 07:04:01PM -0700, YAMAMOTO Takashi wrote: > On Mon, May 10, 2021 at 06:34:53PM -0700, YAMAMOTO Takashi wrote: > > Apologies for the delay in getting to this. I think you might end up with a better result by implementing the span-based flood-fill directly with a simpler stack structure (you probably only need to keep track of fixed y, and incrementing x for each span) and a loop that examines the top of the stack looking for new seeds to push. I understand that this is a direct translation of the recursive procedure, and I think that's probably led to a lot of unnecessary book-keeping. > > i completely agree. > i don't want to do it in this PR though. @dlbeer do you want me to do it within this PR? generally, i don't think it's a good idea to do too many things in a single PR. but i can do that if it's considered as a blocker of this PR. > Well, unless you wanted to create a new one. I like what you're trying to do here, but I think the mechanical translation from a recursive algorithm is a bit too hard to follow and modify. * i want to work on it. (just expressing honest intention. no promise) * i want to do it in a separate PR. * if this PR will not likely be merged anytime soon, i can add it as separate commits to this PR.
Are you talking about doing a separate PR with just a direct replacement of the existing code with a simpler iterative implementation? If that's what you mean, then I'd definitely be happy to merge such a PR.

i meant a separate PR on the top of this one.

…
-- Daniel Beer @.***> http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

dlbeer · 2021-05-11T03:09:10Z

On Mon, May 10, 2021 at 07:59:21PM -0700, YAMAMOTO Takashi wrote: > On Mon, May 10, 2021 at 07:04:01PM -0700, YAMAMOTO Takashi wrote: > On Mon, May 10, 2021 at 06:34:53PM -0700, YAMAMOTO Takashi wrote: > > Apologies for the delay in getting to this. I think you might end up with a better result by implementing the span-based flood-fill directly with a simpler stack structure (you probably only need to keep track of fixed y, and incrementing x for each span) and a loop that examines the top of the stack looking for new seeds to push. I understand that this is a direct translation of the recursive procedure, and I think that's probably led to a lot of unnecessary book-keeping. > > i completely agree. > i don't want to do it in this PR though. @dlbeer do you want me to do it within this PR? generally, i don't think it's a good idea to do too many things in a single PR. but i can do that if it's considered as a blocker of this PR. > Well, unless you wanted to create a new one. I like what you're trying to do here, but I think the mechanical translation from a recursive algorithm is a bit too hard to follow and modify. * i want to work on it. (just expressing honest intention. no promise) * i want to do it in a separate PR. * if this PR will not likely be merged anytime soon, i can add it as separate commits to this PR. > Are you talking about doing a separate PR with just a direct replacement of the existing code with a simpler iterative implementation? If that's what you mean, then I'd definitely be happy to merge such a PR. i meant a separate PR on the top of this one.

Ah ok, yes that's fine -- once the second PR is ready I can merge both.

…

-- Daniel Beer ***@***.***> http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

yamt · 2021-05-11T03:28:09Z

On Mon, May 10, 2021 at 07:09:07PM -0700, YAMAMOTO Takashi wrote: btw, > you probably only need to keep track of fixed y, and incrementing x for each span do you mean to have a single "y" var? i don't think it works for complex shapes.
No, I meant that each span would need a y value and a next x-value (in other words, each stack level).

i'm not sure if i understand.
as far as we keep the current processing order, i think we need to keep some context
(left, right, i, which y-direction we were looking at) to restore the context.
maybe we can unify x and left though.

…
-- Daniel Beer @.***> http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

yamt · 2021-05-11T03:29:33Z

On Mon, May 10, 2021 at 07:59:21PM -0700, YAMAMOTO Takashi wrote: > On Mon, May 10, 2021 at 07:04:01PM -0700, YAMAMOTO Takashi wrote: > On Mon, May 10, 2021 at 06:34:53PM -0700, YAMAMOTO Takashi wrote: > > Apologies for the delay in getting to this. I think you might end up with a better result by implementing the span-based flood-fill directly with a simpler stack structure (you probably only need to keep track of fixed y, and incrementing x for each span) and a loop that examines the top of the stack looking for new seeds to push. I understand that this is a direct translation of the recursive procedure, and I think that's probably led to a lot of unnecessary book-keeping. > > i completely agree. > i don't want to do it in this PR though. @dlbeer do you want me to do it within this PR? generally, i don't think it's a good idea to do too many things in a single PR. but i can do that if it's considered as a blocker of this PR. > Well, unless you wanted to create a new one. I like what you're trying to do here, but I think the mechanical translation from a recursive algorithm is a bit too hard to follow and modify. * i want to work on it. (just expressing honest intention. no promise) * i want to do it in a separate PR. * if this PR will not likely be merged anytime soon, i can add it as separate commits to this PR. > Are you talking about doing a separate PR with just a direct replacement of the existing code with a simpler iterative implementation? If that's what you mean, then I'd definitely be happy to merge such a PR. i meant a separate PR on the top of this one.
Ah ok, yes that's fine -- once the second PR is ready I can merge both.

well, if you don't want to merge this PR until both are ready,
i will just add commits to this PR.

…
-- Daniel Beer @.***> http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

This reduces the memory usage a bit.

dlbeer · 2021-05-11T04:26:54Z

On Mon, May 10, 2021 at 08:29:51PM -0700, YAMAMOTO Takashi wrote: > On Mon, May 10, 2021 at 07:59:21PM -0700, YAMAMOTO Takashi wrote: > On Mon, May 10, 2021 at 07:04:01PM -0700, YAMAMOTO Takashi wrote: > On Mon, May 10, 2021 at 06:34:53PM -0700, YAMAMOTO Takashi wrote: > > Apologies for the delay in getting to this. I think you might end up with a better result by implementing the span-based flood-fill directly with a simpler stack structure (you probably only need to keep track of fixed y, and incrementing x for each span) and a loop that examines the top of the stack looking for new seeds to push. I understand that this is a direct translation of the recursive procedure, and I think that's probably led to a lot of unnecessary book-keeping. > > i completely agree. > i don't want to do it in this PR though. @dlbeer do you want me to do it within this PR? generally, i don't think it's a good idea to do too many things in a single PR. but i can do that if it's considered as a blocker of this PR. > Well, unless you wanted to create a new one. I like what you're trying to do here, but I think the mechanical translation from a recursive algorithm is a bit too hard to follow and modify. * i want to work on it. (just expressing honest intention. no promise) * i want to do it in a separate PR. * if this PR will not likely be merged anytime soon, i can add it as separate commits to this PR. > Are you talking about doing a separate PR with just a direct replacement of the existing code with a simpler iterative implementation? If that's what you mean, then I'd definitely be happy to merge such a PR. i meant a separate PR on the top of this one. > Ah ok, yes that's fine -- once the second PR is ready I can merge both. well, if you don't want to merge this PR until both are ready, i will just add commits to this PR.

Ok, sounds good to me.

…

-- Daniel Beer ***@***.***> http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

dlbeer · 2021-05-11T04:31:41Z

On Mon, May 10, 2021 at 08:28:28PM -0700, YAMAMOTO Takashi wrote: > On Mon, May 10, 2021 at 07:09:07PM -0700, YAMAMOTO Takashi wrote: btw, > you probably only need to keep track of fixed y, and incrementing x for each span do you mean to have a single "y" var? i don't think it works for complex shapes. > No, I meant that each span would need a y value and a next x-value (in other words, each stack level). i'm not sure if i understand. as far as we keep the current processing order, i think we need to keep some context (left, right, i, which y-direction we were looking at) to restore the context. maybe we can unify x and left though.

Yeah, you're right -- I think the bare minimum you can get away with is 3 pieces of state: y value, current/leftmost x, and rightmost x. Rough outline of a depth-first span-based flood-fill then is: * Examine top of stack - if top.x == top.right, pop frame and try again - otherwise: - seed (top.x, top.y - 1) - seed (top.x, top.y + 1) The procedure seed (x, y) is: * If (x, y) is already filled, do nothing * Otherwise: - x = left-most unfilled pixel in this span - right = right-most unfilled pixel in this span - push a new record { y, x, right } Haven't thought that through very carefully, so beware of bugs. The main thing though is to make sure the whole span gets filled before seeding above/below, so you avoid looping over the same pixels.

…

-- Daniel Beer ***@***.***> http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

This restructure allows us to unify "call" and "return_from_call".

This is also a preparation to eliminate the uses of goto.

Just for readability.

yamt · 2021-05-11T06:11:05Z

On Mon, May 10, 2021 at 08:28:28PM -0700, YAMAMOTO Takashi wrote: > On Mon, May 10, 2021 at 07:09:07PM -0700, YAMAMOTO Takashi wrote: btw, > you probably only need to keep track of fixed y, and incrementing x for each span do you mean to have a single "y" var? i don't think it works for complex shapes. > No, I meant that each span would need a y value and a next x-value (in other words, each stack level). i'm not sure if i understand. as far as we keep the current processing order, i think we need to keep some context (left, right, i, which y-direction we were looking at) to restore the context. maybe we can unify x and left though.
Yeah, you're right -- I think the bare minimum you can get away with is 3 pieces of state: y value, current/leftmost x, and rightmost x. Rough outline of a depth-first span-based flood-fill then is: * Examine top of stack - if top.x == top.right, pop frame and try again - otherwise: - seed (top.x, top.y - 1) - seed (top.x, top.y + 1) The procedure seed (x, y) is: * If (x, y) is already filled, do nothing * Otherwise: - x = left-most unfilled pixel in this span - right = right-most unfilled pixel in this span - push a new record { y, x, right } Haven't thought that through very carefully, so beware of bugs. The main thing though is to make sure the whole span gets filled before seeding above/below, so you avoid looping over the same pixels.
…
-- Daniel Beer @.***> http://dlbeer.co.nz/ PGP: BA6E 0B26 1F89 246C E3F3 C910 1E58 C43A 160A 553B

thank you for the comment.
unfortunately i've just implemented what i was thinking before i noticed your comment. :-)
i pushed what i implemented to this PR.

yamt · 2021-05-11T06:19:10Z

except that my implementation uses separate "counters" for above/below seeding, i think it's basically the same as what you explained.

dlbeer · 2021-05-11T22:30:55Z

Ok, looks good -- thanks!

yamt mentioned this pull request Apr 5, 2021

Add quirc_end_with_workarea() #96

Closed

yamt force-pushed the flood-fill2 branch from 29a2f47 to 7e344c3 Compare April 7, 2021 23:33

kaworu reviewed Apr 16, 2021

View reviewed changes

kaworu assigned dlbeer Apr 16, 2021

kaworu added the enhancement label Apr 16, 2021

yamt force-pushed the flood-fill2 branch from 7e344c3 to e9777f2 Compare April 17, 2021 13:24

yamt force-pushed the flood-fill2 branch from e9777f2 to c121aea Compare May 6, 2021 00:59

yamt added 2 commits May 11, 2021 11:05

quirc_resize: Make this a bit more careful about integer overflows

f1dd37f

Also, avoid malloc(0), which is not too portable.

yamt added 3 commits May 11, 2021 12:55

flood_fill_seed: separate a logic to fill a line

aad3fc6

flood_fill_seed: Unify "left" and "x" vars

40def01

This reduces the memory usage a bit.

Add QUIRC_ASSERT

dd6c64c

flood_fill_seed: Add assertions

1fa9b0c

yamt added 3 commits May 11, 2021 13:46

flood_fill_seed: Simplify the flow by having a single "return point".

38f882b

flood_fill_line: call user callback as well

0b69940

flood_fill_seed: Move flood_fill_line to the "caller"

eadbc2c

This restructure allows us to unify "call" and "return_from_call".

yamt added 4 commits May 11, 2021 14:54

flood_fill_seed: Reduce code duplicatino

a436fde

This is also a preparation to eliminate the uses of goto.

flood_fill_seed: Switch from goto to while

87ca2b6

flood_fill_seed: initialize left_up and left_down together

22269c9

Just for readability.

flood_fill_seed: Add comments on exit conditions

6b3575c

yamt force-pushed the flood-fill2 branch from c121aea to 6b3575c Compare May 11, 2021 06:07

dlbeer merged commit 5adb759 into dlbeer:master May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve stack usage for flood filling #100

Improve stack usage for flood filling #100

yamt commented Apr 5, 2021

kaworu left a comment

kaworu Apr 16, 2021

yamt Apr 17, 2021

kaworu Apr 16, 2021

yamt Apr 17, 2021

kaworu May 2, 2021

yamt May 6, 2021

dlbeer commented Apr 18, 2021

yamt commented Apr 18, 2021

yamt commented May 11, 2021

dlbeer commented May 11, 2021 via email

yamt commented May 11, 2021

yamt commented May 11, 2021

dlbeer commented May 11, 2021 via email

dlbeer commented May 11, 2021 via email

yamt commented May 11, 2021

dlbeer commented May 11, 2021 via email

yamt commented May 11, 2021

yamt commented May 11, 2021

dlbeer commented May 11, 2021 via email

dlbeer commented May 11, 2021 via email

yamt commented May 11, 2021

yamt commented May 11, 2021

dlbeer commented May 11, 2021

Improve stack usage for flood filling #100

Improve stack usage for flood filling #100

Conversation

yamt commented Apr 5, 2021

kaworu left a comment

Choose a reason for hiding this comment

kaworu Apr 16, 2021

Choose a reason for hiding this comment

yamt Apr 17, 2021

Choose a reason for hiding this comment

kaworu Apr 16, 2021

Choose a reason for hiding this comment

yamt Apr 17, 2021

Choose a reason for hiding this comment

kaworu May 2, 2021

Choose a reason for hiding this comment

yamt May 6, 2021

Choose a reason for hiding this comment

dlbeer commented Apr 18, 2021

yamt commented Apr 18, 2021

yamt commented May 11, 2021

dlbeer commented May 11, 2021 via email

yamt commented May 11, 2021

yamt commented May 11, 2021

dlbeer commented May 11, 2021 via email

dlbeer commented May 11, 2021 via email

yamt commented May 11, 2021

dlbeer commented May 11, 2021 via email

yamt commented May 11, 2021

yamt commented May 11, 2021

dlbeer commented May 11, 2021 via email

dlbeer commented May 11, 2021 via email

yamt commented May 11, 2021

yamt commented May 11, 2021

dlbeer commented May 11, 2021