Add ICO and CUR file decoder. #2579
Conversation
|
|
|
|
|
Even though there is no magic head for ICOs, should we still provide And should we name the classes with |
How do you identify without leading bytes? |
|
Maybe we can combine an Or we just provide a decoder, but this decoder will not be used without specification. |
I think we can do without providing like this using SixLabors.ImageSharp.Formats.Icon;
async using var fs = File.OpenRead(@"xxx.ico");
var ico = IconDecoder.Instance.Decode(new (), fs); |
frg2089
force-pushed
the
feat/icon
branch
2 times, most recently
from
6f211c1 to
a70c1ca
Compare
@JimBobSquarePants Yes. I have reverted all changes to tiff files. |
There was a problem hiding this comment.
Thanks for this, your efforts are really appreciated! There's a lot to discuss.
Some notes...
I don't think it's a good idea to merge the two formats. We're creating unnecessary ambiguity. If you look at format metadata, we avoid exposing unconstrained configuration values.
It's imperative that the pixel data is drawn on the frame and not resized. Position 0,0, seems sane as an origin. Any other behavior such as resizing or introducing new image types or editing existing signifies a major breaking change for the library.
We need an encoder. I can't accept code against main without one as we need to ensure that we have covered encoding requirements in the format design.
I'll be honest and say this won't get merged before V4. There's a lot to do to ensure that the decoder is safe (I haven't checked bounds handling and EOF detection) and the design practical.
I refactored the code. |
|
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. |
| if (bmpMetadata != null) | ||
| { | ||
| result.Metadata.SetFormatMetadata(BmpFormat.Instance, bmpMetadata); | ||
| } | ||
|
|
||
| if (pngMetadata != null) | ||
| { | ||
| result.Metadata.SetFormatMetadata(PngFormat.Instance, pngMetadata); | ||
| } |
There was a problem hiding this comment.
I think the PNG data may not be the only one. ICO file may have 2 and more PNG data.
So, we should store the PngMetadata in the FrameMetadata instead of the ImageMetadata.
There was a problem hiding this comment.
They’re just for easy conversation really. See below re bits per pixel. You may want to capture palette data also
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) |
|
Apologies for the radio silence over the last few weeks. I've been very busy fixing up the animation behavior for v3.1 |
|
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 |
|
|
||
| protected IconDir FileHeader { get => this.fileHeader; private set => this.fileHeader = value; } | ||
|
|
||
| protected IconDirEntry[] Entries { get; private set; } = []; |
There was a problem hiding this comment.
SA1010
Should I avoid this syntax?
There was a problem hiding this comment.
You should be fine now that I've updated the StyleCop version.
Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
but AlphaMask have some problems
…and the case where the stream start position is not 0.
Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
|
@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. |
Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
Done! |
Prerequisites
Description
Some Questions
Warning
We don't support bmp palettes yet.