Hello. I have been reading through gegl-buffer.c and have caught a few minor bugs that I intend to patch once I have completed my modifications to the file.

I noticed that there is a method in gegl-buffer.c called gegl_buffer_void () that contains a comment /* FIXME: handle z==0 correctly */. The code seemed rather messy, so I cleaned it up without modifying the behaviour (I'm fairly certain).

Below, I provide the original code and my modified version, along with an explanation of the changes I have made.

What I am wondering is if anyone could provide some guidance as to the desired behaviour of this method so the FIXME can be properly addressed.

ORIGINAL CODE:
--------------------------
static void
gegl_buffer_void (GeglBuffer *buffer) {
gint width = buffer->extent.width; gint height = buffer->extent.height; gint tile_width = buffer->tile_storage->tile_width; gint tile_height = buffer->tile_storage->tile_height; gint bufy = 0;

{
gint z;
gint factor = 1;
for (z = 0; z max_z; z++)
{
bufy = 0;
while (bufy < height)
{
gint tiledy = buffer->extent.y + buffer->shift_y + bufy; gint offsety = gegl_tile_offset (tiledy, tile_height); gint bufx = 0;
gint ty = gegl_tile_indice (tiledy / factor, tile_height);

if (z != 0 || /* FIXME: handle z==0 correctly */ ty >= buffer->min_y)
while (bufx < width)
{
gint tiledx = buffer->extent.x + buffer->shift_x + bufx; gint offsetx = gegl_tile_offset (tiledx, tile_width);

gint tx = gegl_tile_indice (tiledx / factor, tile_width);

if (z != 0 || tx >= buffer->min_x) gegl_tile_source_command (GEGL_TILE_SOURCE (buffer), GEGL_TILE_VOID, tx, ty, z, NULL);

if (z != 0 || tx > buffer->max_x) goto done_with_row;

bufx += (tile_width - offsetx) * factor; }
done_with_row:
bufy += (tile_height - offsety) * factor;

if (z != 0 || ty > buffer->max_y) break;
}
factor *= 2;
}
}
}
--------------------------

MODIFIED CODE: --------------------------
static void
gegl_buffer_void (GeglBuffer *buffer) {
const gint width = buffer->extent.width; const gint height = buffer->extent.height; const gint tile_width = buffer->tile_storage->tile_width; const gint tile_height = buffer->tile_storage->tile_height; const gint extent_plus_shift_x = buffer->extent.x + buffer->shift_x; const gint extent_plus_shift_y = buffer->extent.y + buffer->shift_y;

{ gint z;
gint bufy = 0;
gint factor = 2;

/* z == 0 */ while (bufy < height)
{
gint tiledy = extent_plus_shift_y + bufy; gint offsety = gegl_tile_offset (tiledy, tile_height); gint bufx = 0;
gint ty = gegl_tile_indice (tiledy, tile_height);

if (ty >= buffer->min_y) while (bufx < width)
{
gint tiledx = extent_plus_shift_y + bufx; gint offsetx = gegl_tile_offset (tiledx, tile_width);

gint tx = gegl_tile_indice (tiledx, tile_width);

if (tx >= buffer->min_x) gegl_tile_source_command (GEGL_TILE_SOURCE (buffer), GEGL_TILE_VOID, tx, ty, z, NULL);

if (tx > buffer->max_x) break;

bufx += tile_width - offsetx; }

if (ty > buffer->max_y) break;

bufy += tile_height - offsety; }

/* z > 0 */
if (height > 0 && width > 0) /* can height and width be negative? */ for (z = 1; z max_z; z++)
{
gint ty = gegl_tile_indice (extent_plus_shift_y / factor, tile_height);
gint tx = gegl_tile_indice (extent_plus_shift_x / factor, tile_width);

gegl_tile_source_command (GEGL_TILE_SOURCE (buffer), GEGL_TILE_VOID, tx, ty, z, NULL);

factor *= 2; }
}
}

REASONING
-------------------
I'll try to talk you through my reasoning. Several variables seemed to go unchanged throughout the method, so they became const. In particular, adding the extent and the shift for every loop iteration seemed wasteful, so I created new variables for them. I unrolled the for-loop and addressed the z ==0 case separately, since that is apparently the case to fix. Here is how I handle the two cases:

z == 0 ---------
- I removed the variable "factor" from the equations because it's simply equal to 1. This is why I instead initialize "factor" to 2, to get it ready for the z>0 cases.

- I also removed all the z!=0 checks, since we already know that z==0.

- The GOTO seems to do the exact same thing as BREAK would, so I replaced it. Also, there was no reason to increment "bufy" if the break happens, as it is reset to 0 at the beginning of the for-loop, so I swapped the order.

z > 0
-------
- All the if-statements are automatically TRUE, so I removed them.

- The last two if-statements lead to BREAKs, meaning that the while-loops NEVER actually loop, so I replaced the while-loops with if-statements.

- Since the while-loops are now if-statements, bufx and bufy are never incremented, so they are ALWAYS 0 and have therefore been removed. Since they are 0, we only ever compare width and height to 0. Also, since they are the only reason we compute offsety and offsetx, those variables have been removed.

- The only line that actually seems to perform an action is gegl_tile_source_command. So if width 0. Because of this, the entire for-loop is useless unless they're both positive, so this check has been moved around the for-loop.

Adam Turcotte

There is no need to be unsure here, since these kind of changes should come along with test cases (unless there already is a tests for this code, which I suspect is the case). The test for GEGL lives in ./tests and you run them with 'make check'.

There does not appear to be a test that properly examines the behaviour of gegl_buffer_void (). I commented out the entire contents of this method and all 36 buffer tests still passed.

The only method that appears to call gegl_buffer_void () is gegl_buffer_dispose (), and I can verify that this method is called during the tests since, despite the fact that all 36 tests pass without it, there are several buffer leaks.

Is it possible that gegl_buffer_void () is never called during these tests? If so, what test case would ensure that it is called?

Perhaps even more importantly, if there are 36 test cases that manage to pass without a gegl_buffer_void () method in place, what exactly is this method supposed to accomplish?

Adam Turcotte