Determine if 'non-standard' bit counts are allowed

[Brett208]

From the perspective of a generic Image Header, what bit counts do we want to consider valid? Currently, only bit counts of 1, 4, 8, 16, 24, and 32 are considered valid. Do we want to eliminate this check and consider any bit count valid (does not necessarily mean we can process it for read/write, but that it could be used in forming a valid Image Header).

If we want to open up the concept of bit count to any value, then I would propose moving the check IsValidBitCount/VerifyValidBitCount from ImageHeader into BmpWriter because in this case it would be a limitation of our Writer and not a limitation on the possible well formed BMP file bit counts.

Additionally, we would want to expand the scope of IsIndexedBitCount to return true for any value <= 8. Currently it only returns true for a Bit Count of 1, 4, and 8.

If we want to keep the discrete values, it would appear 2 and 64 are also acceptable values and should be added to the lists.

From https://en.wikipedia.org/wiki/BMP_file_format

Pixel array (bitmap data)

The pixel array is a block of 32-bit DWORDs, that describes the image pixel by pixel. Usually pixels are stored "upside-down" with respect to normal image raster scan order, starting in the lower left corner, going from left to right, and then row by row from the bottom to the top of the image.[4] Unless BITMAPCOREHEADER is used, uncompressed Windows bitmaps also can be stored from the top to bottom, when the Image Height value is negative.

In the original OS/2 DIB, the only four legal values of color depth were 1, 4, 8, and 24 bits per pixel (bpp).[4] Contemporary DIB Headers allow pixel formats with 1, 2, 4, 8, 16, 24 and 32 bits per pixel (bpp).[16] GDI+ also permits 64 bits per pixel.[17]

[DanRStevens]

My issue is not with the IsValidBitCount check, but rather with the IsIndexedBitCount check. According to the same wikipedia article, the palette is required for bit depths <= 8. I think that should be the check for a palettized/indexed image. Whether or not it's valid is a separate question.

[Brett208]

If I check to see if a bit count is indexed and it passes, then I would also assume it is a valid bit count. We would be forcing the user to make multiple checks here when I don't see any value in it. I don't see a use case where someone would want to know that a bit count was less than 8 and was also invalid.

[DanRStevens]

Why would you need to check both?

I feel like this is a case of premature optimization.

[Brett208]

If I am looking for a valid indexed bit count, I would need to first check that it was valid, then check if it was indexed. For example, if the bit count was 7, it would pass being indexed but fail being a valid bit count. Currently, I only make one check because 7 is not considered an indexed bit count by the code.

-Brett

[DanRStevens]

I can't think of any case where you would need to "look for a valid indexed bit count". This doesn't seem like a use case we should need to support.

The only place that tests both, is the unit test code, and it does so by calling both methods individually.

My thought is, if the format is invalid, how would we even get to a point where we are testing if it was indexed or not.

Further, by writing the IsIndexedBitCount method as an array search, it becomes less efficient than if it was a simple range comparison.

If you want to have an array of valid indexed bit counts, I'm fine with that. I just don't think the indexed check methods should use it.

[Brett208]

Outside of unit testing, the function "look for a valid indexed bit count" is used in IndexedBmpWriter::Write.

If we do not test against the indexed bit count array, then we would first call a method to test the value is less than 8 and then call a function to make sure it was part of the larger bit count array. It seems more efficient to just test against the array first. Also, for an invalid value, the end of the array will be reached quicker testing against a smaller subset (just the indexed bit counts as opposed to against all valid bit count).

-Brett

[DanRStevens]

I was thinking about this, and I think part of the difference in our thinking stems from the design. This was written for an IndexedBmpWriter class. It doesn't handle any valid Bitmap, only the indexed ones. I can understand having such a restricted class design. There are many cases where it's helpful to have a generic base class interface, which various subclasses derive from.

I probably wouldn't have made that distinction in this particular case myself though.

I probably would have just had a generic Bitmap class, which could read or write any format. In such a class, there would be no need to check if a bit depth was indexed or not to determine if it was valid for that class. Instead, bit depth would simply control the requirement to have a palette when reading, and the existence of a palette would control if one was written or not.

If a single class could handle all formats, there would be no need to check if it was an indexed format or not to determine if it was valid for that class. Hence the check doesn't make much sense in the general context. If you want to get specific like this, where you're effectively testing two properties in a single method, might I suggest also making the name more specific. Perhaps have an IsIndexedImage method for a generic interface, and an IsValidIndexedBitDepth for a more specific class.

---

A side consideration here is why I would have chosen a single class to represent all formats versus a family of classes to handle the various bitmap types. I'm not sure I really have a clear answer to that. I suppose part of that is that palettes are not restricted to indexed bitmaps.

[Brett208]

I'm trying to keep things general concerning bitmaps where applicable, but balance not creating extra work that we won't be using since Outpost 2 doesn't seem to need anything above 8 bits. I am looking at creating a BitmapFile class soon for use when reading a bitmap from file.

Hmm, since a palette may exist optionally on a bitmap greater than 8 bits, the function name could be misleading. When I say IsIndexedBitCount, I think I mean the image requires a palette. Maybe it makes more sense to say DoesBitCountRequirePalette and VerifyBitCountRequiresPalette.

@DanRStevens, what do you think about changing the function name to something similar to this and leaving in ImageHeader with the array (1, 2, 4, 8)?

If you don't want to see these functions in ImageHeader in this format, then we could move them somewhere else. For now they could be moved onto IndexedBmpWriter. I may want to reference them during Reading if we don't support reading non-indexed BMPs so they may need to live somewhere else in the end, not sure what this location would be.

-Brett

[Brett208]

Edit:

In sake of moving forward, I am okay with just using:

static bool ImageHeader::DoesBitCountRequirePalette (uint16_t BitCount) {
    return BitCount <= 8;
}

static void ImageHeader::VerifyBitCountRequiresPalette (uint16_t BitCount) {
    if (!DoesBitCountRequirePalette(bitCount)) {
        throw std::runtime_error("A bit count of " + std::to_string(bitCount) + " does not require a palette");
    }
}

Hmm, the error message sort of sounds strange. Maybe the Verify function needs to exist in IndexedBmpWriter and the boolean in ImageHeader.

Thoughts. Also, closing the issue earlier was accidental.

-Brett

[DanRStevens]

Outpost 2 does have code for processing 16-bit images. I think a lot of the internal graphics code is actually 16-bit. It kind of has to be to support the color range of the 9 palettes used in the art file. I remember seeing code to load 16-bit tileset images, and I'm pretty sure that was tested out and it worked.

---

There is a subtle issue here. Although a 16-bit image may contain an option palette, it is not an indexed image. The pixel values are raw RGB codes. With an 8-bit image, the pixel values are indexes into the palette.

An indexed image requires a palette. A true-color image does not require a palette, but may optionally include one.

---

For an end user perspective, I think IsIndexedImage sounds nicer than IsIndexedBitCount. That fact that it depends solely on bit count is an implementation detail.

If these methods are part of ImageHeader, they can be made non-static, and not require arguments. That would make them much nicer to use. Having static methods is more useful when the method is needed for object construction, such as in a factory method. It is reasonable to provide both a static and a non-static version of such methods.

---

I think you're right, in that the "Verify" method is more natural in the IndexedBmpWriter class than ImageHeader. Though determining if an image is indexed or not very much does fit in ImageHeader.

---

Bit of a side note, but as it relates to placement of methods: I kind of feel like there shouldn't be a split between the reader and writer classes here. The main use case is an in memory bitmap object. How it gets saved or loaded is more of a side concern. I think use static functions for loading, and member functions for saving.