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 <= buffer->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 <= buffer->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 it never executes, and if
height <=0, it REALLY never executes because it never even checks 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
_______________________________________________
Gegl-developer mailing list
Gegl-developer@xxxxxxxxxxxxxxxxxxxxxx
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gegl-developer
