Work on many issues

[daniel-lerch]

Hi Ian,

like with all pull requests I added more documentation ( #37 ). In addition to that I set more restrictive access modifiers in some cases. Furthermore I added a parameter checking for MMALResizerComponent ( #51 ).

In GenericExtensions.cs I changed the writing of float and integer values to make it more obvious which multiplication operator we are using. And I changed the float.ToByte() method by multiplying with 255 instead of 256. This is the mathematically right solution if you have a proportional scale from 0 to 255.

Daniel

[techyian]

Hi Daniel,

Just want to discuss a few things before accepting the PR.

1) Multiplying by 255 instead of 256 - my initial thought was to multiply by 255. However, this answer on SO suggested 256 is a safe value to multiply by so long as the input param isn't 1.0f, and if so, to handle that separately. Happy to discuss this further.

2) The ranges you've specified in the Resizer Component - are these values referenced anywhere officially, or are they based on your own findings?

Thanks, Ian

[daniel-lerch]

Hi Ian,

1. Multiplying by 255 instead of 256...

Sorry about that. I was not aware that the conversion from a float to byte is more like Math.Floor than Math.Round. In my opinion using (byte)Math.Round(val * 255.0f) would have the least precision loss. However using (byte)(val * 255.999) will run slightly faster because you have less instructions and no branch like in current method. Overflow checks also cost performance and are unnecessary because you can't damage more than your RGB values.

2. The ranges you've specified in the...

I did not search for official documentation for this issue. I just tried out resolutions. My boundary checks only prevent an EINVAL. Using 16384×16384 crashed my application with an ENOMEM but may work with more RAM. However there a few other strange results. Using 1×1 as resolution kind of crashed the GPU. It showed coloured stripes all over the display. I could only remove the power supply to reboot. The next interesting point is that you always receive at least 2048 bytes with BGRA encoding. This means resulting images have a resolution of at least 32×16 pixel even if the resolution is configured to 2×2. The last unexpected behavior was achieved using 32767×32767 as desired resolution. In this case the application did not crash with an ENOMEM but returned no data.

Maybe you can test these settings with your Pi, too. After this I would rather add tips to the wiki than forcing lower/higher values by Exceptions. And it would be great if you could add to the wiki that the component is initialized when calling ConfigureOutputPort. This would make it much easier to find errors coming from wrong constructor values, etc.

Daniel

[daniel-lerch]

Hi Ian,

which approach do you prefer for the colour conversions? Do you want me to search for an official documentation for supported resolutions with the ResizerComponent? Are there any other points that need to be discussed/changed before you can merge this pull request?

Daniel

[techyian]

Hi Daniel,

I've got round to checking the Math conversion and I'm happy to change the value to 255.999f as regression tests still pass with this value.

With regards to the Resizer component, I think we should just error if the lower bound is 1 - that is a very strange bug that you've uncovered and I can reproduce on my Pi. I will raise a ticket with the Pi guys to see if they've come across it. The upper bound is constrained by the amount of memory MMAL can reserve, an image of 16384x16384 using YUV420 is trying to reserve approx. 402Mb of memory to process the image; considering the max camera sensor resolution on even the 2nd gen module is 3280x2464, the resulting image would be extremely pixelated.

If you can make the relevant changes I'd be happy to accept the PR.

Thanks, Ian