Skip to content

Conversation

@ynse01
Copy link
Contributor

@ynse01 ynse01 commented Nov 25, 2021

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 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

This implements the Portable Bitmap support idea I posted earlier.

This adds support for encoding and decoding of Portable Bitmap images. These are the following image types:

  • Pbm - Black and White images
  • Pgm - Grayscale images
  • Ppm - Color images

All of these in both Plain and Binary excoding.
These image formats fall under the PNM family of images. See The PNM format
The somewhat related PAM image format, is not implemented here.

The class structure of the code itself is largly based on the existing Tga encoder / decoder.

I could find any other reference to black-and-white pixels. So, I mapped these pixels in a L8 pixel. White is bit value 0 in Pbm, but maps to L8 value 255. Black is bit valaue 1, but maps to L8 value 0. This choice aligns these pixels better with the existing use of L8.

Test code:
I'm open for suggestion on how to improve the test coverage. Some of my struggle was with the RoundTripTest. I couldn't get the L8 pixel images to load properly. Image.Load always returns RGB pixels, which break the pixel comparison.
In PgmDecoderTest, there is an issue with differences in behavior of Magick. It treats black-and-white pixels differently. And in the same test, the RGB input images don't load in Magick, where they DO load in IrfanView, XnConvert and my code.

@codecov
Copy link

codecov bot commented Nov 25, 2021

Codecov Report

Merging #1851 (3730a02) into master (ee83e99) will decrease coverage by 0%.
The diff coverage is 83%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #1851    +/-   ##
=======================================
- Coverage      87%     87%    -1%     
=======================================
  Files         944     959    +15     
  Lines       49753   50392   +639     
  Branches     6165    6258    +93     
=======================================
+ Hits        43575   44113   +538     
- Misses       5157    5246    +89     
- Partials     1021    1033    +12     
Flag Coverage Δ
unittests 87% <83%> (-1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...c/ImageSharp/Formats/Pbm/PbmImageFormatDetector.cs 60% <60%> (ø)
src/ImageSharp/Formats/Pbm/PlainEncoder.cs 77% <77%> (ø)
src/ImageSharp/Formats/Pbm/PlainDecoder.cs 78% <78%> (ø)
src/ImageSharp/Formats/Pbm/BinaryDecoder.cs 79% <79%> (ø)
src/ImageSharp/Formats/Pbm/PbmDecoder.cs 80% <80%> (ø)
src/ImageSharp/Formats/Pbm/BinaryEncoder.cs 82% <82%> (ø)
src/ImageSharp/Formats/Pbm/PbmDecoderCore.cs 90% <90%> (ø)
src/ImageSharp/Formats/Pbm/PbmMetadata.cs 90% <90%> (ø)
src/ImageSharp/Formats/ImageExtensions.Save.cs 94% <94%> (ø)
src/ImageSharp/Formats/Pbm/PbmEncoderCore.cs 98% <98%> (ø)
... and 8 more

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 ee83e99...3730a02. Read the comment docs.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for taking the time for this! Implementation generally looks good, but it would be nice to see validation and debug output in the decoder tests. That would help us reasoning about a format we aren't familiar with.

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Nov 29, 2021

@ynse01 Looks like you're way ahead of me dealing with the individual files but could you do me a favor and run the following command from the repo root to updated the shared infrastructure code and then build your solution to ensure the .gitattributes file is copied across containing new instructions for handling the image type.

git submodule foreach git pull origin master

EDIT. You'll need to remove the test files you added and and them to GitLFS following the submodule update.

I can see 'ppm', 'pbm', 'pgm' files loaded in binary format.

https://docs.github.com/en/repositories/working-with-files/managing-large-files/configuring-git-large-file-storage

@ynse01
Copy link
Contributor Author

ynse01 commented Nov 29, 2021

The test images should now be in Git LFS.

I can't find why the test coverage dropped. Can it be a false positive, as it reports a delta of only -1 % ? Let me know if I need to take action on the coverage.

@JimBobSquarePants
Copy link
Member

I can't find why the test coverage dropped. Can it be a false positive, as it reports a delta of only -1 % ? Let me know if I need to take action on the coverage.

@ynse01 I'm firmly convince the code coverage is hocus pocus. 9/10 times it seems to report nonsensical results. I only keep it around to be indicative and because Enterprise types love to see those kinds of metrics.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more points and this good to merge!

@JimBobSquarePants can you take a look at the shared-infrastructure module update? Is that just a syncup with latest upstream?

This was referenced Jul 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants