-
Notifications
You must be signed in to change notification settings - Fork 338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Functionality to split encoding/decoding of colors and symbols #91
Conversation
Instead of averaging everything, or cutoffs (I experimented with cutoffs), this is using the nlargest for each color channel. Could also use N brightest pixels, but this is simpler to calculate. (probably worth investigating if it's better though...) N=9 pixels right now. We're sampling 18px for the 8x8 (only sampling 9px for the 5x5), so we're filtering out the dimmest 50%. (based on some trial+error, N=5 seems ok for the 5x5 case)
Reduces the number of magical config numbers (at the cost of more complexity in passing around bits_per_symbol everywhere). I'd like to do the same with interleave_partitions, *I think*. When all of this is working, we should be able to cleanly decouple the symbol+color sections of each cimbar frame.
Probably some optimizations here...
We'll need it for backwards-compatibility
Also, let's increase the number of fountain chunks -- it'll be 12 for 4-color 8x8 (it was 10 in 0.5.x)
We'll (hopefully) be using the decoded fountain header as a key to determine the real values for our decode colors.
The idea here is that if we have split symbols/colors, it seems more reasonable to run our math against the symbol-only capacity, so we can still succeed even if the color decode fails completely.
The premise here is to use the symbol decode to compute the expected fountain headers for the upcoming color decode -- and then use those known values to compute the average real-world color values for each relevant tile in the image. We can then use those averages to compute a more accurate color correction matrix.
Get the interleaved positions, compute avg colors.
Leaning heavily on the python `colour-science` package to teach me the right algorithms here. Still need some refactoring -- the current complexity of CimbReader::init_ccm() is less than ideal. But this *might* be everything we need?
Also add a save/load ccm for the cimbar cli to use for debugging/horseracing purposes. *Also*, the existing ccm update seems like a cross-thread race condition? (: ... a threadlocal should work just fine. (could also move it out of the CimbDecoder class and into e.g. CimbReader, where it would be implicitly threadlocal)
The error rates look better without it (~13% less with <300 color error bits on my sample set) for a relatively low compute cost (~4% more CPU time per frame). The gist here is that we probably need all the data points we can get for color decodes, and skipping cols to save us cycles isn't helping the overall decode performance. I might put it back at some point, but this feels like the right side of the compute vs decode optimization tradeoff in this case. We still break the other way in other places (e.g. OTSU threshhold instead of adaptive for the scan step...)
…ncoder + submodule bump for new sample files!
Somewhat of a misnomer as an option -- might call it just "c" mode eventually (the 4 would be the default number of colors per tile, which can still be overriden to 8)
…r 0.5.x The config namespace is reaching its limits. Going to need a rewrite...
{ | ||
Encoder en(ecc, cimbar::Config::symbol_bits(), color_bits); | ||
if (legacy_mode) | ||
en.set_legacy_mode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently the toggle for the old mode (interleaved symbol/color bits). It probably needs to be refactored into a config option, but for now this works.
("z,compression", "Compression level. 0 == no compression.", cxxopts::value<int>()->default_value(turbo::str::str(compressionLevel))) | ||
("color-correct", "Toggle decoding color correction. 1 == on. 0 == off.", cxxopts::value<int>()->default_value("1")) | ||
("color-correct", "Toggle decoding color correction. 2 == full (fountain mode only). 1 == simple. 0 == off.", cxxopts::value<int>()->default_value("2")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaning towards removing the old ("simple") color-correct logic. It's nice that it can be run under more conditions, but in its current form it's just not good enough to be useful.
("z,compression", "Compression level. 0 == no compression.", cxxopts::value<int>()->default_value(turbo::str::str(compressionLevel))) | ||
("color-correct", "Toggle decoding color correction. 1 == on. 0 == off.", cxxopts::value<int>()->default_value("1")) | ||
("color-correct", "Toggle decoding color correction. 2 == full (fountain mode only). 1 == simple. 0 == off.", cxxopts::value<int>()->default_value("2")) | ||
("color-correction-file", "Debug -- save color correction matrix generated during fountain decode, or use it for non-fountain decodes", cxxopts::value<string>()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used for doing a two-pass manual debug/verification process:
- decode sample in fountain mode, saving ccm to file
- use ccm file when decoding in non-fountain mode, generating a sample output file (errors and all) that can be used to validate error rates (e.g. with the cimbar grader tool )
if (result.count("mode")) | ||
{ | ||
string mode = result["mode"].as<string>(); | ||
legacy_mode = (mode == "4c") or (mode == "4C"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-m 4C
to switch back to cimbar 0.5.x mode.
RGB(0xFF, 0xFF, 0), | ||
RGB(0xFF, 0, 0xFF), | ||
RGB(0, 0xFF, 0) | ||
RGB(0, 0xFF, 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're changing the format anyway, I fixed the color ordering. In practice, because reedsolomon is fixing byte errors (not bit errors), this shouldn't change anything. But it does lead to less bit errors.
That is:
green -> fails as yellow or cyan (1 bit error), but almost never reads as magenta (2 bits)
yellow -> fails to green (1 bit error), but rarely to cyan (2 bits)
cyan -> fails to green (1 bit error), rarely to yellow (2 bits)
magenta -> no common failures (this has implications for optimal color choice)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a higher level note, it might make sense to fold the dark/light distinction into these color sets. Targeting a white background will require a different color set anyway.
RGB(0xFF, 0xFF, 0), // yellow | ||
RGB(0xFF, 0xFF, 0xFF), | ||
RGB(0, 0xFF, 0), | ||
RGB(0, 0xFF, 0xFF), // cyan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No 8-color configurations are supported by 0.6.0 -- but this is the correct (minimal bit errors) order of the existing colors.
{ | ||
return 10; | ||
return legacy_mode? 10 : bitspercell << 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fountain_chunks_per_frame needs to be a per-config option. mode B is 12, 4C and 8C are 10.
But this little workaround/hack is a sign that this Config namespace needs a rewrite to be able to better do its job. (this also will reduce the amount of distinct params that get passed into the Decoder/Encoder/etc)
@@ -16,6 +16,11 @@ namespace cimbar | |||
return true; | |||
} | |||
|
|||
static constexpr unsigned color_mode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My current thought is to combine this with dark
mode.
@@ -27,5 +27,8 @@ namespace cimbar | |||
static constexpr unsigned cell_size = 8; | |||
static constexpr unsigned cell_offset = 8; | |||
static constexpr unsigned cells_per_col = 112; | |||
|
|||
static constexpr int interleave_partitions = 2; | |||
static constexpr int fountain_chunks_per_frame = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two are unused at present.
@@ -64,10 +76,63 @@ inline Decoder::Decoder(int ecc_bytes, int color_bits, bool interleave) | |||
template <typename STREAM> | |||
inline unsigned Decoder::do_decode(CimbReader& reader, STREAM& ostream) | |||
{ | |||
if (_coupled) | |||
return do_decode_coupled(reader, ostream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"do it the old way"
if (data.size() < 3*3*4) | ||
return false; | ||
|
||
cv::Mat temp(3, 3, CV_32F, data.data()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These save/load functions are not intended for anything beyond manual debugging/testing/verification at present. (this is for the two-pass "fountain decode, then non-fountain decode, then diff to check error rates" use case).
@@ -15,8 +15,8 @@ class Encoder : public SimpleEncoder | |||
using SimpleEncoder::SimpleEncoder; | |||
|
|||
unsigned encode(const std::string& filename, std::string output_prefix); | |||
unsigned encode_fountain(const std::string& filename, std::string output_prefix, int compression_level=6, double redundancy=1.2, int canvas_size=0); | |||
unsigned encode_fountain(const std::string& filename, const std::function<bool(const cv::Mat&, unsigned)>& on_frame, int compression_level=6, double redundancy=4.0, int canvas_size=0); | |||
unsigned encode_fountain(const std::string& filename, std::string output_prefix, int compression_level=16, double redundancy=1.2, int canvas_size=0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated default compression (what cimbar_js will use) to 16. Arguably we could go further, but this seems to be in the right place on the performance <-> size curve.
template <typename STREAM> | ||
inline std::optional<cv::Mat> SimpleEncoder::encode_next(STREAM& stream, int canvas_size) | ||
{ | ||
if (_coupled) | ||
return encode_next_coupled(stream, canvas_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"do it the old way"
@@ -92,7 +166,7 @@ inline std::optional<cv::Mat> SimpleEncoder::encode_next(STREAM& stream, int can | |||
template <typename STREAM> | |||
inline fountain_encoder_stream::ptr SimpleEncoder::create_fountain_encoder(STREAM& stream, int compression_level) | |||
{ | |||
unsigned chunk_size = cimbar::Config::fountain_chunk_size(_eccBytes, _bitsPerColor + _bitsPerSymbol); | |||
unsigned chunk_size = cimbar::Config::fountain_chunk_size(_eccBytes, _bitsPerColor + _bitsPerSymbol, (_colorMode==0 and _coupled)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is gross, and returns to that "the config object needs to be more capable" idea.
return res; | ||
} | ||
|
||
void increment_block_id() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this object to track the block_id. That we increment_*
it is an arbitrary implementation detail -- we could also set_*
and do the computation outside this class. It's not in the critical path, so performance is not optimal but shouldn't be an issue.
@@ -141,6 +148,114 @@ bool CimbReader::done() const | |||
return !_good or _positions.done(); | |||
} | |||
|
|||
void CimbReader::init_ccm(unsigned color_bits, unsigned interleave_blocks, unsigned interleave_partitions, unsigned fountain_blocks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method coordinates the new color correction logic. It needs a refactor, but the comments hopefully explain the basics (cimbar-py also contains an implementation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But to simplify:
- we use the deterministic 48-bit fountain-chunk header to sample expected color values to use as an ad-hoc color "key".
- the sampled color averages for the 4 colors are then transformed into the ccm
a0178c2 Merge pull request #92 from sz3/0.6-mode-switch 4f73a9e Update sample image in README 656588b Add color change back for menu icon based on mode selection ad8cda7 Cleanup some includes/comments 390041e Update docs for mode B! 95afff4 Switch ccm matrix to use a thread_local 347d231 Less && in package-wasm, so we can re-run the script with (some) impunity dbf77a2 Update cimbar.js with 0.6 modes 7b54894 Merge pull request #91 from sz3/mode-b 7206b91 Use new samples submodule rev 196a893 Cleanup some includes/comments 55299de `fountain_chunks_per_frame()` config function needs to return "10" for 0.5.x 9f6600d Add toggle for old "4c" mode in the cimbar_js and the cli tools af823fd Update tests, and add back some "legacy" (0.5.x = "4c") support for encoder a05d414 Disable color decode "skip" logic 8c3b0a7 Make new color correction mode the default 88c30e0 Use Moore-Penrose least squares to compute our CCM 2f491ef Changes for the second-pass CCM+color decode fcb214d Working towards proper color correction be36c02 Add FountainHeader logic to CimbReader 36676d7 Update FountainMetadata to include the block_id 2c6319d Change default encode_id to 109, and return to older color calc? 2e7e3aa Change calc for required frames we generate for small files 6b58b25 Flush symbols/colors separately 80a668c Tweaks to Encoder default params 33b6cef Reintroduce the legacy decoder function 55dbf68 First pass at decoder for split symbols/colors 4f9622f Simplify loop e7fdfe3 First pass at splitting symbol and color channels, encoder edition db0812c WIP to vary fountain chunk size by number of symbol bits... 4067532 color_mode for encoder? 32b9ffa Experiment with a different color calculation? git-subtree-dir: app/src/cpp/libcimbar git-subtree-split: a0178c2
This operates as a distinct option from the other config parameters (grid size, tile size, number of colors, ...), and like number of colors is a runtime parameter.
The motivation is to allow the symbol decode to occur independently of the color decode, allowing
(1) the decode to succeed -- at reduced capacity -- even if the color decode fails,
(2) the color decode to use the information it has from the symbol decode to generate a better color correction matrix, which will allow the color decode to succeed in a larger variety of conditions.
As an added bonus, the color decode can succeed while the symbol decode fails -- but in practice should not be common (in any case, it's not ideal).
The end result here is a new version of libcimbar,
0.6.0
, and a change to the format which will be called "mode B". The0.5.x
versions ("4-color" and "8-color") will be "mode 4C" and "mode 8C" respectively. "mode 8C" support will be removed in0.6.0
, but 4C (the more useful one) will continue to be supported until at least0.7.0
, whenever that happens.