mrcodetastic / ESP32-HUB75-MatrixPanel-DMA

An Adafruit GFX Compatible Library for the ESP32, ESP32-S2, ESP32-S3 to drive HUB75 LED matrix panels using DMA for high refresh rates. Supports panel chaining.
MIT License
951 stars 211 forks source link

Using color depth other than 8 bpp corrupts colors badly #81

Closed vortigont closed 3 years ago

vortigont commented 3 years ago

Need to check this issue, should not be such a loss in quality at least for 5-7 bits

mrcodetastic commented 3 years ago

It's not really a bug, it's because the library is designed for 8bpp color and therefore r g b values being 8 bit / uint8_t (and this should not be changed)... but if somebody does deside to set PIXEL_COLOR_DEPTH_BITS < 8 then: uint8_t mask = (1 << color_depth_idx); // 24 bit color ... doesn't work as it'll drops MSB's (i.e. never increment color_depth_idx high enough).

i.e. Set the PIXEL_COLOR_DEPTH_BITS to '4' and then pass an 8 bit number... the top four MSB's will get dropped and one will end up with some random number / colour.

TL/DR: Fixed: https://github.com/mrfaptastic/ESP32-HUB75-MatrixPanel-I2S-DMA/commit/889ed51fe10c4c2b488432a26e832785dd0ee270

:)

mrcodetastic commented 3 years ago

Run pattern plasma with 3-4 bit colour... it actually looks pretty good still now!

mrcodetastic commented 3 years ago

64x32 Pattern Plasma, 8bit color per RGB. We're going to need 16384 bytes of SRAM just for the frame buffer(s).

64x32 Pattern Plasma, 5bit color per RGB (looks just as good to be honest) We're going to need 10240 bytes of SRAM just for the frame buffer(s).

There we go. 60% reduction for just as good output. I think it's fair to say @vortigont, with your improvements, and this color depth fix - this library is now pretty much perfect.

vortigont commented 3 years ago

Nice catch, @mrfaptastic! Indeed chopping off MSB's was a weird way to reduce color depth :)) Yup! Now this lib could suit to any needs :) How do think if it worth the efforts to try and incorporate color depth control into config struct and make it run-time tunable?

mrcodetastic commented 3 years ago

How do think if it worth the efforts to try and incorporate color depth control into config struct and make it run-time tunable?

I don't think it's worth the effort to be honest. It is very rare anybody would or should need to change this. Also, I feel it'll complicate the workings of the library.

vortigont commented 3 years ago

I think this could be very handy option for a long chain of modules actually. And at the same time lowering color depth you increase refresh rate. So it might be useful to have a tunable for this. But no hurry, it's pretty easy to use it via define using platformio build-time options, for example. Just two points comes to my mind: 1) chopping off LSB's introduces an average rounding error of 0,5 LSB. Maybe it's not a big deal compared to the cheap implementation of "chopping LSB's" :) And another 2) would be nice to fit reduced color depth to the GFX's default RGB565. So the is no need for conversion 565 to rgb24 and followed by chopping LSB's

But yeah, this would be a little bit more complicated approach and it requires profiling to make sure there is no performance loss. OK, let's put this off for some lazy times maybe... :)

mrcodetastic commented 3 years ago

But yeah, this would be a little bit more complicated approach and it requires profiling to make sure there is no performance loss. OK, let's put this off for some lazy times maybe... :)

Personally I think it's best to keep things as is with the library for simplicity. In any case, when the colour depth is reduced the performance of the drawing increases substatially (run PatternPlasma with 4 bit colour and you'll see how quick it is) which negates any bit shifting operations. As for any rounding errors, at the end of the day it's about what people see with the naked eye, and from what I can see - it works well enough.

would be nice to fit reduced color depth to the GFX's default RGB565. So the is no need for conversion 565 to rgb24 and followed by chopping LSB's

I think what I might do next is make a RGB24 version of GFX_Root. 565 is only there for backwards compatability.

https://github.com/mrfaptastic/GFX_Root