[Octave-patch-tracker] [patch #9077] image package: new function imfill.m

[Hartmut]

Follow-up Comment #18, patch #9077 (project octave):

Thanks a lot for carefully adjusting my code to Octave coding guidelines, and
reviewing it.

I went through the new version of the code (the latest version in the official
repo) and will make some comments below. Otherwise I am fine with the code, as
long as the tests still pass :)

Here are my comments:
* line 22: You have added one of the two supported (but in Matlab
undocumented) syntax versions: imfill(I, 'holes'). Then it would probably a
good idea to add the second supported but undocumented version as well:
imfill(I, CONN, 'holes').
* line 28: two typos
* line 90: in this comment, we need to remove the first half: "imfill(BW,
CONN)", this is no valid syntax.
* I have seen how you avoid my stupy "padding", very nice :)
* on several occasions you are calling the two helper functions check_loc and
the set_nonborder_pixels with the full variable img as parameter. Isn't this
creating a second memory copy of the (potentially huge) image data in img? I
was intentionally only calling check_loc with "size(img)" instead of "img" in
order to save this memory. And since your "fancy" way of avoiding the "stupid"
padding all around, was motivated by saving memory, shouldn't we try to avoid
this second full copy of the image data here as well? (But maybe my
understanding of when Octave will do a memory copy is not exhaustive...)

Otherwise I am fine with releasing this :)

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/patch/?9077>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/