Update decoder objects for per-frame mode switching #94
Conversation
I say "color_mode", but this also includes the coupled legacy behavior (merged symbols and colors). Motivation: per-frame bouncing between color_mode, so we can test success/failure for mode B/4C and pick the one that's working. Details: When running a decoder across multiple frames, we have certain state (loaded hashes for imagehash hamming distance, for example) that is longer lived -- that we don't want to run each frame. This *currently* is stored in the CimbDecoder (also the fountain_decoder_sink, though that's not relevant to this change). To allow us to switch between modes, we need the CimbDecoder to not store color_mode, but rather take it as an argument where required. Misc: There are other ways to accomplish this, but this change seems like the best immediate path forward. The actual technical requirement is: (1) to run in different modes (2) on different threads (3) at the same time.
…g data ... since we're using that as the detection mechanism for auto-detect. Also, a simplification to Decoder::do_decode()
| @@ -208,12 +207,12 @@ std::tuple<uchar,uchar,uchar> CimbDecoder::avg_color(const Cell& color_cell) con | |||
| return center.mean_rgb(); | |||
| } | |||
|
|
|||
| unsigned CimbDecoder::decode_color(const Cell& color_cell) const | |||
| unsigned CimbDecoder::decode_color(const Cell& color_cell, unsigned color_mode) const | |||
There was a problem hiding this comment.
My least favorite part of the changeset...
| : _image(img) | ||
| , _fountainColorHeader(0U) | ||
| , _cellSize(Config::cell_size() + 2) | ||
| , _positions(Config::cell_spacing(), Config::cells_per_col(), Config::cell_offset(), Config::corner_padding()) | ||
| , _decoder(decoder) | ||
| , _good(_image.cols >= Config::image_size() and _image.rows >= Config::image_size()) | ||
| , _colorCorrection(color_correction) | ||
| , _colorMode(color_mode) |
There was a problem hiding this comment.
CimbReader is created per frame (the _image(img) is a hint of that), so storing color_mode is no problem.
| { | ||
| cv::Mat sample = TestCimbar::loadSample("6bit/4color_ecc30_fountain_0.png"); | ||
|
|
||
| CimbDecoder decoder(4, 2); | ||
| CimbReader cr(sample, decoder); | ||
| CimbReader cr(sample, decoder, 0); |
There was a problem hiding this comment.
This is the old test case, before #91 . As long as we're supporting mode 4C, it makes sense to keep a few tests around to make sure it doesn't break.
| CimbReader reader(img, _decoder, should_preprocess, color_correction); | ||
| return do_decode(reader, ostream); | ||
| CimbReader reader(img, _decoder, color_mode, should_preprocess, color_correction); | ||
| return do_decode(reader, ostream, color_mode==0); |
There was a problem hiding this comment.
This color_mode==0 check is what determines where we do the "coupled" decode -- I don't think it really makes sense to lock them together that way long term, so this is something that will hopefully be addressed by future changes to the way runtime configuration is done.
| fountain_decoder_sink<cimbar::zstd_decompressor<std::ofstream>> fds(tempdir.path(), cimbar::Config::fountain_chunk_size(30, 6, true)); | ||
|
|
||
| unsigned bytesDecoded = dec.decode_fountain(encodedImg, fds, 1); | ||
| assertEquals( 7500, bytesDecoded ); |
There was a problem hiding this comment.
The key here is that the fountain decode actually succeeds, the aggregator (the fountain_decoder_sink) just isn't prepared for it. This is a sanity check that (1) it doesn't decode, because it shouldn't work, (2) it doesn't crash.
There was a problem hiding this comment.
... but after further on-device testing with a beta build of CFC, it looks like this test case isn't sufficient 🙃
There was a problem hiding this comment.
Error was a result of feeding bad data to wirehair -- resolved by f1e136f
…size... ... and the fountain *stream* (sink)'s chunk size, we shouldn't send it the decoded bytes. It's not going to be able to do anything useful with them. Instead, we'll have a /dev/null style "null_stream" that just tracks how many bytes were written to it.
sz3
force-pushed
the
for-autodetect-decode
branch
from
c299fa1 to
f1e136f
Compare
f21b1a5 Merge pull request #94 from sz3/for-autodetect-decode f1e136f Decoder: when we known there's a mismatch between the fountain chunk size... f5607a3 Add test to validate we can survive giving the fountain sink the wrong data 7b11358 Use per call switch for color_mode -- rather than per object instance ae13549 Merge pull request #93 from sz3/old-app-hint-4c e560c03 Switch cimbar_send to the old default (4C) to be consistent with cimbar_js 5e8a2a2 Switch encoder default back to mode 4C? d09bdda Use the menu to communicate that 4C is for backwards compatibility git-subtree-dir: app/src/cpp/libcimbar git-subtree-split: f21b1a5
Motivation: per-frame bouncing between color_mode, so we can test success/failure for mode B/4C and pick the one that's working.
Details:
When running a decoder across multiple frames, we have certain state (loaded hashes for imagehash hamming distance, for example) that is longer lived -- that we don't want to run each frame. This currently is stored in the CimbDecoder (also the fountain_decoder_sink, though that's not relevant to this change). To allow us to switch between modes, we need the CimbDecoder to not store color_mode, but rather take it as an argument where required.
Misc:
There are other ways to accomplish this, but this change seems like the best immediate path forward. The actual technical requirement is:
(1) to run in different modes
(2) on different threads
(3) at the same time.