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 La16 and La32 IPixel formats. #1062

Merged
merged 20 commits into from
Dec 15, 2019
Merged

Conversation

JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented Dec 12, 2019

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

This PR acts as a precursor to another planned PR building upon the work done adding a non-generic API to the library #907

The planned work will change the behavior of the non-generic Image.Load() behavior to select the correct pixel format to decode images to rather than defaulting to Rgba32.

This PR lays the groundwork to allow this by adding missing pixel formats.

The above is no longer the plan, rather we make it easier for users to make informed pixel format choices by inspecting metadata.

  • Introduces La32 and La16, pixel formats representing luminance and alpha at 8-bit and 16-bit per component.
  • Renames Gray8, Gray16, and Alpha8 to L8, L16, and A8 for consistancy
  • Adds PixelOperations overloads and tests for the new types and the missing one for Bgra5551.
  • Fixes rounding issues converting from RGB to L.
  • Updates PngDecoder to use new pixel formats when decoding.
  • Updates the JpegDecoder to decode to Rgb24 or L8 instead of Rgba32.
  • Adds format specific extensions to ImageMetadata and ImageFrameMetadata to make it easier to return format specific metadata.

I appreciate there's a lot to read but that's the nature of the IPixel API.

@codecov
Copy link

codecov bot commented Dec 12, 2019

Codecov Report

Merging #1062 into master will increase coverage by 0.09%.
The diff coverage is 93.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1062      +/-   ##
==========================================
+ Coverage   89.94%   90.03%   +0.09%     
==========================================
  Files        1131     1145      +14     
  Lines       50633    51762    +1129     
  Branches     3715     3783      +68     
==========================================
+ Hits        45542    46605    +1063     
- Misses       4335     4402      +67     
+ Partials      756      755       -1
Impacted Files Coverage Δ
...ntries/IccProfileSequenceIdentifierTagDataEntry.cs 50% <ø> (ø)
.../Profiles/Exif/ExifTagDescriptionAttributeTests.cs 100% <ø> (ø)
...files/ICC/TagDataEntries/IccLutAToBTagDataEntry.cs 57.14% <ø> (ø)
...s/ICC/TagDataEntries/IccUInt16ArrayTagDataEntry.cs 50% <ø> (ø)
src/ImageSharp/Metadata/ImageMetadata.cs 100% <ø> (ø)
...ICC/DataReader/IccDataReader.NonPrimitivesTests.cs 100% <ø> (ø)
...ImageSharp/Metadata/Profiles/ICC/Various/IccLut.cs 87.5% <ø> (ø)
...ta/Profiles/ICC/DataReader/IccDataReader.Curves.cs 86.07% <ø> (ø)
...data/Profiles/ICC/Curves/IccFormulaCurveElement.cs 92.3% <ø> (ø)
...adata/Profiles/ICC/DataReader/IccDataReader.Lut.cs 96.07% <ø> (ø)
... and 235 more

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 9275555...8395e9c. Read the comment docs.

@tocsoft
Copy link
Member

tocsoft commented Dec 12, 2019

Things to consider with returning anything but Rgba32 is composability, If Image.Load sometimes returns La16 then you can't now draw over the top a 2nd image that might have loaded as Rgba32 and save it in color as the draw image call will downscale the Rgba32 image to La16 during drawing.

@JimBobSquarePants
Copy link
Member Author

Things to consider with returning anything but Rgba32 is composability, If Image.Load sometimes returns La16 then you can't now draw over the top a 2nd image that might have loaded as Rgba32 and save it in color as the draw image call will downscale the Rgba32 image to La16 during drawing.

Yeah, I had a think about that and I would expect that behavior. The option is always there to use the generic version if need be.

@antonfirsov
Copy link
Member

antonfirsov commented Dec 12, 2019

A few remarks (did not have the chance to go through the code changes though):

  • We should compare both Load and LoadResizeSave benchmark results before/after. (For both YCbCr Jpegs and whatever is considered to be the most common in PNG. 4 components <=> 3 components mass packing/unpacking can be tricky, can't recall what's happening there. I want to make sure that we are aware of the perf impacts.
  • Regarding @tocsoft 's concern on drawing: We should probably document this behavior in the remarks section of the xml docs for Image.Load(), and leave a note to self to also do so in the articles when we get the chance.
  • Does the PR effect the default PNG/Jpeg pixel format when we resave an image?

@JimBobSquarePants
Copy link
Member Author

JimBobSquarePants commented Dec 13, 2019

@antonfirsov @tocsoft

I'm beginning to think that returning different pixelformat types is a bad idea. It adds complexity to the internal workings of our decoders that I can't enforce.

I'm going to revert the JpegDecoder changes in this PR but all other changes still stand.

Update...

Yep, definitely sticking to Rgba32. I've updated every non-generic loading methods to specify the return type via intellisense and added simplified methods to return format specific metadata. The idea now is that users will know it's Rgba32 but can use Identify to retrive enough relevent information to choose the pixel format they wish to decode to.

@antonfirsov When you have a moment I want to talk Core 3+ and a plan to perform the Jpeg IDCT + Colorspace transforms via intrinsics in a manner that matches other high performance decoders. I saw a 33% slowdown when decoding to Rgb24

@antonfirsov
Copy link
Member

antonfirsov commented Dec 14, 2019

I'm beginning to think that returning different pixelformat types is a bad idea.

It's worth to do so in long term, because it is an enabler for memory & CPU optimizations together with the shuffling intrinsics which are not yet utilized. The speedup and the reduced memory footprint for the most common (3 component, alpha-free) jpeg thumbnail making use case should be significant enough to justify the extra complexity. Basically, all major imaging libraries work like this, for a good reason.

Therefore, tying our own hands by communicating in docs that an untyped Image is actually an Image<Rgba32> is not wise. If users start to rely on it, it would disable the optimization opportunity.

There are many ways to handle the drawing composability issues - most straightforward is docs.

@JimBobSquarePants
Copy link
Member Author

Therefore, tying our own hands by communicating in docs that an untyped Image is actually an Image is not wise. If users start to rely on it, it would disable the optimization opportunity.

@antonfirsov Interestingly enough that was actually the case in some areas of the documentation already, it was quite inconsistent.

I'll revert that documentation change for now and will wait to change the type until after we have those optimizations. (Hopefully something we can begin very soon once we have merged #1061)

@JimBobSquarePants
Copy link
Member Author

Just pushed some changes to fix the casing of any Metadata related files. It was doing my head in.

@antonfirsov
Copy link
Member

Interestingly enough that was actually the case in some areas of the documentation already, it was quite inconsistent.

That is most likely an accidental leftover for #907.

When you have a moment I want to talk Core 3+ and a plan to perform the Jpeg IDCT + Colorspace transforms via intrinsics in a manner that matches other high performance decoders.

Okkay, let's jump into it :) I'm prepering an analysis right now. Sorry in advance, but it will be long as hell. I'm trying best to explain things as deeply and clearly as possible.

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.

Just a few remarks and suggestions, but in principle, everything is OK.

src/ImageSharp/Formats/Png/PngScanlineProcessor.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/IImageDecoder.cs Show resolved Hide resolved
src/ImageSharp/Formats/Png/PngScanlineProcessor.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Png/PngScanlineProcessor.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Png/PngScanlineProcessor.cs Outdated Show resolved Hide resolved
src/ImageSharp/Formats/Png/PngScanlineProcessor.cs Outdated Show resolved Hide resolved
@JimBobSquarePants JimBobSquarePants merged commit a42e30f into master Dec 15, 2019
@JimBobSquarePants JimBobSquarePants deleted the js/smart-non-generic-api branch December 15, 2019 10:49
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