ofPixels memory allocation size fix

[dimitre]

This one will need the tests updated to reflect what pixelSize represents now.
closes #8225

[dimitre]

This one will need tests updated because pixelSize represented a different thing in the past.
I think we are going to a more complete and correct way of treating pixels
ideas? suggestions?

[ofTheo]

Don't we want a simple revert of:
f7522b9#diff-7d727932e9b85218273507defcbf4b9e9c9a300c10d62867e1459b2f167b45f8L399

Looking at this PR I am not sure if it's actually reverting and might be changing behavior even more.

I'll note in the lines what I mean.

cc @prisonerjohn

[ofTheo]

pixels size used to be:
pixelsSize = bytesFromPixelFormat(w,h,_pixelFormat) / sizeof(PixelType);

bytesFromPixelFormat is effectively: w * h * 24 / 8 ( for RGB )
so: newSize = w * h * 3; for RGB

size_t ofPixels_<PixelType>::bytesFromPixelFormat(size_t w, size_t h, ofPixelFormat format){
	return w * h * pixelBitsFromPixelFormat(format) / 8;  <-- pixelBitsFromPixelFormat for rgb is 24 
}

a simpler fix would be:

pixelsSize = w*h * getNumChannels()

But maybe safer to first revert the commit where the regression was introduced and then do a PR with the bit depth support after?

[dimitre]

Part of the problem (historical ofPixels problem) is we have two different units
one is PixelType (it can be char, float, etc)

and the other are formats that doesn't have an uniform number of bytes per channel like RGB565 (5 + 6 + 5 total 16 bits) or NV12 (12 bits per channel)

but the array that stores pixel information will be always a PixelType (so we consider an "uncompressed" pixel type, to be worked on shaders).

we can go one way or another.

if we support all the images format I suggest changing the Pixels array to uint8_t instead of PixelType and allocate everything following "bytesPerPixel"

Or keep everything as it is, and comment out all image types that doesn't follow PixelType bit depth.
makes sense?

[ofTheo]

again good to note that pixelsSize is not w*h but w*h * getNumChannels();
pixelsSize is what ofPixels::size() returns and if you look here, it needs to represent the total number of bytes not just w*h.

void ofPixels_<PixelType>::copyFrom(const ofPixels_<SrcType> & mom){
	if(mom.isAllocated()){
		allocate(mom.getWidth(),mom.getHeight(),mom.getNumChannels());

		const float srcMax = ( (sizeof(SrcType) == sizeof(float) ) ? 1.f : std::numeric_limits<SrcType>::max() );
		const float dstMax = ( (sizeof(PixelType) == sizeof(float) ) ? 1.f : std::numeric_limits<PixelType>::max() );
		const float factor = dstMax / srcMax;

		if(sizeof(SrcType) == sizeof(float)) {
			// coming from float we need a special case to clamp the values
			for(size_t i = 0; i < mom.size(); i++){
				pixels[i] = ofClamp(mom[i], 0, 1) * factor;
			}
		} else{
			// everything else is a straight scaling
			for(size_t i = 0; i < mom.size(); i++){
				pixels[i] = mom[i] * factor;
			}
		}
	```

[dimitre]

I propose we gave more thought and try to solve incongruences that always existed in ofPixels instead of reverting last PR. When you say the total number of bytes it is not true, it is the total number of channels, each of them can be one byte (as it always supported) or more.

I'll take a look soon at this PR. so size() is back to original behavior

[dimitre]

Please take a look again and see the difference. it is now not based in bytes for pixels allocation.

[dimitre]

I reverted size to the behavior it always was, and changed the PR title.
this now handles well new Pixels allocation that is based on "total number of channels" instead of total number of bytes, which is only true if we have a 8 bit PixelType (ofPixels)

[ofTheo]

Awesome thanks @dimitre !