Add avifRGBImage to the C++ API#3042
Conversation
|
The issue is that you are supposed to be able to also use RGBImage as a view, in which case it does not own the pixel, but it doesn't know it. RGBImage does not have any boolean saying whether it owns the pixels, it's the caller's responsibility to decide whether to call avifRGBImageFreePixels. |
maryla-uc
left a comment
There was a problem hiding this comment.
Now I'm wondering if RGBImageOwnedPixelsPtr makes it sounds like it's a pointer of pixels. RGBImageWithOwnedPixelsPtr is even longer but maybe more explicit? Or something else or leave it as is, as you see fit.
|
Let's go with RGBImagePtr + comment |
| 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); } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
No description provided.