From 65d608cdac701ab10f84bd8dc4a6c0be7eef26f1 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 5 Aug 2016 14:02:12 -0400 Subject: MAINT-6584: Clarify LLKDUMessageError::flush() throwing exception. --- indra/llkdu/llimagej2ckdu.cpp | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'indra') diff --git a/indra/llkdu/llimagej2ckdu.cpp b/indra/llkdu/llimagej2ckdu.cpp index 9347e51b85..2adb90320c 100644 --- a/indra/llkdu/llimagej2ckdu.cpp +++ b/indra/llkdu/llimagej2ckdu.cpp @@ -165,6 +165,12 @@ struct LLKDUMessageError : public LLKDUMessage // terminating handler→flush call." // So throwing an exception here isn't arbitrary: we MUST throw an // exception if we want to recover from a KDU error. + // Because this confused me: the above quote specifically refers to + // the kdu_error class, which is constructed internally within KDU at + // the point where a fatal error is discovered and reported. It is NOT + // talking about the kdu_message subclass passed to + // kdu_customize_errors(). Destroying this static object at program + // shutdown will NOT engage the behavior described above. if (end_of_message) { throw "KDU throwing an exception"; -- cgit v1.2.3 From a5ce63eb3c4a3f3ea57ca2a353954a2afd43fc72 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 5 Aug 2016 17:44:13 -0400 Subject: MAINT-6584: Add explanatory comments to LLImageJ2CKDU implementation. These comments are inherently fragile, in that they enumerate all present callers of certain methods. Adding, removing or relocating calls would invalidate these comments. However, the LLImageJ2CKDU implementation is probably pretty stable by now. --- indra/llkdu/llimagej2ckdu.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) (limited to 'indra') diff --git a/indra/llkdu/llimagej2ckdu.cpp b/indra/llkdu/llimagej2ckdu.cpp index 2adb90320c..1201170f6d 100644 --- a/indra/llkdu/llimagej2ckdu.cpp +++ b/indra/llkdu/llimagej2ckdu.cpp @@ -202,6 +202,10 @@ LLImageJ2CKDU::~LLImageJ2CKDU() // Stuff for new simple decode void transfer_bytes(kdu_byte *dest, kdu_line_buf &src, int gap, int precision); +// This is called by the real (private) initDecode() (keep_codestream true) +// and getMetadata() methods (keep_codestream false). As far as nat can tell, +// mode is always MODE_FAST. It was called by findDiscardLevelsBoundaries() +// as well, when that still existed, with keep_codestream true and MODE_FAST. void LLImageJ2CKDU::setupCodeStream(LLImageJ2C &base, bool keep_codestream, ECodeStreamMode mode) { S32 data_size = base.getDataSize(); @@ -212,6 +216,12 @@ void LLImageJ2CKDU::setupCodeStream(LLImageJ2C &base, bool keep_codestream, ECod // mCodeStreamp.reset(); + // It's not clear to nat under what circumstances we would reuse a + // pre-existing LLKDUMemSource instance. As of 2016-08-05, it consists of + // two U32s and a pointer, so it's not as if it would be a huge overhead + // to allocate a new one every time. + // Also -- why is base.getData() tested specifically here? If that returns + // NULL, shouldn't we bail out of the whole method? if (!mInputp && base.getData()) { // The compressed data has been loaded @@ -301,6 +311,9 @@ void LLImageJ2CKDU::cleanupCodeStream() mTileIndicesp.reset(); } +// This is the protected virtual method called by LLImageJ2C::initDecode(). +// However, as far as nat can tell, LLImageJ2C::initDecode() is called only by +// llimage_libtest.cpp's load_image() function. No detectable production use. bool LLImageJ2CKDU::initDecode(LLImageJ2C &base, LLImageRaw &raw_image, int discard_level, int* region) { return initDecode(base,raw_image,0.0f,MODE_FAST,0,4,discard_level,region); @@ -333,6 +346,9 @@ bool LLImageJ2CKDU::initEncode(LLImageJ2C &base, LLImageRaw &raw_image, int bloc return true; } +// This is the real (private) initDecode() called both by the protected +// initDecode() method and by decodeImpl(). As far as nat can tell, only the +// decodeImpl() usage matters for production. bool LLImageJ2CKDU::initDecode(LLImageJ2C &base, LLImageRaw &raw_image, F32 decode_time, ECodeStreamMode mode, S32 first_channel, S32 max_channel_count, int discard_level, int* region) { base.resetLastError(); -- cgit v1.2.3 From 80adc9b6aa9814c74fd0c67c5de9d1765a089925 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 5 Aug 2016 17:57:24 -0400 Subject: MAINT-6584: Introduce KDUError exception from other viewer project. Specifically, manually apply changesets b4db8a8 and b98371d from nat_linden/viewer-mac-mainloop. We need to throw from a new place, but if we threw const char* (current convention), the new throw wouldn't be patched when we merge to new exception convention. --- indra/llkdu/llimagej2ckdu.cpp | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) (limited to 'indra') diff --git a/indra/llkdu/llimagej2ckdu.cpp b/indra/llkdu/llimagej2ckdu.cpp index 1201170f6d..ba1560ec56 100644 --- a/indra/llkdu/llimagej2ckdu.cpp +++ b/indra/llkdu/llimagej2ckdu.cpp @@ -34,6 +34,15 @@ #include "kdu_block_coding.h" +#include + +namespace { +struct KDUError: public std::runtime_error +{ + KDUError(const std::string& msg): std::runtime_error(msg) {} +}; +} // anonymous namespace + class kdc_flow_control { public: @@ -173,7 +182,7 @@ struct LLKDUMessageError : public LLKDUMessage // shutdown will NOT engage the behavior described above. if (end_of_message) { - throw "KDU throwing an exception"; + throw KDUError("LLKDUMessageError::flush()"); } } }; @@ -406,9 +415,9 @@ bool LLImageJ2CKDU::initDecode(LLImageJ2C &base, LLImageRaw &raw_image, F32 deco mTPosp->x = 0; } } - catch (const char* msg) + catch (const KDUError& msg) { - base.setLastError(ll_safe_string(msg)); + base.setLastError(msg.what()); return false; } catch (...) @@ -502,9 +511,9 @@ bool LLImageJ2CKDU::decodeImpl(LLImageJ2C &base, LLImageRaw &raw_image, F32 deco return false; } } - catch (const char* msg) + catch (const KDUError& msg) { - base.setLastError(ll_safe_string(msg)); + base.setLastError(msg.what()); base.decodeFailed(); cleanupCodeStream(); return true; // done @@ -695,9 +704,9 @@ bool LLImageJ2CKDU::encodeImpl(LLImageJ2C &base, const LLImageRaw &raw_image, co base.updateData(); // set width, height delete[] output_buffer; } - catch(const char* msg) + catch(const KDUError& msg) { - base.setLastError(ll_safe_string(msg)); + base.setLastError(msg.what()); return false; } catch( ... ) @@ -719,9 +728,9 @@ bool LLImageJ2CKDU::getMetadata(LLImageJ2C &base) setupCodeStream(base, false, MODE_FAST); return true; } - catch (const char* msg) + catch (const KDUError& msg) { - base.setLastError(ll_safe_string(msg)); + base.setLastError(msg.what()); return false; } catch (...) -- cgit v1.2.3 From 1773f44b6d9fffa7f5d0b2449184b2998e99be5a Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sat, 6 Aug 2016 11:39:11 -0400 Subject: MAINT-6584: Don't crash on inconsistent dims in a JPEG-2000 image. Previous code would crump with LL_ERRS. But a bad image file should fail only the image load -- not crash the viewer. While at it, validate all components present, not just 0, 1, 2. While at it, make the failure message report which component and what the mismatched dimensions are, not just "Components don't have matching dimensions!" --- indra/llkdu/llimagej2ckdu.cpp | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) (limited to 'indra') diff --git a/indra/llkdu/llimagej2ckdu.cpp b/indra/llkdu/llimagej2ckdu.cpp index ba1560ec56..025c77b85e 100644 --- a/indra/llkdu/llimagej2ckdu.cpp +++ b/indra/llkdu/llimagej2ckdu.cpp @@ -31,18 +31,31 @@ #include "llpointer.h" #include "llmath.h" #include "llkdumem.h" +#include "stringize.h" #include "kdu_block_coding.h" #include +#include namespace { +// exception used to keep KDU from terminating entire program -- see comments +// in LLKDUMessageError::flush() struct KDUError: public std::runtime_error { - KDUError(const std::string& msg): std::runtime_error(msg) {} + KDUError(const std::string& msg): std::runtime_error(msg) {} }; } // anonymous namespace +// stream kdu_dims to std::ostream +// Turns out this must NOT be in the anonymous namespace! +inline +std::ostream& operator<<(std::ostream& out, const kdu_dims& dims) +{ + return out << "(" << dims.pos.x << "," << dims.pos.y << ")," + "[" << dims.size.x << "x" << dims.size.y << "]"; +} + class kdc_flow_control { public: @@ -287,13 +300,19 @@ void LLImageJ2CKDU::setupCodeStream(LLImageJ2C &base, bool keep_codestream, ECod S32 components = mCodeStreamp->get_num_components(); - if (components >= 3) - { // Check that components have consistent dimensions (for PPM file) - kdu_dims dims1; mCodeStreamp->get_dims(1,dims1); - kdu_dims dims2; mCodeStreamp->get_dims(2,dims2); - if ((dims1 != dims) || (dims2 != dims)) + // Check that components have consistent dimensions (for PPM file) + for (int idx = 1; idx < components; ++idx) + { + kdu_dims other_dims; + mCodeStreamp->get_dims(idx, other_dims); + if (other_dims != dims) { - LL_ERRS() << "Components don't have matching dimensions!" << LL_ENDL; + // This method is only called from methods that catch KDUError. + // We want to fail the image load, not crash the viewer. + throw KDUError(STRINGIZE("Component " << idx << " dimensions " + << other_dims + << " do not match component 0 dimensions " + << dims << "!")); } } -- cgit v1.2.3