ofPixels copy broken

[prisonerjohn]

I posted a comment in the problematic commit but I'll add it here too as it's a 7 month old change...

Here's the commit: f7522b9#diff-7d727932e9b85218273507defcbf4b9e9c9a300c10d62867e1459b2f167b45f8L399

And the issue:
ofPixels.size() was changed to return the number of bytes instead of the number of pixels, and that breaks copying with different data formats.

If you try something like this:

ofFloatPixels fPix;
fPix.allocate(512, 512, 1);
ofPixels cPix;
cPix = fPix;

then you get a buffer overrun in copyFrom() because the loop runs on ofPixels.size().

for(size_t i = 0; i < mom.size(); i++){
	pixels[i] = ofClamp(mom[i], 0, 1) * factor;
}

There's already an ofPixels.getTotalBytes() that returns the total bytes, why make size() return the same thing?

[dimitre]

Good catch.
thoughts:
There are inconsistencies here as pixels is made of PixelType, and we have some formats that doesn't have an uniform pixeltype, like RGB565, which the total number of bits would be a more correct manner of doing the correct allocation. OR we can mark as unimplemented if pixel format is RGB565.

https://github.com/openframeworks/openFrameworks/pull/8226/files
in this PR size() is again reflecting only the pixel count, ignoring channel bit depth or channel count.
if we decide this is the correct usage of size we should update doxygen to reflect.
today it hints returning total bytes, but only considering one byte per channel.
yes there is historical confusion needing clarification and cleanup.

	/// \brief Get the number of values that the ofPixels object
	/// contains, so an RGB data 400x400 would be 480,000, whereas RGBA data
	/// of the same dimensions would be 640,000.

[artificiel]

not that the doxygen is necessarily "truth" but in this case the number of values ... contained is pretty clearly independent from the resolution of the said values (for example it makes a difference between RGB and RGBA but it makes no difference between 8 or 10 or 16 bits RGB)

the commit message is not very verbose only mentionning a "critical flaw" -- what flaw/problem is changing the return of .size() fixing? as i understand @prisonerjohn's comment, it breaks the copyFrom() iterator which works on a canvas*channels basis, not memory size.

and since the subject is being brought up, how does getBitsPerPixel() respond to RGB565 data? (is that being tested? how does it move to texture? it would be great to have an example with such source media).

[ofTheo]

Definitely in favor of your solution Dimitre for a quick regression fix. Then we can look at the edge cases mentioned and make sure they are working in another PR?

[dimitre]

I agree it is good to merge and we can think of edge cases.
The tests will fail but they are flawed anyway depending of what we decide is the correct implementation. we can adjust this in another PR.