search

swharden / Spectrogram

.NET library for creating spectrograms (visual representations of frequency spectrum over time)
https://nuget.org/packages/Spectrogram
MIT License
320 stars 58 forks source link

Replace `System.Drawing.Common` with `SkiaSharp` to improve cross platform support #61

Closed vadd98 closed 4 weeks ago

vadd98 commented 1 month ago

Hi!

I replaced the dependency to System.Drawing.Common with SkiaSharp, allowing the use of this library on all OS platforms targeting the newests .NET versions

swharden commented 1 month ago

Hi @vadd98, thanks for this PR!

It's pretty big and appears to be breaking too 😅

I'm very interested in merging this in eventually, but it may take me a while before I can review this one! Just wanted to give you a heads up is may take longer than typical for me to get to this one.

swharden commented 1 month ago

Note that if this had been implemented in a non-breaking way (or if it's updated to be such) then I'll merge it in much faster and issue a release quickly!

Putting all SkiaSharp related new code in an Experimental folder and namespace would make it go even faster, as I can merge and release it right away and I can take as long as I need to dive into the rest of the code base and refactor it to use your logic.

👆 I figured I'd give you some alternative options to help this get merged faster in case you'd like to see this on NuGet sooner than later 🚀

GeorgeS2019 commented 4 weeks ago

@vadd98 ColorMap not implemented yet?

vadd98 commented 4 weeks ago

@GeorgeS2019 it should be already implemented, probably I just skipped it somewhere. I'm going to take a look

@swharden I can move all the SkiaSharp related code to an Experimental folder, that would not be a problem. Just give me any hint on how you'd prefer to manage the Experimental namespace: for example, do you want there a new ImageMaker class with just the SkiaSharp implementation?

vadd98 commented 4 weeks ago

I fixed the ColorMap implementation. It seems that SkiaSharp does not support anymore the Index8 format (https://github.com/mono/SkiaSharp/issues/694) and only Gray8 is available. I needed to apply a ColorFilter and then convert the image to the Rgba8888 format in order to get a working ColorMap. If you have any suggestion on how it could be possible to improve this flow it will be very welcomed

swharden commented 4 weeks ago

After thinking about it some more, I recognize that a goal is to completely remove System.Drawing.Common so there's no choice - this must be very breaking 😅

It may be painful for people who are already using this package, but (1) they don't have to upgrade if they don't want to and (2) I'm prepared to go for it and answer questions as they pop up!

I'll make some refinements to this PR, merge it, publish a new package this morning, and we'll see how it goes 😅😅😅

In the interest of speed I may adopt a dependency on ScottPlot which already solves lots of these problems (System.Drawing.Bitmap / SKBitmap conversion, Color/SKColor, colormaps, etc.)

Thanks again for your work on this! I'll commit directly to this PR and merge shortly 😎

swharden commented 4 weeks ago

goal is to completely remove System.Drawing.Common

Adding context, https://learn.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/6.0/system-drawing-common-windows-only

swharden commented 4 weeks ago

Looking deeper at this, SpectrogramGenerator is the core functionality of this package and it would greatly benefit from being rewritten. I want to take a little time to do it right.

I'll probably focus on this today and see how far I get. Hopefully I can refactor this to bring it up to modern standards quick enough to let me release a new package today, and we can continue to refine this with time.

I'm going to try to use ScottPlot color and colormap logic to minimize the amount of primitive testing I have to do along the way. We can always break away from ScottPlot in the future once the core functionality of this library is working well.

GeorgeS2019 commented 4 weeks ago

https://github.com/mono/SkiaSharp/issues/908

Perhaps use an older version of SkiaSharp

swharden commented 4 weeks ago

I bumped the package version to 2.0.0-alpha so it will be on NuGet shortly, but you'll have to allow adding preview package versions to install it into your apps for now.

Hopefully this gets the ball rolling, but leaving it a preview package protects us from people getting surprised about breaking changes, and gives us a little more time to refine the API.

Thanks again for all your help!

vadd98 commented 4 weeks ago

Thanks for your work! I'll test the new alpha version in my projects