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

Pixel remapping/Swizzling #1115

Closed
onepiecefreak3 opened this issue Feb 16, 2020 · 13 comments · Fixed by #1388
Closed

Pixel remapping/Swizzling #1115

onepiecefreak3 opened this issue Feb 16, 2020 · 13 comments · Fixed by #1388

Comments

@onepiecefreak3
Copy link

This feature is implemented in many graphics hardwares at least of most gaming consoles. Adding this feature would enable ImageSharp to natively allow for these specifications of graphics hardware to be supported out of the box.

As per OpenCV naming and implementation this feature is most likely known as "Remapping" or "Point remapping". So as the naming goes, it should incorporate either one.

Point remappings are, as the name suggests, a process to rearrange the pixels from a source to a destination point, where each source point is modified as per following method:
dst(x,y) = src(fx(x,y), fy(x,y))

As for the implementation following this function, it should allow executing an action for both the x- and y-coordinate, and return its modified value respectively. Or an even simpler approach as seen in the Kanvas library of the Kuriimu2 project, where it just takes in a Point and returns a new one with the modified coordinates.

Sources:
OpenCV documentation on remapping: https://docs.opencv.org/2.4/modules/imgproc/doc/geometric_transformations.html?highlight=remap
Kuriimu2's Kanvas Swizzle interface:https://github.com/FanTranslatorsInternational/Kuriimu2/blob/dev/src/Kontract/Kanvas/IImageSwizzle.cs

@antonfirsov
Copy link
Member

antonfirsov commented Feb 20, 2020

As far as I remember our discussion, remapping should actually alter the image. This could be implemented easily in a single ImageProcessor that consumes an IImageSwizzle-like interface as a parameter.

@use191

This comment was marked as spam.

@Sergio0694
Copy link
Member

This also seems a pretty good candidate to use the value delegate trick (for those unfamiliar, I mean using a struct type implementing an interface and making the method generic with the T : struct, ISomeInterface constraint, instead of taking an actual delegate like a Func<...>), since if I'm reading that linked OpenCV correctly, you'd basically have to invoke two custom functions to map the x, y coordinates of every single pixel in the original image 🤔
If there were two non-inlineable callvirt for each pixel, that'd be quite the performance hit.

@use191

This comment was marked as spam.

@Mahiar7

This comment was marked as spam.

@louis126

This comment was marked as spam.

@onepiecefreak3
Copy link
Author

@Sergio0694 Of course having 2 delegates calling for each coordinate x and y could hit performance in the situation you described. That's why I also included a sample from Kanvas, where a Point struct is just taken in, modified, and then returned. That might at least reduce the delegate count to one. But however you want to implement it, is of course the decision of this team here. I just gave my input on already existing examples for this kind of operation.

@Evangelink
Copy link
Contributor

Not sure to fully understand what's supposed to be done for this ticket. My understanding is that we want to:

  1. Create an IImageSwizzle-like interface

  2. Create a new generic Processor class

  3. Add a ctor with this Func signature

  4. Do the processing in the OnFrameApply

Am I right?

@antonfirsov
Copy link
Member

antonfirsov commented Oct 6, 2020

@Evangelink yes, the steps are mostly correct, but a few key things missing, for example the extension method. I would do the following:

Essential:

  1. Define IImageSwizzle interface
  2. Define a non-generic SwizzleProcessor : IImageProcessor class that will act as a factory for the generic processor. (See other processor implementations.)
  3. Define a generic SwizzleProcessor<TPixel> class and implement OnFrameApply
  4. Define an extension method over the non-generic IImageProcessingContext that takes an IImageSwizzle. This can be used with Mutate and Clone. (See other extension methods delegating to processors.)

API sugar:

  1. Define an internal DefaultSwizzle : IImageSwizzle type that takes a Func<Point, Point> in constructor
  2. Define a public extension method over IImageProcessingContext that takes Func<Point, Point> and delagates to DefaultSwizzle. Note: this stuff will be relatively slow.

Regression tests:

  1. Define regression tests as in other processors, but instead of validating with reference outputs, validate the result by manually "Unswizzling" the image in the test code, and comparing the result to the original.

Performance sugar from #1115 (comment):

  1. Alter the the non-generic SwizzleProcessor to SwizzleProcessor<TSwizzle> where TSwizzle : struct, IImageSwizzle
  2. Similarly, change the generic SwizzleProcessor<TPixel> to SwizzleProcessor<TImageSwizzle, TPixel>. All calls to the TImageSwizzle member will be inlined then, which improves speed.
  3. Alter the extension method that takes IImageSwizzle to:
public static Swizzle<TImageSwizzle>(this IImageProcessingContext context, TImageSwizzle swizzle)
    where TImageSwizzle : struct, IImageSwizzle { ... }

OFC you may decide to add 8-10 to your implementation immediately.

@Sergio0694
Copy link
Member

FYI @Evangelink in case you were not familiar with these "value delegates" (constrained struct + interface), to add to @antonfirsov's point with respect to 5. and 8., the idea is that once you add the generic constraint, you'd no longer use a Func<Point, Point> at all, but instead inject that functionality through an interface method on that IImageSwizzle interface, so that the JIT compiler can see the actual type in the generic instantiation of that processor type and devirtualize/inline it 😊

You do not want to define a struct with that interface that just wraps some delegate, otherwise that'd defeat the whole point of this optimization (as those calls would remain virtual, just with even more steps due to the extra generic argument).

As in (just an example to illustrate the general idea, ignore the actual type names):

// Before
class DefaultSwizzle
{
    public DefaultSwizzle(Func<Point, Point> swizzler) { }
}

// After
interface ISwizzler
{
    Point Swizzle(Point point);
}

class DefaultSwizzle<TSwizzler>
    where TSwizzler : struct, ISwizzler
{
    public DefaultSwizzle(TSwizzler swizzler) { }
}

And following this, you'd have to make the following changes when actually using these types:

// Before
var defaultSwizzle = new DefaultSwizzle(xy => /* Some logic... */ xy);

// After
struct MySwizzler : ISwizzler
{
    Point Swizzle(Point point) => /* Some logic... */ point;
}

var defaultSwizzle = new DefaultSwizzle<MySwizzler>(default);

If you need more examples of this pattern, you can browse the various row processors we use in ImageSharp here and that are leveraged by all the existing image processors, they do the same trick with a "value delegate" implemented as a struct + interface.

Hope this helps! 😄

@Evangelink
Copy link
Contributor

Sorry for the delay, I have started to work on this ticket but I'd like some more info. You can see the little I have done so far in the draft PR that I have linked.

The first question is functional, I am not sure to understand if the swizzle function we are talking about here is meant to change the value of the pixel at coordinates (x, y) or to provide new coordinates (x', y').

The second part of my questions is technical, and depend upon the previous answer:

  • Do we want to work on a Point object? It seems that most of the rest is relying on row operations for performance so that seems weird to have this transformation being by Point.

  • Looking at the other transformations I can see that most of the processing is done in BeforeImageApply rather than OnFrameApply, shall I follow the same logic?

  • Shall I do in place modification or create a separate image? In case of the second option, shall I inherit from AffineTransformProcessor instead?

@antonfirsov
Copy link
Member

antonfirsov commented Oct 16, 2020

The first question is functional, I am not sure to understand if the swizzle function we are talking about here is meant to change the value of the pixel at coordinates (x, y) or to provide new coordinates (x', y').

Provide the new coordinates.

If you need better functional understanding I would suggest to look at the OpenCV stuff in the opening post, and maybe try some examples. Are your questions more related to the ImageSharp API suggestions?


Do we want to work on a Point object?

See https://github.com/SixLabors/ImageSharp/pull/1388/files#r506247769


Shall I do in place modification or create a separate image? In case of the second option, shall I inherit from AffineTransformProcessor instead?

You need to work on a new buffer. I would derive from CloningImageProcessor TransformProcessor<TPixel>, check existing samples (AffineTransformProcessor is one of them). Swizzling is not an affine transformation, but a nonlinear one.

EDIT:
You also need to implement IResamplingTransformImageProcessor<TPixel>, so your code will be probably almost identical to AffineTransformProcessor<T>, but I still wouln't derive for clarity and to respect Liskov Substitution Principle.


Looking at the other transformations I can see that most of the processing is done in BeforeImageApply rather than OnFrameApply, shall I follow the same logic?

I think it's a better practice to work with OnFramewApply:

protected override void OnFrameApply(ImageFrame<TPixel> source, ImageFrame<TPixel> destination)
{
this.source = source;
this.destination = destination;
this.resampler.ApplyTransform(this);
}

Processors which do everything in BeforeImageApply for any reason, can be refactored to move the core logic to OnFramewApply IMO.

@antonfirsov
Copy link
Member

You also need to implement IResamplingTransformImageProcessor<TPixel>

@Evangelink I'm sorry, this is total nonsense I told after drinking insufficient amount of coffee. There's no need for resampling for swizzle.

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

Successfully merging a pull request may close this issue.

7 participants