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

Implement discontiguous buffer handling #1109

Merged
merged 66 commits into from
Feb 21, 2020
Merged

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Feb 8, 2020

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

Marked as WIP because processors and codecs need adaption, but the core logic is ready to review!

  • MemorySource<T> has been replaced with MemoryGroup<T>, which is a list of uniformly sized buffers (IMemoryOwner<T> and Memory<T>)
  • Several helpers intend to provide span-like functionalities in MemoryGroupExtensions eg. .CopyTo()
  • MemoryGroupView<T> is an indirection to expose these memory buffers in a safe way, see docs and comments for more info
  • The methods exposed over AdvancedMemoryExtensions have been changed to reflect the new memory model, .GetPixelSpan<T>() has been made obsolete
  • buffer2d.GetSpan<T>() ---> buffer2d.GetSingleSpan<T>(), temporarily marked [Obsolete] to guide refactor

TODO

We should no longer process Buffer2D as a single span (except of a few special cases like resize KernelMap). Guidance: go through all [Obsolote] warnings.

  • Add tests for each codec and processor in a similar manner like it has been added to Jpeg tests. (Artifically limit buffer capacity of allocator to emulate the discontiguous case.)
  • Fix remaining codecs
  • Fix remaining processors

Resolves #898, Fixes #805

@antonfirsov antonfirsov requested a review from a team February 8, 2020 02:58
@codecov
Copy link

codecov bot commented Feb 21, 2020

Codecov Report

Merging #1109 into master will increase coverage by 0.1%.
The diff coverage is 79.31%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1109     +/-   ##
=========================================
+ Coverage   82.04%   82.15%   +0.1%     
=========================================
  Files         680      686      +6     
  Lines       28562    29048    +486     
  Branches     3238     3308     +70     
=========================================
+ Hits        23434    23864    +430     
- Misses       4453     4485     +32     
- Partials      675      699     +24
Flag Coverage Δ
#unittests 82.15% <79.31%> (+0.1%) ⬆️
Impacted Files Coverage Δ
...ry/DiscontiguousBuffers/MemoryGroup{T}.Consumed.cs 90.9% <ø> (ø)
src/ImageSharp/Advanced/AdvancedImageExtensions.cs 70% <0%> (-10%) ⬇️
src/ImageSharp/Image.Decode.cs 96.77% <100%> (+0.1%) ⬆️
src/ImageSharp/Memory/BufferArea{T}.cs 96.77% <100%> (+0.47%) ⬆️
src/ImageSharp/Memory/Buffer2D{T}.cs 97.72% <100%> (-2.28%) ⬇️
...emory/DiscontiguousBuffers/MemoryGroup{T}.Owned.cs 95% <100%> (ø)
src/ImageSharp/Image.LoadPixelData.cs 100% <100%> (ø) ⬆️
src/ImageSharp/ImageFrame.LoadPixelData.cs 85.71% <100%> (+5.71%) ⬆️
src/ImageSharp/ImageFrameCollection{TPixel}.cs 89.28% <100%> (+0.19%) ⬆️
src/ImageSharp/Formats/Jpeg/Components/Block8x8.cs 52.56% <100%> (ø) ⬆️
... and 50 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 a3f169f...ad2632f. Read the comment docs.

@antonfirsov antonfirsov changed the title [WIP] Implement discontiguous buffer handling for images Implement discontiguous buffer handling Feb 21, 2020
@antonfirsov
Copy link
Member Author

Looks like I managed to fix everything, removed the WIP tag, ready for final review.

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.

Fantastic work!

All looks good to me, just spotted a possible optimization opportunity in RowOctet<T>

I like that we can implement any enhancements to individual codecs in the future.

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