geometry: clean up imOut if operation fails

[akx]

Alternative to #9962

Sibling of #9784 - an earlier iteration of it uncovered Python complaining about a value being returned while an exception was set. _transpose did indeed just wholesale toss out the retval of the underlying operation function...

[radarhere]

I understand completely that you dislike that the return value is ignored.

However, the return value only indicates failure if the modes or the sizes don't match, and _transpose creates imOut from those aspects of imIn, and ImagingBoxBlur (the only other places one of these functions is called from) does almost the same. It seems to me that there is no way that the return value would ever be a failure.

Is it worth considering the alternative solution of removing the mode and size checks from ImagingFlipLeftRight through to ImagingTransverse, so that there is no return value at all, and documenting this in a comment?

[akx]

It seems to me that there is no way that the return value would ever be a failure.

As noted, an earlier iteration of #9784 had an issue where I'd carelessly copy-pasted all checks across Geometry.c the same, even though some of the functions actually needed to check whether imIn's size is the transposition of imOut's size, not the same size.

That in turn caused CPython RuntimeErrors because we were both returning a value from a C extension function while having set an exception, because this function wasn't checking correctly... If this function had behaved correctly CPython-API-wise, I would've spent less time debugging that exception, and more time figuring I was a dummy for copy-pasting blindly :)

Given, as you note, that the Imaging* functions are libImaging internal APIs that are used by other functions, I'd want them to have the same checks that other libImaging functions do.

[radarhere]

Wouldn't you also want to check the return values here?

Pillow/src/libImaging/BoxBlur.c

Lines 281 to 289 in 11de4b9

	/* Transpose result for blur in another direction. */
	ImagingTranspose(imTransposed, xradius == 0 ? imIn : imOut);
	/* Reuse imTransposed as a source and destination there. */
	for (i = 0; i < n; i++) {
	ImagingHorizontalBoxBlur(imTransposed, imTransposed, yradius);
	}
	/* Restore original orientation. */
	ImagingTranspose(imOut, imTransposed);

[akx]

Wouldn't you also want to check the return values here?

Yep, good catch. This particular PR was just to fix this particular instance which bit me.

I can do another pass over all of these fallible cases in another PR?

[radarhere]

I suspect you are already aware, but op has been checked in the previous switch, so thedefault case will never run here.

[akx]

I was thinking of combining the two switch/cases even if it means duplicating the allocations, it might be clearer.

EDIT: Tried, looks ugly :)