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 PremultiplyAlpha to ResizeOptions #1504

Merged
merged 3 commits into from
Jan 14, 2021
Merged

Add PremultiplyAlpha to ResizeOptions #1504

merged 3 commits into from
Jan 14, 2021

Conversation

ptasev
Copy link
Sponsor Contributor

@ptasev ptasev commented Jan 13, 2021

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

Added PremultiplyAlpha to the ResizeOptions. This option was always on by default, but now the user has the ability to turn it off through the options.

Fixes #1498

@CLAassistant
Copy link

CLAassistant commented Jan 13, 2021

CLA assistant check
All committers have signed the CLA.

PixelConversionModifiers.Premultiply.ApplyCompanding(compand);
PixelConversionModifiers conversionModifiers = premultiplyAlpha ?
PixelConversionModifiers.Premultiply.ApplyCompanding(compand) :
PixelConversionModifiers.None.ApplyCompanding(compand);
Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Applying compand to the None modifier. Not sure if correct.

Copy link
Member

@antonfirsov antonfirsov Jan 13, 2021

Choose a reason for hiding this comment

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

It's OK, although not super happy about how it looks now. Would replace the ApplyCompanding helper with something like:

public static PixelConversionModifiers GetModifiers(bool compand, bool premultiplyAlpha);

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I split it out into a separate private function. I hope that's what you meant.

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with the private function for now. This is the only place that uses it.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

LGTM with a minor suggestion.

Opened SixLabors/Imagesharp.Tests.Images#14 with your images. As soon as it gets merged, we need to push an update of the submodule reference for tests/Images/External.

I leave the approval & merge of the submodule PR tor @JimBobSquarePants since we need his final blessing anyways.

Thanks for taking the time!

PixelConversionModifiers.Premultiply.ApplyCompanding(compand);
PixelConversionModifiers conversionModifiers = premultiplyAlpha ?
PixelConversionModifiers.Premultiply.ApplyCompanding(compand) :
PixelConversionModifiers.None.ApplyCompanding(compand);
Copy link
Member

@antonfirsov antonfirsov Jan 13, 2021

Choose a reason for hiding this comment

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

It's OK, although not super happy about how it looks now. Would replace the ApplyCompanding helper with something like:

public static PixelConversionModifiers GetModifiers(bool compand, bool premultiplyAlpha);

@JimBobSquarePants
Copy link
Member

Reference image merged.

git submodule foreach git pull origin master

Will update all the modules.

@codecov
Copy link

codecov bot commented Jan 13, 2021

Codecov Report

Merging #1504 (770dc06) into master (5ab593f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1504   +/-   ##
=======================================
  Coverage   83.52%   83.52%           
=======================================
  Files         741      741           
  Lines       32664    32672    +8     
  Branches     3661     3662    +1     
=======================================
+ Hits        27281    27289    +8     
  Misses       4669     4669           
  Partials      714      714           
Flag Coverage Δ
unittests 83.52% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ng/Processors/Transforms/Resize/ResizeProcessor.cs 100.00% <100.00%> (ø)
...ssors/Transforms/Resize/ResizeProcessor{TPixel}.cs 100.00% <100.00%> (ø)
src/ImageSharp/Processing/ResizeOptions.cs 75.00% <100.00%> (+3.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ab593f...770dc06. Read the comment docs.

@JimBobSquarePants
Copy link
Member

Thanks for this @ptasev ! I'll get it merged for you now. It'll be available to you in our MyGet feed once built.

@JimBobSquarePants JimBobSquarePants merged commit da7a8b7 into SixLabors:master Jan 14, 2021
JimBobSquarePants added a commit that referenced this pull request Mar 13, 2021
Add PremultiplyAlpha to ResizeOptions
@ptasev ptasev deleted the pt/resize-alpha-option branch April 18, 2021 17:14
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.

Resize: enable opting-out for alpha premultiplication
4 participants