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

Consume memory primitives from SixLabors.Core #665

Merged
merged 6 commits into from
Aug 5, 2018
Merged

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Aug 4, 2018

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

  • Updated SixLabors.Core package reference
  • Removed duplicates of common memory primitives which are now defined in SixLabors.Core
  • Move ImageSharp-specific memory primitives and utils to the SixLabors.ImaSharp.Memory namespace
  • Update + cleanup tests and benchmarks

Fixes #619.

@codecov
Copy link

codecov bot commented Aug 4, 2018

Codecov Report

Merging #665 into master will decrease coverage by 0.06%.
The diff coverage is 59.09%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #665      +/-   ##
=========================================
- Coverage   89.67%   89.6%   -0.07%     
=========================================
  Files         890     878      -12     
  Lines       37633   37229     -404     
  Branches     2472    2450      -22     
=========================================
- Hits        33747   33359     -388     
+ Misses       3182    3177       -5     
+ Partials      704     693      -11
Impacted Files Coverage Δ
src/ImageSharp/Advanced/AdvancedImageExtensions.cs 76.47% <ø> (ø) ⬆️
...geSharp.Tests/TestUtilities/TestImageExtensions.cs 82.37% <ø> (ø) ⬆️
.../Processors/Transforms/AffineTransformProcessor.cs 100% <ø> (ø) ⬆️
src/ImageSharp/Formats/Gif/GifEncoderCore.cs 96.75% <ø> (ø) ⬆️
...rs/Normalization/HistogramEqualizationProcessor.cs 100% <ø> (ø) ⬆️
src/ImageSharp/Memory/MemoryOwnerExtensions.cs 85.71% <ø> (ø)
...geSharp.Drawing/Processing/PatternBrush{TPixel}.cs 83.33% <ø> (ø) ⬆️
src/ImageSharp/Formats/Gif/LzwDecoder.cs 93.02% <ø> (ø) ⬆️
...rocessors/Convolution/Convolution2PassProcessor.cs 100% <ø> (ø) ⬆️
src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs 82.58% <ø> (ø) ⬆️
... and 74 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 d874ee7...bf8f377. Read the comment docs.

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.

Just some cleanup required to remove method duplication. Other that that this all looks great!

@@ -120,7 +123,7 @@ public BufferArea<T> GetSubArea(int x, int y, int width, int height)
public BufferArea<T> GetSubArea(Rectangle rectangle)
{
ImageSharp.DebugGuard.MustBeLessThanOrEqualTo(rectangle.Width, this.Rectangle.Width, nameof(rectangle));
DebugGuard.MustBeLessThanOrEqualTo(rectangle.Height, this.Rectangle.Height, nameof(rectangle));
SixLabors.DebugGuard.MustBeLessThanOrEqualTo(rectangle.Height, this.Rectangle.Height, nameof(rectangle));

int x = this.Rectangle.X + rectangle.X;
Copy link
Member

Choose a reason for hiding this comment

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

We're mixing two DebugGuard methods now. We should ditch the ImageSharp one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Having common internals is just too dangerous, we should probably drop [InternalsVisibleTo(...)] in SL.Core to avoid these issues.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, though I was wondering whether we should just make them public and avoid duplication? They're useful and well tested.

Copy link
Member Author

Choose a reason for hiding this comment

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

One can accidentally include and use such internals with R# (been there). I would rather share them through submodules in the future. For now it's just safer to hide them IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Hide them then. We can investigate private packages like they use for ASP Core in the future

@antonfirsov antonfirsov merged commit afa2a1b into master Aug 5, 2018
@antonfirsov antonfirsov deleted the af/memory-bridge branch October 6, 2018 15:08
antonfirsov added a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019
Consume memory primitives from SixLabors.Core
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants