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 span overloads for Image.Load() and Image.DetectFormat() #618

Merged
merged 7 commits into from
Jun 18, 2018

Conversation

antonfirsov
Copy link
Member

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 implements API-s proposed in #565 by @jherby2k with the help of System.IO.UnmanagedMemoryStream which is a new dependency for targets > netstandard 1.1.

Related tests have been refactored heavily (isolation, cleanup, code share).

Copy link
Member

@tocsoft tocsoft left a comment

Choose a reason for hiding this comment

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

looking good, only the one item I can see and that's the dependence remaining for netstandard 2.0

@@ -44,6 +44,9 @@
<PackageReference Include="System.Memory" Version="4.5.0" />
<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="4.5.0" />
</ItemGroup>
<ItemGroup Condition=" '$(TargetFramework)' != 'netstandard1.1' ">
Copy link
Member

Choose a reason for hiding this comment

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

you can skip this for our net standard 2.0 > targets too so you probably want to change this to '$(TargetFramework)' == 'netstandard1.3' instead

@tocsoft tocsoft added this to the 1.0.0-beta5 milestone Jun 17, 2018
@codecov
Copy link

codecov bot commented Jun 17, 2018

Codecov Report

Merging #618 into master will decrease coverage by 0.03%.
The diff coverage is 96.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #618      +/-   ##
==========================================
- Coverage   88.58%   88.55%   -0.04%     
==========================================
  Files         878      881       +3     
  Lines       36984    36994      +10     
  Branches     2627     2627              
==========================================
- Hits        32762    32759       -3     
- Misses       3440     3452      +12     
- Partials      782      783       +1
Impacted Files Coverage Δ
...harp.Tests/Image/ImageTests.Load_FileSystemPath.cs 100% <100%> (ø)
...mageSharp.Tests/Image/ImageTests.Load_FromBytes.cs 100% <100%> (ø)
...mageSharp.Tests/Formats/ImageFormatManagerTests.cs 100% <100%> (ø) ⬆️
...ageSharp.Tests/Image/ImageTests.Load_FromStream.cs 100% <100%> (ø)
...ImageSharp.Tests/Image/ImageTests.LoadPixelData.cs 100% <100%> (ø)
tests/ImageSharp.Tests/TestFormat.cs 69.51% <66.66%> (-7.87%) ⬇️
src/ImageSharp/Image.FromBytes.cs 83.33% <79.16%> (ø) ⬆️
.../ImageSharp.Tests/Image/ImageTests.DetectFormat.cs 97.14% <97.14%> (ø)
...eSharp.Tests/Image/ImageTests.ImageLoadTestBase.cs 97.43% <97.43%> (ø)
tests/ImageSharp.Tests/TestFileSystem.cs 36.36% <0%> (-40.91%) ⬇️
... and 10 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 097c150...afc13ad. Read the comment docs.

@antonfirsov
Copy link
Member Author

@tocsoft good catch, fixed!

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.

Looks good. Nice trick with UnmanagedMemoryStream!

@antonfirsov antonfirsov merged commit 2e62c60 into master Jun 18, 2018
{
fixed (byte* ptr = &data.GetPinnableReference())
{
using (var stream = new UnmanagedMemoryStream(ptr, data.Length))

Choose a reason for hiding this comment

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

It's unfortunate to allocate a heap object to wrap the span...

Copy link
Member

Choose a reason for hiding this comment

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

Good spot, I've opened #671 to deal with it.

What do you think elsewhere? Looking good?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense in the DetectFormat() use case, thanks!

When doing a full image decoding (Image.Load(...)), that allocation is a drop in the bucket however. Using UnmanagedMemoryStream seemed to be the best way for "converting" a span into a Stream.

@JimBobSquarePants JimBobSquarePants deleted the af/memory-bridge2 branch September 3, 2019 11:12
antonfirsov added a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019
Add span overloads for Image.Load() and Image.DetectFormat()
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.

4 participants