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

Fix for Windows 2.0 or OS/2 1.x bitmaps only use 3 bytes per color palette entry #792

Merged

Conversation

brianpopow
Copy link
Collaborator

@brianpopow brianpopow commented Dec 16, 2018

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

The following bitmap is not decoded correctly:

pal8os2.zip

The output is:

bmpv2_output

The expected output is:

bmpv2_input

Windows 2.0 or OS/2 1.x bitmaps use 3 bytes per color palette instead of 4, as described here:

http://www.fileformat.info/format/bmp/egff.htm

This PR provides a fix for that.

@codecov
Copy link

codecov bot commented Dec 16, 2018

Codecov Report

Merging #792 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #792      +/-   ##
=========================================
+ Coverage    88.7%   88.7%   +<.01%     
=========================================
  Files        1012    1012              
  Lines       43248   43253       +5     
  Branches     3127    3127              
=========================================
+ Hits        38363   38369       +6     
+ Misses       4181    4180       -1     
  Partials      704     704
Impacted Files Coverage Δ
src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs 89.9% <100%> (+0.23%) ⬆️
src/ImageSharp/Formats/Bmp/BmpFileHeader.cs 76.92% <0%> (+7.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 590609a...10af2ed. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 16, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@4f1e624). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #792   +/-   ##
========================================
  Coverage          ?   88.7%           
========================================
  Files             ?    1012           
  Lines             ?   43253           
  Branches          ?    3127           
========================================
  Hits              ?   38369           
  Misses            ?    4180           
  Partials          ?     704
Impacted Files Coverage Δ
src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs 89.9% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f1e624...2f3c0fc. Read the comment docs.

@brianpopow brianpopow changed the title WIP: Fix for Windows 2.0 or OS/2 1.x bitmaps only use 3 bytes per color palette WIP: Fix for Windows 2.0 or OS/2 1.x bitmaps only use 3 bytes per color palette entry Dec 16, 2018
@brianpopow
Copy link
Collaborator Author

@JimBobSquarePants: maybe you can help me here, i think i am missing something obvious.

I wanted to create a unit test which verifies the Decoded bitmap with reference bitmap, which was added yesterday.

[Theory]
[WithFile(WinBmpv2, PixelTypes.Rgba32)]
public void BmpDecoder_CanDecodeBmpv2<TPixel>(TestImageProvider<TPixel> provider)
	where TPixel : struct, IPixel<TPixel>
{
        using (Image<TPixel> image = provider.GetImage(new BmpDecoder()))
	{
		// TODO: how to compare to the reference?
	}
} 

I think i need to use CompareToReferenceOutput from the TestImageExtension, but i am unsure howto do it correctly.

Can you point me to an example where i can see how i should use it.

@JimBobSquarePants
Copy link
Member

@brianpopow No worries. When comparing a decoder we save the output (defaults to 32bit png) so we can view it manually then we call the CompareToOriginal extension method which compares our decoded result against a reference decoder (System.Drawing for bitmaps).

image.CompareToOriginal(provider);

That reference you added yesterday would be useful only when testing encoding once we can encode that bitmap type. You would normally just add the image to the input folder for testing the decoder.

@brianpopow
Copy link
Collaborator Author

@JimBobSquarePants thank you for the clarification!

The image i have added to the submodul was a bitmap with 40 bytes header which imagesharp can already decode. I wanted to use that as a reference.

Saving the image as png and then comparing to System.Drawing makes much more sense, then the stunt i wanted to do :-) . So the image i have added to the submodul can be safely deleted. Sorry for this misunderstanding.

I have added now two tests. One for decoding a windows BMPv2 and to make sure decoding images with a palette of 4 bytes per entry still works, another one for that case.

@brianpopow
Copy link
Collaborator Author

brianpopow commented Dec 18, 2018

unfortunately that does not work either. System.Drawing can not decode this bitmap.

edit: works on windows, but not on ubuntu.

@JimBobSquarePants JimBobSquarePants merged commit abaf558 into SixLabors:master Dec 20, 2018
@JimBobSquarePants
Copy link
Member

Merged. Thanks so much for your help with bitmaps. I don't understand the various specs well enough just now and you've really made a positive difference to the library. 👍

@brianpopow
Copy link
Collaborator Author

@JimBobSquarePants: thank you! I am happy, that i could help a bit.

@brianpopow brianpopow changed the title WIP: Fix for Windows 2.0 or OS/2 1.x bitmaps only use 3 bytes per color palette entry Fix for Windows 2.0 or OS/2 1.x bitmaps only use 3 bytes per color palette entry Jan 25, 2019
antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019
…letteFix

WIP: Fix for Windows 2.0 or OS/2 1.x bitmaps only use 3 bytes per color palette entry
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