Skip to content

BMP: Support encoding L2 and L4 images#3011

Open
RunDevelopment wants to merge 1 commit into
image-rs:mainfrom
RunDevelopment:bmp-l4
Open

BMP: Support encoding L2 and L4 images#3011
RunDevelopment wants to merge 1 commit into
image-rs:mainfrom
RunDevelopment:bmp-l4

Conversation

@RunDevelopment

Copy link
Copy Markdown
Member

I suppose this is a continuation of both #2953 and #2625.

Changes:

  • Add support for writing L2 and L4 bitmaps
  • Fix palette padding for L1
  • Truncate too-large palettes for L8

I added support for writing L2 and L4 BMP images in this PR. This was quite easy after #2953 added L1, since I just had to generalize the logic it added. Now L1, L2, L4, and L8 all use the same functions for writing. The La8 path uses a simplified version of its previous path. All in all, this makes the code for writing easy to understand.

Note: BMP does support 2 bpp paletted images (=they're not disallowed), but not many decoders can read it. In my testing, Chrome, Gimp and image could read it, while Firefox, Windows Photo Viewer and Paint.net could not, and Photoshop read it incorrectly. I also verified that the pal2 images from the BMP test suite by Jason Summers have the same issues, so the problem really is with those programs and not with the files produced by this encoder.

I also found some issues with the existing code that I fixed along the way:

  1. The docs for encode_with_palette said that a given palette is ignored if the color type is incompatible, but the function actually returns an error. I documented the error.
  2. When L1 was given a palette with a length != 2, it would write the length of the palette in the header but then write a palette with exactly 2 entries. This corrupted the image data. I fixed this by making sure we write exactly as many palette entries as specified in the header.
  3. When L8 was given a palette with >256 entries, it would write all entries. This is a problem for some decoders (like us) which only allow 256 entries max. (Our decoder handles this case incorrectly btw.) I fixed this by truncating the palette to at most 2^n entries for Ln.

Given this comment from #2625, this PR might not be accepted. In that case, just tell me and I'll spin off the fixes into a separate PR.

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.

1 participant