[Bug] ip::horizontalFlip

Developing Cinder
1

Hello, I just found a bug in ip::flipHorizontal.

Surface.hasAlpha() is used to decide the pixel byte increment.

In the case of BGRX hasAlpha returns false, but the increment should be 4 and not 3.

I am not sure where else hasAlpha is used to decide the byte increment and I am not sure if making hasAlpha return true for BGRX would break something else…

@andrewfb ?

I guess the following should fix the problem in the case of flipHorizontal but not anywhere else:

template<typename T>
void flipHorizontal( SurfaceT<T> *surface )
{
	const int32_t height = surface->getHeight();
	const int32_t width = surface->getWidth();
	const int32_t halfWidth = width / 2;
	
	const int32_t numBytes = surface->getPixelBytes();

		for( int32_t y = 0; y < height; ++y ) {
			T *rowPtr = surface->getData( ivec2( 0, y ) );
			for( int32_t x = 0; x < halfWidth; ++x ) {
				for( int c = 0; c < numBytes; ++c ) {
					T temp = rowPtr[x*numBytes+c];
					rowPtr[x*numBytes+c] = rowPtr[(width-x-1)*numBytes+c];
					rowPtr[(width-x-1)*numBytes+c] = temp;
				}
			}
		}
}
2

I think you probably want to use surface->getPixelInc() rather than surface->getPixelBytes()

3

True! thanks! I guess it worked for me because I was using a surface with 4 bytes per pixel…

Edit: hmm now that I think about it, I am not sure whats the difference between getPixelInc() and getPixelBytes() is it related to padding?

4

Sorry about that - I just pushed a fix here.

getPixelInc() is units of T, correlating with the number of channels per pixel, including a potentially unused alpha. whereas getPixelBytes() is actual bytes. So for a RGBA or RGBX Surface32f, pixel inc is 4, and pixelBytes is 4*sizeof(float)=16. Just there for convenience.

5

Thanks a lot for the fix and explanation Andrew!