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

Managed ICO decoder #166

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MaceWindu
Copy link

For review.

ICO decoder with access to all images in container as frames. Windows built-in decoder expose single image and not available on other platforms.

Implementation is tested on ~4K ICO files from different sources.

Notes on implementation:

  • encoder is not provided. I personally don't need it but could be implemented later if such request arrives.
  • 16-bit BMP decoding not implemented as I didn't found any ICO file that use it.
  • some libraries implement 2-bit indexed BMP decoding, but this encoding is not valid according to BMP spec and I didn't see any ICO images that use it
  • PNG image decoding implemented using existing PNG decoder as single frame. Doesn't look like an issue to me as APNG shouldn't be used here

General notes on found issues:

  • library currently rejects empty ICO containers as they are smaller than 16 bytes (smallest possible in 6 bytes). Not a big deal as chances to find such image is low (I have one :-) )
  • it would be nice to extend ImageFileInfo.FrameInfo with additional information like source BPP. Consider following scenario for ICO:

User need to extract image version with best quality. For that currently we can only inspect frame dimensions to pick biggest one. This doesn't work if container provides several image versions with different color depth.

Side question: could we expect TIFF and BMP support for Linux platform soon? Personally we need only decoder, so if this part is ready - it would be nice to release it sooner.

@saucecontrol
Copy link
Owner

This looks good from a functional standpoint, but it's not something I'd accept for inclusion in the main library in its current state. The implementation is not really in keeping with the design philosophy of this project (for example, it should be resilient to truncated streams, and even though ICO images are tiny, using a reference-type enumerable to iterate over pixel and transparency data is silly wasteful). It also doesn't conform to the code style of the rest of the project (this is largely my fault for not providing a more complete .editorconfig -- I'll work on that).

That said, I'm very pleased you were able to put this together, and it looks at first glance like you were able to do so without using any internal APIs. It would be worth looking at opportunities to improve the main library's surface to make it easier to build plugins like this, as I can see a couple of places internal functionality would have been useful. e.g. instead of creating an entire new pipeline for embedded PNG frames, it would be nice to be able to use CodecManager.GetDecoderForStream directly.

How about making this a separate plugin library, in the ManagedCodecs folder, for now? I think it's a useful example, and ICO has some interesting properties (such as separated transparency data) that will help us round out the public interfaces in the main library and plugin infrastructure.

Longer term, I'd think an ICO codec could delegate to either BMP or PNG for the individual frame decoding so that it acts only as a container demuxer, or it could be integrated with the BMP codec directly.

As for when BMP will be supported xplat, I've started on a managed codec (decode is always harder than encode), but I need to make some changes to the way format conversion is currently implemented. I've got some of that work done already as well, but it's been difficult to dedicate time to this project with most of my funding drying up in the last couple of years.

@MaceWindu
Copy link
Author

Yeah, I didn't expected it to be accepted as-is and submited it to collect feedback on what should be fixed.

  1. I didn't used internal APIs as I implemented it first as external decoder for use in my project and only then moved it to PR as it will be benefitial to other users too
  2. I can move it to ManagedCodecs, but I think it will be better if I keep it as one of my projects for now and release it as plugin to nuget (I can do it next week) to make it available to everyone. I don't think ManagedCodecs currently have binary distibution

@saucecontrol
Copy link
Owner

I don't think ManagedCodecs currently have binary distribution

It's not shipped as a package today, but there's no reason it couldn't be.

I'm planning to get some work done on the managed BMP codec in the new year, and I think this will fit in better once that's done. Will check back with you at that time.

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

Successfully merging this pull request may close these issues.

2 participants