Skip to content
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

Merged
merged 22 commits into from
May 17, 2022
Merged

Conversation

brianpopow
Copy link
Collaborator

@brianpopow brianpopow commented Feb 9, 2022

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

This PR will add support for decoding webp images with animations.

Related to #1802

@brianpopow brianpopow changed the title WIP: Add support for decoding webp images with animations Add support for decoding webp images with animations Feb 10, 2022
@JimBobSquarePants JimBobSquarePants requested a review from a team March 7, 2022 02:42
@JimBobSquarePants
Copy link
Member

I think this one will have to wait 'til after 2.1 It's a lot to review. You're a machine!

@JimBobSquarePants JimBobSquarePants added this to the Future milestone Mar 15, 2022
@brianpopow
Copy link
Collaborator Author

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
@JimBobSquarePants JimBobSquarePants modified the milestones: Future, 3.0.0 Mar 27, 2022
# Conflicts:
#	src/ImageSharp/Formats/Webp/WebpDecoderCore.cs
#	tests/ImageSharp.Tests/TestImages.cs
@JimBobSquarePants
Copy link
Member

@brianpopow I've started reviewing this. Late now though so will pick up tomorrow.

@iAmBipinPaul
Copy link

Does this PR also handles support for Encode animated webp images.

ie converting Gif to Webp

#1802

@brianpopow
Copy link
Collaborator Author

Does this PR also handles support for Encode animated webp images.

ie converting Gif to Webp

#1802

No, only decoding is covered with this PR, but I plan to add support for encoding once this PR is merged.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a 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.

src/ImageSharp/Formats/Webp/WebpAnimationDecoder.cs Outdated Show resolved Hide resolved
ref TPixel srcPixel = ref srcPixelRow[x];
ref TPixel dstPixel = ref dstPixelRow[x];
srcPixel.ToRgba32(ref srcRgba);
dstPixel.ToRgba32(ref dstRgba);
Copy link
Member

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

src/ImageSharp/Formats/Webp/WebpAnimationDecoder.cs Outdated Show resolved Hide resolved

if (srcRgba.A is not 0)
{
int dstFactorA = dstRgba.A * (255 - srcRgba.A) / 255;
Copy link
Member

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?

Copy link
Member

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> ?

Copy link
Collaborator Author

@brianpopow brianpopow May 17, 2022

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

src/ImageSharp/Formats/Webp/WebpChunkParsingUtils.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Webp/WebpChunkParsingUtils.cs Outdated Show resolved Hide resolved
Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

Really nice work 👍

@brianpopow brianpopow merged commit c661ab1 into main May 17, 2022
@brianpopow brianpopow deleted the bp/webpanimation branch May 17, 2022 12:57
@iAmBipinPaul
Copy link

Hope support for encoding webp images with animations is coming soon.

@JimBobSquarePants
Copy link
Member

@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.

@iAmBipinPaul
Copy link

Yes, I know.
I will keep this in mind but my intentions was not like demanding here.

Maybe English is not my first language so.

Thank you!

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

Successfully merging this pull request may close these issues.

3 participants