Add new PixelAlphaRepresentation property and implement for TPixel types #1420
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1420 +/- ##
==========================================
+ Coverage 83.07% 83.63% +0.56%
==========================================
Files 707 733 +26
Lines 31831 31919 +88
Branches 3590 3590
==========================================
+ Hits 26445 26697 +252
+ Misses 4669 4508 -161
+ Partials 717 714 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
src/ImageSharp/PixelFormats/PixelImplementations/PixelOperations/Generated/_Common.ttinclude
Outdated
Show resolved
Hide resolved
Normally |
|
How are we doing here? I'd like to merge and move on. |
|
@JimBobSquarePants I can take a final look tomorrow (sill want to check the tests), but if its too late feel free to merge. You can also base next PR on current state since the product changes look good. |
|
Yeah, no worries. This PR adds a lot of (admittedly autogenerated) tests so well worth a review. |
tests/ImageSharp.Tests/PixelFormats/PixelOperations/Generated/_Common.ttinclude
Outdated
Show resolved
Hide resolved
tests/ImageSharp.Tests/PixelFormats/PixelOperations/Generated/_Common.ttinclude
Outdated
Show resolved
Hide resolved
...p.Tests/PixelFormats/PixelOperations/Generated/PixelOperationsTests.Specialized.Generated.tt
Show resolved
Hide resolved
| public void PixelTypeInfoHasCorrectAlphaRepresentation() | ||
| { | ||
| var alphaRepresentation = <#=pixelType#>.PixelOperations.Instance.GetPixelTypeInfo().AlphaRepresentation; | ||
| Assert.Equal(<#=alpha#>, alphaRepresentation); |
There was a problem hiding this comment.
I'm concerned whether this type of testing is valuable here. It's almost a copy-paste of the product logic.
I would rather try something like this in the base type:
[Fact]
public void PixelAlphaRepresentation_DefinesPresenceOfAlphaChannel()
{
TPixel pixel = default;
pixel.FromRgba32(new Rgba32(0, 0, 0, 123));
Rgba32 dest;
pixel.ToRgba32(ref dest);
bool hasAlpha = GetPixelTypeInfo().AlphaRepresentation != PixelAlphaRepresentation.None;
byte exectedAlpha = hasAlpha ? 123 : 255;
Assert.Equal(expectedAlpha, dest.A);
}There was a problem hiding this comment.
All I'm testing here is the correct assignment of the value. Any behavior based upon that value is the responsibility of the caller.
There was a problem hiding this comment.
That test could be useful though to check I assigned it correctly.
There was a problem hiding this comment.
All I'm testing here is the correct assignment of the value.
My point explained further:
it's very easy to make a mistake in such a test here (tempting to copy-paste product code to test code brainlessly, especially if it's generated), but practically impossible, if the test is validating against actual behavior.
But I'm fine with keeping this as an improvement opportunity for later.
There was a problem hiding this comment.
I've implemented a version of the test in the base class. The original test remains in the generated versions though as I do not use code generation for this property in the source.
| /// If a color channel value in a premultiplied format is greater than the alpha | ||
| /// channel, the standard source-over blending math results in an additive blend. | ||
| /// </summary> | ||
| Associated, |
There was a problem hiding this comment.
Unless we know image formats where we can report this by Identify this will be a completely unimplemented virtual feature for now. I know it's me who proposed the API, but now I'm concerned exposing as-is, feels a bit misleading and confusing.
How about replacing the summary with the text "Reserved for future use", and moving the docs into a comment?
There was a problem hiding this comment.
I dunno... We'd then have to have a tracking issue etc. What if someone want to implement their own pixel format type and use it to represent already premultiplied pixel data?
There was a problem hiding this comment.
If a pixel had premultiplied alpha would it break any of our processors methods? if it would then either we need to go though and add checks to any that it will break even if we don't do anything else with it or we need to fix them so they function with eather type.
There was a problem hiding this comment.
It's unlikely I think that it would break anything. I couldn't be 100% sure though but since no one has raised an issue suggesting that it does so far I think we're ok.
There was a problem hiding this comment.
It will only make difference when we start utilizing it in ResizeProcessor. There's still some non-trivial work out there: Should we watch for the value in the processor code, before calling PixelOperations.To/FromVector4? Or should we do it right in the converter method overriding request to PixelConversionModifiers.Premultiply?
Other than this I don't really see open questions here according to my understanding of the feature, but I'm generally reluctant for defining public API-s for stuff that is not even prototyped.
There was a problem hiding this comment.
Might be overthinking in this case, IDK.
There was a problem hiding this comment.
I think the best place to do the check is in PixelConversionModifiers.Premultiply. However, there are still per-pixel calls to Premultiply in the convolution API.
src/ImageSharp/PixelFormats/PixelImplementations/PixelOperations/A8.PixelOperations.cs
Outdated
Show resolved
Hide resolved
Add new PixelAlphaRepresentation property and implement for TPixel types
Prerequisites
Description
The first in several PRs targetting #1396
PixelAlphaRepresentationproperty to thePixelTypeInfoclass which describes the pixel alpha behavior for the image pixel format.TPixeltypes.I've automated the test generation using T4 as there's a lot of repetition.
Follow up PRs will wire up image formats and optimize methods based upon the new information.