Skip to content

Comments

Add avifRGBImage to the C++ API#3042

Merged
vrabaud merged 3 commits intoAOMediaCodec:mainfrom
vrabaud:main
Feb 18, 2026
Merged

Add avifRGBImage to the C++ API#3042
vrabaud merged 3 commits intoAOMediaCodec:mainfrom
vrabaud:main

Conversation

@vrabaud
Copy link
Contributor

@vrabaud vrabaud commented Feb 17, 2026

No description provided.

@vrabaud vrabaud requested a review from maryla-uc February 17, 2026 14:18
@maryla-uc
Copy link
Contributor

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.
Maybe we can just call it RGBImageOwnedPixelsPtr ...? With a comment explaining usage?

@vrabaud vrabaud requested review from y-guyon and removed request for y-guyon February 17, 2026 14:43
Copy link
Contributor

@maryla-uc maryla-uc left a comment

Choose a reason for hiding this comment

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

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.

@vrabaud
Copy link
Contributor Author

vrabaud commented Feb 18, 2026

Let's go with RGBImagePtr + comment

@vrabaud vrabaud merged commit 4f927e8 into AOMediaCodec:main Feb 18, 2026
25 checks passed
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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants