-
Notifications
You must be signed in to change notification settings - Fork 0
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
Allow custom pitch when writing bitmaps #256
base: master
Are you sure you want to change the base?
Changes from all commits
2264fa4
ffa5afb
98b7cab
c9d5edd
82e89cf
dc5b645
bb81732
3942cea
d2546c1
a3a853c
967ce2d
413b1b6
70f4280
1869af6
96e2d11
df91957
b3f7803
c3323f6
893ff39
04521ba
c48a75d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,8 +42,9 @@ struct ImageHeader | |
void VerifyValidBitCount() const; | ||
static void VerifyValidBitCount(uint16_t bitCount); | ||
|
||
std::size_t CalculatePitch() const; | ||
static std::size_t CalculatePitch(uint16_t bitCount, int32_t width); | ||
// Default pitch granularity is 4 bytes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had intentionally left this as interval. Perhaps it is a regional language different but interval seems a better and easier to understand choice than granularity to me. Being a comment, it probably isn't worth arguing over now that it has been changed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A disagreement in terminology might signal a disagreement about what is intended here. I would define an interval as having a start and end point. Granularity is about the minimum spacing of values. Here the pitch is rounded to some multiple of a certain value. That is defining the granularity of the value. As in, values are a multiple of 4 bytes. Is does not define the length of an interval as 4 bytes. For example, the spacing between scanlines could easily be 12 bytes, even though the granularity of that value is 4 bytes. More deeply, I'm not sure I would really consider pitch to be an interval. It's more of a spacing between scanlines. If pitch were an interval, I'd be uncertain how to define the start and end points. Looking back at I vaguely recall some requirement that pitch is a minimum of 4 bytes, though can be a larger multiple of it. I might also consider the phrasing: |
||
std::size_t CalculateDefaultPitch() const; | ||
static std::size_t CalculateDefaultPitch(uint16_t bitCount, int32_t width); | ||
|
||
// Does not include padding | ||
std::size_t CalcPixelByteWidth() const; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,8 +62,9 @@ void BitmapFile::ReadPalette(Stream::BidirectionalSeekableReader& seekableReader | |
|
||
void BitmapFile::ReadPixels(Stream::BidirectionalSeekableReader& seekableReader, BitmapFile& bitmapFile) | ||
{ | ||
std::size_t pixelContainerSize = bitmapFile.bmpHeader.size - bitmapFile.bmpHeader.pixelOffset; | ||
BitmapFile::VerifyPixelSizeMatchesImageDimensionsWithPitch(bitmapFile.imageHeader.bitCount, bitmapFile.imageHeader.width, bitmapFile.imageHeader.height, pixelContainerSize); | ||
const std::size_t pixelContainerSize = bitmapFile.bmpHeader.size - bitmapFile.bmpHeader.pixelOffset; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this might be more of a maximum upper bound, rather than an exact size. There may be other sections stored in the bitmap file, after the pixel data, before the end of the file. |
||
const std::size_t pitch = FindPitch(bitmapFile.imageHeader.width, bitmapFile.imageHeader.height, pixelContainerSize); | ||
BitmapFile::VerifyPixelSizeMatchesImageDimensionsWithPitch(bitmapFile.imageHeader.bitCount, pitch, bitmapFile.imageHeader.height, pixelContainerSize); | ||
|
||
bitmapFile.pixels.clear(); | ||
bitmapFile.pixels.resize(pixelContainerSize); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hack of mine is causing the unit tests to fail. They happen to be fairly robust for this small section of the code compared to others.
The next step is probably to figure out a proper bit twiddle or calculation that determines pitch without assuming it is a 4 byte interval. @DanRStevens, feel free to chime in if you have something up your sleeve.
-Brett
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pitch is either assumed, or given. It's not generally calculated (other than in an assumption, such as rounding the scanline byte width up to a multiple of 4 bytes). The problem with trying to calculate it, is the data source could always have chosen a larger pitch than is necessary.
I find it a little non-intuitive to use
pixelCount
here. When I think pixel could, I generally assume it's simplywidth * height
, which is not particularly useful for calculatingpitch
if you already have the width. Plus,pitch
is a byte measurement, not a pixel measurement. ThebitDepth
needs to be accounted for.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are wanting to work on this branch feel free. I'm not going to pick it up again for a long time.
-Brett
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I quite have the context to pick this one up right now.