From 03bff896bd18b71c9a2d8e0b163647b1cd64b871 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 3 Aug 2016 20:40:03 -0400 Subject: MAINT-6584: Use RAII classes to manage helper object lifespans. Use boost::scoped_ptr instead of raw pointers to LLKDUMemSource, LLKDUDecodeState, kdu_coords and kdu_dims so cleanup is simpler, and automated on destruction of LLImageJ2CKDU. Replace pointer to kdu_codestream with a custom RAII class. kdu_codestream is itself an opaque handle, so we don't need to add another layer of indirection. Just wrap it to ensure its destroy() method is reliably called when needed. Make static instances of LLKDUMessageWarning and LLKDUMessageError self-register, eliminating the companion static bool and explicit checks in code. --- indra/llkdu/llimagej2ckdu.cpp | 98 +++++++++++++++++-------------------------- indra/llkdu/llimagej2ckdu.h | 45 +++++++++++++++++--- 2 files changed, 78 insertions(+), 65 deletions(-) (limited to 'indra/llkdu') diff --git a/indra/llkdu/llimagej2ckdu.cpp b/indra/llkdu/llimagej2ckdu.cpp index 0b2ac03b9d..0d37b123ec 100644 --- a/indra/llkdu/llimagej2ckdu.cpp +++ b/indra/llkdu/llimagej2ckdu.cpp @@ -138,15 +138,21 @@ struct LLKDUMessageWarning : public LLKDUMessage { LLKDUMessageWarning(): LLKDUMessage("Warning") - {} + { + kdu_customize_warnings(this); + } }; +// Instantiating LLKDUMessageWarning calls kdu_customize_warnings() with the +// new instance. Make it static so this only happens once. static LLKDUMessageWarning sWarningHandler; struct LLKDUMessageError : public LLKDUMessage { LLKDUMessageError(): LLKDUMessage("Error") - {} + { + kdu_customize_errors(this); + } virtual void flush(bool end_of_message = false) { @@ -165,20 +171,20 @@ struct LLKDUMessageError : public LLKDUMessage } } }; +// Instantiating LLKDUMessageError calls kdu_customize_errors() with the new +// instance. Make it static so this only happens once. static LLKDUMessageError sErrorHandler; -static bool kdu_message_initialized = false; - LLImageJ2CKDU::LLImageJ2CKDU() : LLImageJ2CImpl(), -mInputp(NULL), -mCodeStreamp(NULL), -mTPosp(NULL), -mTileIndicesp(NULL), -mRawImagep(NULL), -mDecodeState(NULL), -mBlocksSize(-1), -mPrecinctsSize(-1), -mLevels(0) + mInputp(), + mCodeStreamp(), + mTPosp(), + mTileIndicesp(), + mRawImagep(NULL), + mDecodeState(), + mBlocksSize(-1), + mPrecinctsSize(-1), + mLevels(0) { } @@ -198,38 +204,27 @@ void LLImageJ2CKDU::setupCodeStream(LLImageJ2C &base, bool keep_codestream, ECod // // Initialization // - if (!kdu_message_initialized) - { - kdu_message_initialized = true; - kdu_customize_errors(&sErrorHandler); - kdu_customize_warnings(&sWarningHandler); - } - - if (mCodeStreamp) - { - mCodeStreamp->destroy(); - delete mCodeStreamp; - mCodeStreamp = NULL; - } + mCodeStreamp.reset(); if (!mInputp && base.getData()) { // The compressed data has been loaded // Setup the source for the codestream - mInputp = new LLKDUMemSource(base.getData(), data_size); + mInputp.reset(new LLKDUMemSource(base.getData(), data_size)); } if (mInputp) { + // This is LLKDUMemSource::reset(), not boost::scoped_ptr::reset(). mInputp->reset(); } - mCodeStreamp = new kdu_codestream; - mCodeStreamp->create(mInputp); + mCodeStreamp->create(mInputp.get()); // Set the maximum number of bytes to use from the codestream - // *TODO: This seems to be wrong. The base class should have no idea of how j2c compression works so no - // good way of computing what's the byte range to be used. + // *TODO: This seems to be wrong. The base class should have no idea of + // how j2c compression works so no good way of computing what's the byte + // range to be used. mCodeStreamp->set_max_bytes(max_bytes,true); // If you want to flip or rotate the image for some reason, change @@ -286,34 +281,18 @@ void LLImageJ2CKDU::setupCodeStream(LLImageJ2C &base, bool keep_codestream, ECod if (!keep_codestream) { - mCodeStreamp->destroy(); - delete mCodeStreamp; - mCodeStreamp = NULL; - delete mInputp; - mInputp = NULL; + mCodeStreamp.reset(); + mInputp.reset(); } } void LLImageJ2CKDU::cleanupCodeStream() { - delete mInputp; - mInputp = NULL; - - delete mDecodeState; - mDecodeState = NULL; - - if (mCodeStreamp) - { - mCodeStreamp->destroy(); - delete mCodeStreamp; - mCodeStreamp = NULL; - } - - delete mTPosp; - mTPosp = NULL; - - delete mTileIndicesp; - mTileIndicesp = NULL; + mInputp.reset(); + mDecodeState.reset(); + mCodeStreamp.reset(); + mTPosp.reset(); + mTileIndicesp.reset(); } bool LLImageJ2CKDU::initDecode(LLImageJ2C &base, LLImageRaw &raw_image, int discard_level, int* region) @@ -395,12 +374,12 @@ bool LLImageJ2CKDU::initDecode(LLImageJ2C &base, LLImageRaw &raw_image, F32 deco if (!mTileIndicesp) { - mTileIndicesp = new kdu_dims; + mTileIndicesp.reset(new kdu_dims); } mCodeStreamp->get_valid_tiles(*mTileIndicesp); if (!mTPosp) { - mTPosp = new kdu_coords; + mTPosp.reset(new kdu_coords); mTPosp->y = 0; mTPosp->x = 0; } @@ -427,7 +406,7 @@ bool LLImageJ2CKDU::decodeImpl(LLImageJ2C &base, LLImageRaw &raw_image, F32 deco LLTimer decode_timer; - if (!mCodeStreamp) + if (!mCodeStreamp->exists()) { if (!initDecode(base, raw_image, decode_time, mode, first_channel, max_channel_count)) { @@ -478,15 +457,14 @@ bool LLImageJ2CKDU::decodeImpl(LLImageJ2C &base, LLImageRaw &raw_image, F32 deco kdu_coords offset = tile_dims.pos - dims.pos; int row_gap = channels*dims.size.x; // inter-row separation kdu_byte *buf = buffer + offset.y*row_gap + offset.x*channels; - mDecodeState = new LLKDUDecodeState(tile, buf, row_gap); + mDecodeState.reset(new LLKDUDecodeState(tile, buf, row_gap)); } // Do the actual processing F32 remaining_time = decode_time - decode_timer.getElapsedTimeF32(); // This is where we do the actual decode. If we run out of time, return false. if (mDecodeState->processTileDecode(remaining_time, (decode_time > 0.0f))) { - delete mDecodeState; - mDecodeState = NULL; + mDecodeState.reset(); } else { diff --git a/indra/llkdu/llimagej2ckdu.h b/indra/llkdu/llimagej2ckdu.h index da84749796..5fb093826e 100644 --- a/indra/llkdu/llimagej2ckdu.h +++ b/indra/llkdu/llimagej2ckdu.h @@ -48,6 +48,8 @@ #endif #include "kdu_sample_processing.h" +#include +#include class LLKDUDecodeState; class LLKDUMemSource; @@ -79,18 +81,51 @@ private: void setupCodeStream(LLImageJ2C &base, bool keep_codestream, ECodeStreamMode mode); void cleanupCodeStream(); + // Helper class to hold a kdu_codestream, which is a handle to the + // underlying implementation object. When CodeStreamHolder is reset() or + // destroyed, it calls kdu_codestream::destroy() -- which kdu_codestream + // itself does not. + // + // Call through it like a smart pointer using operator->(). + // + // Every RAII class must be noncopyable. For this we don't need move + // support. + class CodeStreamHolder: public boost::noncopyable + { + public: + ~CodeStreamHolder() + { + reset(); + } + + void reset() + { + if (mCodeStream.exists()) + { + mCodeStream.destroy(); + } + } + + kdu_codestream* operator->() { return &mCodeStream; } + + private: + kdu_codestream mCodeStream; + }; + // Encode variable - LLKDUMemSource *mInputp; - kdu_codestream *mCodeStreamp; - kdu_coords *mTPosp; // tile position - kdu_dims *mTileIndicesp; + boost::scoped_ptr mInputp; + CodeStreamHolder mCodeStreamp; + boost::scoped_ptr mTPosp; // tile position + boost::scoped_ptr mTileIndicesp; int mBlocksSize; int mPrecinctsSize; int mLevels; // Temporary variables for in-progress decodes... + // We don't own this LLImageRaw. We're simply pointing to an instance + // passed into initDecode(). LLImageRaw *mRawImagep; - LLKDUDecodeState *mDecodeState; + boost::scoped_ptr mDecodeState; }; #endif -- cgit v1.2.3