Replace RGBImagePtr by RGBImageCleanup#3048
Replace RGBImagePtr by RGBImageCleanup#3048vrabaud wants to merge 1 commit intoAOMediaCodec:mainfrom
Conversation
vrabaud
force-pushed
the
main
branch
4 times, most recently
from
831c5e3 to
2e74fb1
Compare
wantehchang
left a comment
wantehchang
left a comment
There was a problem hiding this comment.
LGTM. Thanks. I suggest some changes.
| // 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>; | ||
|
|
||
| // Automatically cleans the ressources of the avifRGBImage if it owns it. |
There was a problem hiding this comment.
Nit: "if it owns it" is a conditional, but the destructor calls avifRGBImageFreePixels() unconditionally. I think the comment should instead say that this class assumes the avifRGBImage owns its pixels buffer.
| testutil::CreateImage(/*width=*/12, /*height=*/34, /*depth=*/8, | ||
| AVIF_PIXEL_FORMAT_YUV420, AVIF_PLANES_ALL); | ||
|
|
||
| std::unique_ptr<avifRGBImage> rgb(new avifRGBImage()); |
There was a problem hiding this comment.
Change this to a plain avifRGBImage local variable allocated on the stack. That is how avifRGBImage is intended to be used. avifRGBImage is really a struct that collects the parameters for the RGB-to-YUV and YUV-to-RGB conversion functions.
We should also create a local variable rather than a temporary object for RGBImageCleanup. The local variable cleanup is created as soon as the avifRGBImage variable is properly initialized:
avifRGBImage rgb;
avifRGBImageSetDefaults(&rgb, image.get());
RGBImageCleanup cleanup(&rgb);
ASSERT_EQ(avifRGBImageAllocatePixels(&rgb), AVIF_RESULT_OK);
| // testutil::WriteImage(decoded.get(), "/tmp/avifbasictest.png"); | ||
| } | ||
|
|
||
| TEST(BasicTest, SharedPtr) { |
There was a problem hiding this comment.
Nit: "SharedPtr" doesn't seem to match what the test does...
2 participants
No description provided.