Skip to content

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

Sister PR of #1028. Fix more points of #967.
API-s being exposed:

  • Buffer2D<T> and Buffer2DExtensions: minimum API surface needed by Drawing
  • ParallelHelper: and ParallelExecutionSettings:
    • Everything but IterateRowsWithTempBuffer
    • Both types moved to namespace SixLabors.ImageSharp.Advanced.ParallelUtils

Additional:
Fixed error cases for Configuration.MaxDegreeOfParallelism and ParallelExecutionSettings.MaxDegreeOfParallelism.

@antonfirsov antonfirsov changed the title Af/expose buffer internals Expose Buffer2D and ParallelHelper internals Oct 24, 2019
@codecov
Copy link

codecov bot commented Oct 24, 2019

Codecov Report

Merging #1035 into master will decrease coverage by 0.1%.
The diff coverage is 98.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1035      +/-   ##
==========================================
- Coverage   89.87%   89.76%   -0.11%     
==========================================
  Files        1103     1104       +1     
  Lines       48896    48922      +26     
  Branches     3442     3444       +2     
==========================================
- Hits        43944    43915      -29     
- Misses       4249     4253       +4     
- Partials      703      754      +51
Impacted Files Coverage Δ
...g/Processors/Overlays/VignetteProcessor{TPixel}.cs 100% <ø> (ø) ⬆️
...on/GlobalHistogramEqualizationProcessor{TPixel}.cs 96.22% <ø> (ø) ⬆️
...s/Convolution/Convolution2PassProcessor{TPixel}.cs 100% <ø> (ø) ⬆️
...s/Binarization/BinaryThresholdProcessor{TPixel}.cs 100% <ø> (ø) ⬆️
...ing/Processors/Transforms/FlipProcessor{TPixel}.cs 100% <ø> (ø) ⬆️
...essors/Convolution/ConvolutionProcessor{TPixel}.cs 100% <ø> (ø) ⬆️
...ssors/Overlays/BackgroundColorProcessor{TPixel}.cs 100% <ø> (ø) ⬆️
...Transforms/ProjectiveTransformProcessor{TPixel}.cs 96.05% <ø> (ø) ⬆️
...Processors/Effects/OilPaintingProcessor{TPixel}.cs 97.7% <ø> (ø) ⬆️
...sors/Convolution/Convolution2DProcessor{TPixel}.cs 100% <ø> (ø) ⬆️
... and 55 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 826205d...fe09e10. 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.

All looks good to me!

{
// Shall be compatible with ParallelOptions.MaxDegreeOfParallelism:
// https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.paralleloptions.maxdegreeofparallelism
if (maxDegreeOfParallelism == 0 || maxDegreeOfParallelism < -1)
Copy link
Member

Choose a reason for hiding this comment

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

👍

/// instantiating a temporary buffer for each <paramref name="body"/> invocation.
/// </summary>
public static void IterateRowsWithTempBuffer<T>(
internal static void IterateRowsWithTempBuffer<T>(
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming this is internal because Drawing does not use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I'm not fully happy with this API yet, so I'd keep it internal for now.

/// </remarks>
/// <typeparam name="T">The value type.</typeparam>
internal sealed class Buffer2D<T> : IDisposable
// TODO: Consider moving this type to the SixLabors.Memory namespace (SixLabors.Core).
Copy link
Member

Choose a reason for hiding this comment

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

That's probably a useful consideration.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not 100% sure yet, depends on future investigations.

/// <summary>
/// Gets the height.
/// </summary>
/// </summary>Bu
Copy link
Member

Choose a reason for hiding this comment

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

Surprised this didn't break compilation!

public void FillDestinationPixels(RowInterval rowInterval, Buffer2D<TPixel> destination)
{
Span<Vector4> tempColSpan = this.tempColumnBuffer.GetSpan();
Span<Vector4> transposedFirstPassBufferSpan = this.transposedFirstPassBuffer.GetSpan();
Copy link
Member

Choose a reason for hiding this comment

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

Good optimization spot.

@JimBobSquarePants JimBobSquarePants merged commit f64e141 into master Oct 24, 2019
@JimBobSquarePants JimBobSquarePants deleted the af/expose-buffer-internals branch October 24, 2019 11:42
@antonfirsov
Copy link
Member Author

@JimBobSquarePants wow that was quick! Does it mean that you are happy with my namespace choice SixLabors.ImageSharp.Advanced.ParallelUtils? I wasn't 100% sure about that.

@JimBobSquarePants
Copy link
Member

Yeah, happy with that, certainly advanced and Utils is a common suffix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants