Tests/LibGfx: Add a CMYK TIFF roundtrip test, and basic CMYK JPEG coverage #26344
Conversation
github-actions
bot
added
the
👀 pr-needs-review
Tests/LibGfx/TestImageWriter.cpp
Outdated
| template<class Writer, class... ExtraArgs> | ||
| static ErrorOr<ByteBuffer> encode_bitmap(Gfx::CMYKBitmap const& bitmap, ExtraArgs... extra_args) | ||
| { | ||
| AllocatingMemoryStream stream; | ||
| TRY(Writer::encode(stream, bitmap, extra_args...)); | ||
| return stream.read_until_eof(); | ||
| } |
There was a problem hiding this comment.
Can't we make the Bitmap another template type and share the code with the function above?
There was a problem hiding this comment.
Thanks!
I thought that was a great idea, but when I did it I discovered that it requires adding *s all over the place, so now I'm no longer sure which version I prefer 😅 I pushed a version that does your three suggested templatized versions, but I made them extra commits since they require quite a few changes to existing test code. (…and I think I'd keep it like that for merging.)
It's IMHO still slightly nicer with the templates than what I originally had – but please shout if you didn't think of the type checking impact and don't like it after all :)
There was a problem hiding this comment.
Actually, I wouldn't mind taking NonnullRefPtr (const& ?) for these functions 🤔
I know this is not idiomatic to the codebase but if that prevents ~20 *s that might be a good move.
| { | ||
| // JPEG is lossy, so the roundtripped bitmap won't match the original bitmap. But it should still have the same size. | ||
| (void)TRY_OR_FAIL((get_roundtrip_bitmap<Gfx::JPEGWriter, Gfx::JPEGImageDecoderPlugin>(TRY_OR_FAIL(create_test_rgb_bitmap())))); | ||
| (void)TRY_OR_FAIL((get_roundtrip_bitmap<Gfx::JPEGWriter, Gfx::JPEGImageDecoderPlugin>(TRY_OR_FAIL(create_test_cmyk_bitmap())))); |
There was a problem hiding this comment.
We really need to implement some sort of safety net and have these tests check against a maximal accepted error. (Your commit is fine as is, I'm just thinking out loud).
Now that we have a lossless CMYK encoder, we can add a roundtrip test for CMYK encoding :^)
Since JPEG encoding is lossy, this only checks that the image size doesn't change during a roundtrip.
This allows us to remove the CMYKBitmap overload. For CMYKBitmap, we'll always go down the `if constexpr`. Requires manually converting all NonnullRefPtrs to const refs due to the stricter type checking on templates. No behavior change.
This allows us to share one version for Bitmap and CMYKBitmap overload. Requires manually converting all NonnullRefPtrs to const refs due to the stricter type checking on templates. No behavior change.
This allows us to share one version for Bitmap and CMYKBitmap overload. Requires manually converting all NonnullRefPtrs to const refs due to the stricter type checking on templates. No behavior change.
2 participants
Now that we have a lossless CMYK encoder, we can add a roundtrip
test for CMYK encoding :^)
Since JPEG encoding is lossy, the JPEG test only checks that the image size
doesn't change during a roundtrip.