-
-
Notifications
You must be signed in to change notification settings - Fork 851
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
Add support for decoding webp images with animations #1985
Conversation
I think this one will have to wait 'til after 2.1 It's a lot to review. You're a machine! |
Sure, no problem |
# Conflicts: # src/ImageSharp/Formats/Webp/WebpDecoder.cs
# Conflicts: # src/ImageSharp/Formats/Webp/WebpDecoderCore.cs # tests/ImageSharp.Tests/TestImages.cs
@brianpopow I've started reviewing this. Late now though so will pick up tomorrow. |
Does this PR also handles support for Encode animated webp images. ie converting Gif to Webp |
No, only decoding is covered with this PR, but I plan to add support for encoding once this PR is merged. |
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.
Just some questions mostly plus some performance considerations.
ref TPixel srcPixel = ref srcPixelRow[x]; | ||
ref TPixel dstPixel = ref dstPixelRow[x]; | ||
srcPixel.ToRgba32(ref srcRgba); | ||
dstPixel.ToRgba32(ref dstRgba); |
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.
It would be better if we could convert the rows using bulk operations per scanline
|
||
if (srcRgba.A is not 0) | ||
{ | ||
int dstFactorA = dstRgba.A * (255 - srcRgba.A) / 255; |
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 reads like an operation we should already have code for?
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.
@brianpopow Correct me if I'm wrong but are we not simply drawing the frame over the canvas here using normal src-over with an opacity of 1?
If that's the case we can use the bulk operation just like we do in DrawImageProcessor<TPixel>
?
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.
Yeah, I think you are right. I will try that.
edit: changed with 980a1f0
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.
Really nice work 👍
Hope support for encoding webp images with animations is coming soon. |
@iAmBipinPaul yeah no mate. Don’t comment with rubbish like that. Do it again and you’ll be banned. It happens when it happens. Everyone contributing does so in their spare time. |
Yes, I know. Maybe English is not my first language so. Thank you! |
Prerequisites
Description
This PR will add support for decoding webp images with animations.
Related to #1802