Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 new PixelAlphaRepresentation property and implement for TPixel types #1420
Add new PixelAlphaRepresentation property and implement for TPixel types #1420
Changes from all commits
686aef7
181d434
842cd37
f30e22f
e18ef9f
ed949f6
dae53a3
589bd68
a0a6aea
f23fd09
8a065ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 callingPixelOperations.To/FromVector4
? Or should we do it right in the converter method overriding request toPixelConversionModifiers.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be overthinking in this case, IDK.
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 think the best place to do the check is in
PixelConversionModifiers.Premultiply
. However, there are still per-pixel calls toPremultiply
in the convolution API.