Automate memory management in JPEG2KEncode and JPEG2KDecode.

Instead of using strdup() and free() to store comment_text in JPEG2KEncode,
store a std::string. Similarly, instead of manually managing pointers to
encoder, decoder, image, stream and codestream_info in JPEG2KDecode and
JPEG2KEncode, use std::unique_ptr for each, specifying their respective
deleter functions.

1 files changed, 88 insertions, 144 deletions

diff --git a/indra/llimagej2coj/llimagej2coj.cpp b/indra/llimagej2coj/llimagej2coj.cpp
index 6037517103..a2a43e4baa 100644
--- a/indra/llimagej2coj/llimagej2coj.cpp
+++ b/indra/llimagej2coj/llimagej2coj.cpp
@@ -31,6 +31,8 @@
 #include "openjpeg.h"
 #include "event.h"
 #include "cio.h"
+#include <memory>                   // std::unique_ptr
+#include <string>
 
 #define MAX_ENCODED_DISCARD_LEVELS 5
 
@@ -231,33 +233,6 @@ public:
         parameters.cp_reduce = discardLevel;
     }
 
-    ~JPEG2KDecode()
-    {
-        if (decoder)
-        {
-            opj_destroy_codec(decoder);
-        }
-        decoder = nullptr;
-
-        if (image)
-        {
-            opj_image_destroy(image);
-        }
-        image = nullptr;
-
-        if (stream)
-        {
-            opj_stream_destroy(stream);
-        }
-        stream = nullptr;
-
-        if (codestream_info)
-        {
-            opj_destroy_cstr_info(&codestream_info);
-        }
-        codestream_info = nullptr;
-    }
-
     bool readHeader(
         U8* data,
         U32 dataSize,
@@ -268,46 +243,42 @@ public:
     {
         parameters.flags |= OPJ_DPARAMETERS_DUMP_FLAG;
 
-        decoder = opj_create_decompress(OPJ_CODEC_J2K);
+        decoder.reset(opj_create_decompress(OPJ_CODEC_J2K));
 
-        if (!opj_setup_decoder(decoder, &parameters))
+        if (!opj_setup_decoder(decoder.get(), &parameters))
         {
             return false;
         }
 
-        if (stream)
-        {
-            opj_stream_destroy(stream);
-        }
-
-        stream = opj_stream_create(dataSize, true);
+        stream.reset(opj_stream_create(dataSize, true));
         if (!stream)
         {
             return false;
         }
 
-        opj_stream_set_user_data(stream, this, opj_free_user_data);
-        opj_stream_set_user_data_length(stream, dataSize);
-        opj_stream_set_read_function(stream, opj_read);
-        opj_stream_set_write_function(stream, opj_write);
-        opj_stream_set_skip_function(stream, opj_skip);
-        opj_stream_set_seek_function(stream, opj_seek);
+        opj_stream_set_user_data(stream.get(), this, opj_free_user_data);
+        opj_stream_set_user_data_length(stream.get(), dataSize);
+        opj_stream_set_read_function(stream.get(), opj_read);
+        opj_stream_set_write_function(stream.get(), opj_write);
+        opj_stream_set_skip_function(stream.get(), opj_skip);
+        opj_stream_set_seek_function(stream.get(), opj_seek);
 
         buffer = data;
         size = dataSize;
         offset = 0;
 
         // enable decoding partially loaded images
-        opj_decoder_set_strict_mode(decoder, OPJ_FALSE);
+        opj_decoder_set_strict_mode(decoder.get(), OPJ_FALSE);
 
         /* Read the main header of the codestream and if necessary the JP2 boxes*/
-        if (!opj_read_header((opj_stream_t*)stream, decoder, &image))
+        opj_image_t* img;
+        if (!opj_read_header(stream.get(), decoder.get(), &img))
         {
             return false;
         }
+        image.reset(img);
 
-        codestream_info = opj_get_cstr_info(decoder);
-
+        codestream_info.reset(opj_get_cstr_info(decoder.get()));
         if (!codestream_info)
         {
             return false;
@@ -337,51 +308,44 @@ public:
     {
         parameters.flags &= ~OPJ_DPARAMETERS_DUMP_FLAG;
 
-        decoder = opj_create_decompress(OPJ_CODEC_J2K);
-        opj_setup_decoder(decoder, &parameters);
+        decoder.reset(opj_create_decompress(OPJ_CODEC_J2K));
+        opj_setup_decoder(decoder.get(), &parameters);
 
-        opj_set_info_handler(decoder, opj_info, this);
-        opj_set_warning_handler(decoder, opj_warn, this);
-        opj_set_error_handler(decoder, opj_error, this);
+        opj_set_info_handler(decoder.get(), opj_info, this);
+        opj_set_warning_handler(decoder.get(), opj_warn, this);
+        opj_set_error_handler(decoder.get(), opj_error, this);
 
-        if (stream)
-        {
-            opj_stream_destroy(stream);
-        }
-
-        stream = opj_stream_create(dataSize, true);
+        stream.reset(opj_stream_create(dataSize, true));
         if (!stream)
         {
             return false;
         }
 
-        opj_stream_set_user_data(stream, this, opj_free_user_data);
-        opj_stream_set_user_data_length(stream, dataSize);
-        opj_stream_set_read_function(stream, opj_read);
-        opj_stream_set_write_function(stream, opj_write);
-        opj_stream_set_skip_function(stream, opj_skip);
-        opj_stream_set_seek_function(stream, opj_seek);
+        opj_stream_set_user_data(stream.get(), this, opj_free_user_data);
+        opj_stream_set_user_data_length(stream.get(), dataSize);
+        opj_stream_set_read_function(stream.get(), opj_read);
+        opj_stream_set_write_function(stream.get(), opj_write);
+        opj_stream_set_skip_function(stream.get(), opj_skip);
+        opj_stream_set_seek_function(stream.get(), opj_seek);
 
         buffer = data;
         size = dataSize;
         offset = 0;
 
-        if (image)
-        {
-            opj_image_destroy(image);
-            image = nullptr;
-        }
+        image.reset();
 
         // needs to happen before opj_read_header and opj_decode...
-        opj_set_decoded_resolution_factor(decoder, discard_level);
+        opj_set_decoded_resolution_factor(decoder.get(), discard_level);
 
         // enable decoding partially loaded images
-        opj_decoder_set_strict_mode(decoder, OPJ_FALSE);
+        opj_decoder_set_strict_mode(decoder.get(), OPJ_FALSE);
 
-        if (!opj_read_header(stream, decoder, &image))
+        opj_image_t* img;
+        if (!opj_read_header(stream.get(), decoder.get(), &img))
         {
             return false;
         }
+        image.reset(img);
 
         // needs to happen before decode which may fail
         if (channels)
@@ -389,30 +353,36 @@ public:
             *channels = image->numcomps;
         }
 
-        OPJ_BOOL decoded = opj_decode(decoder, stream, image);
+        OPJ_BOOL decoded = opj_decode(decoder.get(), stream.get(), image.get());
 
         // count was zero.  The latter is just a sanity check before we
         // dereference the array.
-        if (!decoded || !image || !image->numcomps)
-        {
-            opj_end_decompress(decoder, stream);
-            return false;
-        }
-
-        opj_end_decompress(decoder, stream);
-
-        return true;
+        bool result = (decoded && image && image->numcomps);
+        opj_end_decompress(decoder.get(), stream.get());
+        return result;
     }
 
-    opj_image_t* getImage() { return image; }
+    opj_image_t* getImage() { return image.get(); }
 
 private:
+    // opj_destroy_cstr_info(opj_codestream_info_v2_t**) requires a pointer to
+    // pointer, which is too bad because otherwise we could directly pass that
+    // function as the unique_ptr's deleter.
+    static void cstr_info_deleter(opj_codestream_info_v2_t* doomed)
+    {
+        opj_destroy_cstr_info(&doomed);
+    }
+
     opj_dparameters_t         parameters;
     opj_event_mgr_t           event_mgr;
-    opj_image_t*              image = nullptr;
-    opj_codec_t*              decoder = nullptr;
-    opj_stream_t*             stream = nullptr;
-    opj_codestream_info_v2_t* codestream_info = nullptr;
+    std::unique_ptr<opj_codestream_info_v2_t, decltype(cstr_info_deleter)*>
+                              codestream_info{ nullptr, cstr_info_deleter };
+    std::unique_ptr<opj_stream_t, decltype(opj_stream_destroy)*>
+                              stream{ nullptr, opj_stream_destroy };
+    std::unique_ptr<opj_image_t, decltype(opj_image_destroy)*>
+                              image{ nullptr, opj_image_destroy };
+    std::unique_ptr<opj_codec_t, decltype(opj_destroy_codec)*>
+                              decoder{ nullptr, opj_destroy_codec };
 };
 
 class JPEG2KEncode : public JPEG2KBase
@@ -447,43 +417,19 @@ public:
             parameters.irreversible = 1;
         }
 
-        if (comment_text)
-        {
-            free(comment_text);
-        }
-        comment_text = comment_text_in ? strdup(comment_text_in) : nullptr;
+        comment_text = { comment_text_in? comment_text_in : "no comment" };
 
-        parameters.cp_comment = comment_text ? comment_text : (char*)"no comment";
+        // Because comment_text is a member declared before parameters,
+        // it will outlive parameters, so we can safely store in parameters a
+        // pointer into comment_text's data. Unfortunately cp_comment is
+        // declared as (non-const) char*. We just have to trust that this is
+        // legacy C style coding, rather than any intention to modify the
+        // comment string. (If there was actual modification, we could use a
+        // std::vector<char> instead, but let's only go there if we must.)
+        parameters.cp_comment = const_cast<char*>(comment_text.c_str());
         llassert(parameters.cp_comment);
     }
 
-    ~JPEG2KEncode()
-    {
-        if (encoder)
-        {
-            opj_destroy_codec(encoder);
-        }
-        encoder = nullptr;
-
-        if (image)
-        {
-            opj_image_destroy(image);
-        }
-        image = nullptr;
-
-        if (stream)
-        {
-            opj_stream_destroy(stream);
-        }
-        stream = nullptr;
-
-        if (comment_text)
-        {
-            free(comment_text);
-        }
-        comment_text = nullptr;
-    }
-
     bool encode(const LLImageRaw& rawImageIn, LLImageJ2C &compressedImageOut)
     {
         LLImageDataSharedLock lockIn(&rawImageIn);
@@ -491,7 +437,7 @@ public:
 
         setImage(rawImageIn);
 
-        encoder = opj_create_compress(OPJ_CODEC_J2K);
+        encoder.reset(opj_create_compress(OPJ_CODEC_J2K));
 
         parameters.tcp_mct = (image->numcomps >= 3) ? 1 : 0;
         parameters.cod_format = OPJ_CODEC_J2K;
@@ -542,14 +488,14 @@ public:
             parameters.max_cs_size = max_cs_size;
         }
 
-        if (!opj_setup_encoder(encoder, &parameters, image))
+        if (!opj_setup_encoder(encoder.get(), &parameters, image.get()))
         {
             return false;
         }
 
-        opj_set_info_handler(encoder, opj_info, this);
-        opj_set_warning_handler(encoder, opj_warn, this);
-        opj_set_error_handler(encoder, opj_error, this);
+        opj_set_info_handler(encoder.get(), opj_info, this);
+        opj_set_warning_handler(encoder.get(), opj_warn, this);
+        opj_set_error_handler(encoder.get(), opj_error, this);
 
         U32 tile_count = (rawImageIn.getWidth() >> 6) * (rawImageIn.getHeight() >> 6);
         U32 data_size_guess = tile_count * TILE_SIZE;
@@ -561,37 +507,32 @@ public:
 
         memset(buffer, 0, data_size_guess);
 
-        if (stream)
-        {
-            opj_stream_destroy(stream);
-        }
-
-        stream = opj_stream_create(data_size_guess, false);
+        stream.reset(opj_stream_create(data_size_guess, false));
         if (!stream)
         {
             return false;
         }
 
-        opj_stream_set_user_data(stream, this, opj_free_user_data_write);
-        opj_stream_set_user_data_length(stream, data_size_guess);
-        opj_stream_set_read_function(stream, opj_read);
-        opj_stream_set_write_function(stream, opj_write);
-        opj_stream_set_skip_function(stream, opj_skip);
-        opj_stream_set_seek_function(stream, opj_seek);
+        opj_stream_set_user_data(stream.get(), this, opj_free_user_data_write);
+        opj_stream_set_user_data_length(stream.get(), data_size_guess);
+        opj_stream_set_read_function(stream.get(), opj_read);
+        opj_stream_set_write_function(stream.get(), opj_write);
+        opj_stream_set_skip_function(stream.get(), opj_skip);
+        opj_stream_set_seek_function(stream.get(), opj_seek);
 
-        OPJ_BOOL started = opj_start_compress(encoder, image, stream);
+        OPJ_BOOL started = opj_start_compress(encoder.get(), image.get(), stream.get());
 
         if (!started)
         {
             return false;
         }
 
-        if (!opj_encode(encoder, stream))
+        if (!opj_encode(encoder.get(), stream.get()))
         {
             return false;
         }
 
-        OPJ_BOOL encoded = opj_end_compress(encoder, stream);
+        OPJ_BOOL encoded = opj_end_compress(encoder.get(), stream.get());
 
         // if we successfully encoded, then stream out the compressed data...
         if (encoded)
@@ -624,7 +565,7 @@ public:
             cmptparm[c].h = height;
         }
 
-        image = opj_image_create(numcomps, &cmptparm[0], OPJ_CLRSPC_SRGB);
+        image.reset(opj_image_create(numcomps, &cmptparm[0], OPJ_CLRSPC_SRGB));
 
         image->x1 = width;
         image->y1 = height;
@@ -739,15 +680,18 @@ public:
         }*/
     }
 
-    opj_image_t* getImage() { return image; }
+    opj_image_t* getImage() { return image.get(); }
 
 private:
+    std::string         comment_text;
     opj_cparameters_t   parameters;
     opj_event_mgr_t     event_mgr;
-    opj_image_t*        image = nullptr;
-    opj_codec_t*        encoder = nullptr;
-    opj_stream_t*       stream = nullptr;
-    char*               comment_text = nullptr;
+    std::unique_ptr<opj_stream_t, decltype(opj_stream_destroy)*>
+                        stream{ nullptr,   opj_stream_destroy };
+    std::unique_ptr<opj_image_t, decltype(opj_image_destroy)*>
+                        image{ nullptr,   opj_image_destroy };
+    std::unique_ptr<opj_codec_t, decltype(opj_destroy_codec)*>
+                        encoder{ nullptr, opj_destroy_codec };
 };