SixLabors / ImageSharp

:camera: A modern, cross-platform, 2D Graphics library for .NET
https://sixlabors.com/products/imagesharp/
Other
7.34k stars 847 forks source link

Add ICO and CUR file decoder. #2579

Closed frg2089 closed 2 months ago

frg2089 commented 10 months ago

Prerequisites

Description

Some Questions

[!WARNING] We don't support bmp palettes yet.

CLAassistant commented 10 months ago

CLA assistant check
All committers have signed the CLA.

CLAassistant commented 10 months ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Poker-sang commented 10 months ago

Even though there is no magic head for ICOs, should we still provide IcoImageFormatDetector as well? In order to support Image.Load<TPixel>().

And should we name the classes with Ico instead of Icon?

JimBobSquarePants commented 10 months ago

Even though there is no magic head for ICOs, should we still provide IcoImageFormatDetector as well? In order to support Image.Load<TPixel>().

And should we name the classes with Ico instead of Icon?

How do you identify without leading bytes?

Poker-sang commented 10 months ago

Maybe we can combine an ICODir and an entry, and think of it as the header of an ICO file.

Or we just provide a decoder, but this decoder will not be used without specification.

frg2089 commented 10 months ago

Maybe we can combine an ICODir and an entry, and think of it as the header of an ICO file.

Or we just provide a decoder, but this decoder will not be used without specification.

I think we can do without providing IconImageFormatDetector. Because nobody cares if unknown streams are ICOs. They are usually not transmitted without filename. So we should just provide a decoder and use it directly.

like this

using SixLabors.ImageSharp.Formats.Icon;

async using var fs = File.OpenRead(@"xxx.ico");
var ico = IconDecoder.Instance.Decode(new (), fs);
Poker-sang commented 10 months ago

We actually have an open discussion supporting tiffs with multisize frames. It would be good to split any changes to the tiff decoder out so we can draw the pixel data on the frame.

@JimBobSquarePants Yes. I have reverted all changes to tiff files.

frg2089 commented 10 months ago

You've gone the opposite direction to how I hoped I'm afraid here. Please let me know if you need further guidance and I'll try my best to explain.

I refactored the code. Now it doesn't have the IcoPngFrameMetadata, IcoBmpFrameMetadata and the corresponding Cur variants. We can use GetFormatMetadata(XXXFormat.Instance) (which isn't there yet) to get specific metadata. If I'm not doing this the right way, I'm probably not understanding what you're really thinking.

JimBobSquarePants commented 10 months ago

I'm going to write some code to your fork....

OK. So, I've refactored the base decoder to be more efficient and allow us to gather the correct metadata from the png/bmp frames.

I've added a bunch of TODOs in the code also. Please let me know if anything is not clear.

JimBobSquarePants commented 10 months ago

I have some questions about how to store PngMetadata and BmpMetadata.

We should store BitsPerPixel property for encoder. The ICO file may have more than one PNG data.

I’d store that in the Ico/Cur frame metadata. I’d make it an enum though (it’s a fixed range based on available bmp/png fixes)

JimBobSquarePants commented 9 months ago

Apologies for the radio silence over the last few weeks. I've been very busy fixing up the animation behavior for v3.1

frg2089 commented 9 months ago

Do we have some new ways to save frames of different sizes yet?

JimBobSquarePants commented 9 months ago

Do we have some new ways to save frames of different sizes yet?

You encode the size stored in the metadata. Pixel buffers can be constrained to regions

JimBobSquarePants commented 8 months ago

@frg2089 Thanks for all your effort so far. It's been tricky for me to review as I've just moved house and have no real internet just now. I'll be able to do a proper review next week though.

JimBobSquarePants commented 7 months ago

I'm going to have a look at how we manage the sizes in the encoding options. I want to come up with the best possible design so I will look for inspiration in other libraries.

JimBobSquarePants commented 2 months ago

@Poker-sang @frg2089 Thanks for your patience.

I think this is in a really good place now. I can do any additional changes (potentially palette handling) once merged.

The only thing I'd like to add is some more tests to ensure we can encode and decode the files. I scoured the internet and managed to find some test files here.

https://github.com/SystemRage/Iconolatry/tree/master/test_decode

I cannot add the image to your fork due to LFS permissions issues with GitHub but would be very grateful if you were able to do so.

frg2089 commented 2 months ago

The only thing I'd like to add is some more tests to ensure we can encode and decode the files. I scoured the internet and managed to find some test files here.

Done!