-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LibGfx: Add APNG Writing Support to PNGWriter #24021
LibGfx: Add APNG Writing Support to PNGWriter #24021
Conversation
b2e88ea
to
120aa3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a graphics person so I'll let @nico or @LucasChollet weigh in on that. Generally it seems good. You do need to run clang-format on your changes though, that's what CI is complaining about.
566cd6a
to
4ddcf0a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems generally alright. Might be nice to include a test.
For the "screen recording" use case, it'd probably nice to have an API that streams the images instead of having to assemble them all into a big vector.
Also, maybe it makes sense to put this in the PR that adds a call to the new function? As is, this is adding a dead function. (Not the end of the world if its caller comes soon after.)
dbgln("frame #{}", sequence_number); | ||
TRY(writer.add_fcTL_chunk(sequence_number, b.width(), b.height(), delay_num, delay_den, dispose_op, blend_op)); | ||
sequence_number++; | ||
TRY(writer.add_IDAT_chunk(b, true, sequence_number)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is writing bitmap[0]
a second time. Is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APNG accepts two formats. In this, the thumbnail is separate from the first frame. In the other, it's the same as the first frame. I was originally going to allow the submission of a separate thumbnail image, but this probably has little purpose in this context. I might rework it a bit.
@@ -281,4 +322,26 @@ ErrorOr<ByteBuffer> PNGWriter::encode(Gfx::Bitmap const& bitmap, Options options | |||
return ByteBuffer::copy(writer.m_data); | |||
} | |||
|
|||
ErrorOr<ByteBuffer> PNGWriter::encodeAnimated(AK::Vector<Gfx::Bitmap const&> bitmap, u16 delay_num = 1, u16 delay_den = 30, u8 dispose_op = 0, u8 blend_op = 0, Options options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serenity style is encode_animated
Good point. Depends on how I do it. The APNG could be created somewhere, then appended onto with each frame. Right now, I'm not really sure where I could use the function, outside of the extension of shot I've begun to develop that allows recording of APNGs. |
I guess this can be used in pixel paint but that might require some work as I don't think we support animated images there. |
I was thinking this'd be a great opportunity for someone to add basic animation capabilities to PixelPaint. Will need to look into |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! |
We now have an |
f3f7daa
to
3d861e6
Compare
3d861e6
to
544e8b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to rebase it now that GIF support was merged :)
|
||
// Let's get rid of the previously written trailer | ||
if (!is_first_frame) | ||
TRY(m_stream.seek(-4, SeekMode::FromEndPosition)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I still have that in mind:
Please use SeekMode::FromCurrentPosition
(89aa18f)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers! Will get on that asap!
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! |
Last Minute Revive |
This is looking great, but has pending comments. Also, adding something to Tests/LibGfx/TestImageWriter.cpp, similar to test_webp_animation in there, would be good. Marking as "waiting for author" in the meantime :) |
I'm dropping this. Maybe I'll revisit this at some point, or someone else will add it in my place. Sorry! |
Based on SerenityOS#24021. Co-Authored-By: Pixel Brush <[email protected]>
I turned this into #24927. Thanks for the great starting point! |
Based on SerenityOS#24021. Co-Authored-By: Pixel Brush <[email protected]>
Based on #24021. Co-Authored-By: Pixel Brush <[email protected]>
This is a relatively straight-forward commit, allowing programs to export APNG files, with a custom frame duration, blend mode, etc.
This is also my first non-image commit for SerenityOS! (Woohoo!)
To Summarize what this adds: