Abort/crash resizing a sequential image

[angelmixu]

Hi! Here I am again :D

Using the latest 8.8 branch we have a crash (well, it's an g_abort from a g_assert) while processing some stuff. I have managed to get an small example for the crash and the tiff file being used:
93384_1.zip

int main(int argc, char* argv[])
{
	if (VIPS_INIT(argv[0]))
		vips_error_exit(NULL);

	VipsImage* image = vips_image_new_from_file("93384_1.tif", 
		"access", VIPS_ACCESS_SEQUENTIAL,
		nullptr);
	
	VipsImage* outImage = vips_image_new();

	double scaleX = 0.027707808564231738;
	double scaleY = 0.028142589118198873;

	int error = vips_resize(image, &outImage,
		scaleX,
		"vscale", scaleY,
		"kernel", VIPS_KERNEL_LANCZOS3,
		"centre", true,
		NULL);

	printf("resize result: %d\n", error);

	vips_image_write_to_file(outImage, "vips_resize_output.tif", nullptr);

	return 0;
}

What I have been seeing is that when resizing and using sequential access, it is asserting in shrinkv.c line 336:

	if( shrink->sequential &&
		r->top + r->height >= or->im->Ysize ) {
		/* First unused scanline. resample->in->Ysize because we want
		 * the height before the embed.
		 */
		int first = or->im->Ysize * shrink->vshrink;
		int unused = resample->in->Ysize - first;

		g_assert( unused >= 0 ); // <= HERE!!!

In this case, unused is -4, as or->im->Ysize is 157, shrink->vshrink is 17, and resample->in->Ysize is 2665.
So the assert is false and it calls a g_abort, thus exiting the application.

Is this a bug, or should we avoid sequential access in this kind of images?

Thanks!

BTW, could anyone comment on this: #1345 ?

[jcupitt]

Hey @angelmixu, thanks! Could you try 8.8 HEAD? It should be fixed.

The >=0 was an implicit assumption of round down on shrink, but we actually round to nearest, so it's safe to remove the assert.

1345 had dropped off my radar, sorry :( I'll look again.

[jcupitt]

This code has gone in 8.9, by the way, there's something much better, so this hacky thing is only temporary.

[angelmixu]

Hey @angelmixu, thanks! Could you try 8.8 HEAD? It should be fixed.

Awesome! It's working correctly! Thanks :)

1345 had dropped off my radar, sorry :( I'll look again.

Don't worry! We are not in a hurry, but we'd like to close that soonish and end the migration.

This code has gone in 8.9, by the way, there's something much better, so this hacky thing is only temporary.

Oh, would you recommend me to use master instead of the current stable branch?

[jcupitt]

I wouldn't use master in production, though I use it myself for small things.

[angelmixu]

Ok! Thanks again, I'll close the issue :)