Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions include/avif/avif_cxx.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,20 @@ namespace avif
// Struct to call the destroy functions in a unique_ptr.
struct UniquePtrDeleter
{
void operator()(avifEncoder * encoder) const { avifEncoderDestroy(encoder); }
void operator()(avifDecoder * decoder) const { avifDecoderDestroy(decoder); }
void operator()(avifImage * image) const { avifImageDestroy(image); }
void operator()(avifEncoder * encoder) const { avifEncoderDestroy(encoder); }
void operator()(avifGainMap * gainMap) const { avifGainMapDestroy(gainMap); }
void operator()(avifImage * image) const { avifImageDestroy(image); }
void operator()(avifRGBImage * image) const { avifRGBImageFreePixels(image); }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vincent: This doesn't seem correct. avifRGBImageFreePixels() is implemented as follows:

void avifRGBImageFreePixels(avifRGBImage * rgb)
{
    if (rgb->pixels) {
        avifFree(rgb->pixels);
    }

    rgb->pixels = NULL;
    rgb->rowBytes = 0;
}

Note that it doesn't free the avifRGBImage struct. Could you modify an existing test in tests/gtest/ to show how avif::RGBImagePtr should be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix at #3048

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vincent: I thought about this class last night. I would like to suggest two alternatives.

The first alternative is to preserve the main functionality of RGBImagePtr but rename it RGBImageCleanup and define the class directly rather than using std::unique_ptr:

class RGBImageCleanup {
 public:
  RGBImageCleanup(avifRGBImage* rgb) : rgb_(rgb) {}
  ~RGBImageCleanup() { avifRGBImageFreePixels(rgb_); }
 private:
  avifRGBImage* rgb_ = nullptr;
};

By not using std::unique_ptr, we avoid the confusion caused by expecting it to free the avifRGBImage struct.

The second alternative is to derive a C++ RGBImage class from the C avifRGBImage struct. The destructor calls avifRGBImageFreePixels(this). The class may optionally provide constructors and methods such as AllocatePixels() for convenience.

My preference is the first alternative because it is the minimal solution to the cleanup problem. It is similar to absl::Cleanup (or its predecessor in Google's internal repository).

};

// Use these unique_ptr to ensure the structs are automatically destroyed.
using EncoderPtr = std::unique_ptr<avifEncoder, UniquePtrDeleter>;
using DecoderPtr = std::unique_ptr<avifDecoder, UniquePtrDeleter>;
using ImagePtr = std::unique_ptr<avifImage, UniquePtrDeleter>;
using EncoderPtr = std::unique_ptr<avifEncoder, UniquePtrDeleter>;
using GainMapPtr = std::unique_ptr<avifGainMap, UniquePtrDeleter>;
using ImagePtr = std::unique_ptr<avifImage, UniquePtrDeleter>;
// To use when RGBImage actually owns the pixels. RGBImage can also be used as a view, in which case it does not own the pixels.
using RGBImagePtr = std::unique_ptr<avifRGBImage, UniquePtrDeleter>;

} // namespace avif

Expand Down
Loading