Skip to content

Conversation

@Evangelink
Copy link
Contributor

@Evangelink Evangelink commented Oct 16, 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

Fix #1115

@CLAassistant
Copy link

CLAassistant commented Oct 16, 2020

CLA assistant check
All committers have signed the CLA.

@antonfirsov
Copy link
Member

@Evangelink are you still interested / do you have time to push this to completion?

@Evangelink
Copy link
Contributor Author

Sorry @antonfirsov I got distracted by some other work. I will give it a try tomorrow, I hope to be able to make it work!

@Evangelink
Copy link
Contributor Author

@antonfirsov I have made some change but I have no confidence at all in what I have written, I have tried to put up 2 tests based on others but I also don't feel confident about them (not sure I really got how the test infra is working).

I am happy to try to finish the work but I feel like I need a full baby-sitting so I am afraid that's pointless for you... Let me know if you'd rather close this.

@Evangelink
Copy link
Contributor Author

Also I am not sure what's the process to update your submodule.

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.

@Evangelink you don't need reference images to test this feature. I think it's better to implement some basic "unswizzle" code right in the tests then call ImageComparer.Exact.VerifySimilarity to compare the unswizzled image to the original one.

Avoid relying on the submodule change by implementing the invert swizzle operation
@codecov
Copy link

codecov bot commented Nov 26, 2020

Codecov Report

Merging #1388 (7c5f39d) into master (0e0dc2a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1388   +/-   ##
=======================================
  Coverage   83.68%   83.68%           
=======================================
  Files         732      734    +2     
  Lines       31984    31990    +6     
  Branches     3605     3605           
=======================================
+ Hits        26766    26772    +6     
  Misses       4505     4505           
  Partials      713      713           
Flag Coverage Δ
unittests 83.68% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...cessing/Extensions/Transforms/SwizzleExtensions.cs 100.00% <100.00%> (ø)
...ocessors/Transforms/SwizzleProcessor{TSwizzler}.cs 100.00% <100.00%> (ø)

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 0e0dc2a...7c5f39d. Read the comment docs.

@Evangelink Evangelink marked this pull request as ready for review November 26, 2020 14:16
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.

Looks good with one suggestion.

@onepiecefreak3 I know it's been ages, but any chance you can take a look if this is something that would meet your expectations?

@Evangelink Evangelink changed the title [Draft] Pixel swizzler Pixel remapping/Swizzling Nov 26, 2020
@Evangelink
Copy link
Contributor Author

@antonfirsov @JimBobSquarePants Issues are unrelated to this PR, e.g.:

     Decode_DegenerateMemoryRequest_ShouldTranslateTo_ImageFormatException<Rgba32>(provider: Jpg/progressive/Festzug.jpg[Rgba32]) [PASS]
      Output:
        Cannot decode image. Failed to allocate buffers for possibly degenerate dimensions: 1443x1071.

Unhandled Exception: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.

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.

LGTM

@JimBobSquarePants
Copy link
Member

Should we add the ability to swizzle a sub rectangle of the source or are we happy just doing the whole image?

@Evangelink
Copy link
Contributor Author

@JimBobSquarePants I am not sure how you could apply it only to some sub-rectangle. The new coordinates could be on some space outside of the rectangle (either on an existing location in the image or even outside of the image), what would you do in such case?

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Nov 27, 2020

@Evangelink Let's not bother for now then. I'm happy with this as is. Thanks very much for helping out!! 👍

@JimBobSquarePants JimBobSquarePants added this to the 1.1.0 milestone Nov 27, 2020
@JimBobSquarePants JimBobSquarePants merged commit 718945c into SixLabors:master Nov 27, 2020
@Evangelink Evangelink deleted the pixel-swizzle branch November 27, 2020 14:53
@Evangelink
Copy link
Contributor Author

@JimBobSquarePants You are welcome! @antonfirsov Thank you for the strong support :)

JimBobSquarePants added a commit that referenced this pull request Mar 13, 2021
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.

Pixel remapping/Swizzling

4 participants