#338 TDib fails for top-down DIBs

[Vidar Hasfjord]

The default row-order for DIBs is bottom-up, indicated by a postive biHeight in the BITMAPINFOHEADER structure. A negative biHeight indicates top-down row order. See MSDN.

Unfortunately, TDib does not handle a negative biHeight. In particular, if given a filename of a top-down bitmap, the TDib constructor will fail and throw an exception. See [discussion:3b9d1452].

[Vidar Hasfjord]

@jogybl wrote:

May need to find a way to create some top-down bitmaps to test.

If there isn't a ready available application to create a top-down DIB, I would think a simple testing strategy would be to:

1. After adding support for top-down DIBs, create a top-down TDib.
2. Save it into a test BMP file using existing TDib functionality.
3. Load the test BMP file into a bitmap editor, e.g. Windows Paint, to test that it loads and displays correctly.
4. Load the BMP file into a TDib to test that it loads correctly.
5. Test that the loaded TDib displays correctly.
   
   

void WriteTopDownTestDib(ostream& os, int width, int height)
{
  PRECONDITION(width > 0 && height > 1);
  auto dib = TDib{width, -height, 0}; // Create top-down DIB.

  // Make test pattern.
  //
  const auto p = dib.GetBits();
  const auto stride = dib.Pitch();
  memset(p, 0, height * stride); // all black
  memset(p, 0xFF, stride); // first row white

  dib.Write(os, /* writeFileHeader = */ true);
}

Presumably, you can also create a test file by patching a normal bottom-up BMP file in a hex editor. Changing the sign of BITMAPINFOHEADER::biHeight should be enough, it seems.

[Ognyan Chernokozhev]

Good idea, I will try this.

[Ognyan Chernokozhev]

I wrote very simple code to recursively traverse my drives (using std::filesystem::recursive_directory_iterator) and look for .bmp file with negative height and found a whole bunch in AppData\LocalLow\Adobe\Acrobat\DC\ConnectorIcons\

I will use them to fix and test the code.

P.S. Helpful hint:
std::filesystem::directory_options::skip_permission_denied is mandatory option when using recursive_directory_iterator on C: drive

[Vidar Hasfjord]

@jogybl wrote:

[my code] found a whole bunch in AppData\LocalLow\Adobe\Acrobat\DC\ConnectorIcons\

Clever!

I had one of these thumbnail bitmaps in the same location. On resaving in Paint, it turned into ordinary bottom-up format. Looking at the files in a hex editor it seems the sign of the height is the only change in the BITMAPINFOHEADERs.

[Vidar Hasfjord]

@jogybl committed:

• BUG: TDib fails for top-down DIBs [r8893].
• Merged [r8893] from trunk [to branches/7] [r8894],
• Merged [r8893] from trunk [to branches/6.44] [r8896].

Fix looks good, although constructor TDib(int width, int height, ...) still doesn't support top-down DIBs (negative height). The image size is incorrectly calculated if a negative height is given. Either use std::abs here as well, or harden the PRECONDITION to height > 0 and document the limitation. (By the way, the PRECONDITION should say width > 0.)

[Vidar Hasfjord]

@jogybl committed:

• BUG: More fixes for TDib handling of top-down bitmaps [r8898].
• Merged [r8898] from trunk [to branches/7] [r8899].
• Merged [r8898] from trunk [to branches/6.44] [r8900].
  
  

That should do it, I think.

PS. I spotted a little nit: std::abs is now used in "gdiobjec.h", without an include-directive for "<cstdlib>" or "<cmath>". However, the latter is indirectly included via "geometry.h" (where incidentally it is not used). Ideally, move the "<cmath>" include-directive to "gdiobject.h", and make sure other headers depending on it also includes it, if any. Make sure to test the build without PCH.

[Ognyan Chernokozhev]

There are usages of std::lround at the end of geometry.h

[Vidar Hasfjord]

@jogybl wrote:

There are usages of std::lround at the end of geometry.h

Ah, indeed there are. My bad; I trusted Visual Studio IntelliSense, which greys the include directive and shows message #include <cmath> is not used in this file. I guess this false positive is produced because std::lround is only used within templates, and the IntelliSense parser doesn't do proper two-phase lookup within templates (old MSVC issue).

[Vidar Hasfjord]

I did a simple test of the new top-down support in TDib(int width, int height, uint32 nColors, ...), and while it seems to work fine, I ran into an issue with the "nColors" parameter: What are we supposed to pass to create 16/24/32-bit bitmaps?

Note that NColors returns 0 for 16/24/32-bit bitmaps. But the TDib constructor does not allow 0 to be passed. The workaround is ugly:

bool TBmpViewWindow::LoadBitmapFile(LPCTSTR name)
{
  TDib* newDib = {};
  try {
    auto dib = TDib{name};
    const auto bitCount = dib.GetInfoHeader()->biBitCount;
    const auto colorCount = NColors(bitCount);

    // Test TDib(int width, int height, uint32 nColors, ...)
    // by making a copy of the loaded DIB. The bitmap will be
    // flipped vertically if bottom-up.
    //
    newDib = new TDib{dib.Width(), -dib.Height(),
      colorCount > 0 ? static_cast<uint32>(colorCount) :
      bitCount == 16 ? 65536 :
      bitCount == 24 ? 16777216 :
      16777216 + 1}; // Force 32-bit.

    const auto sizeImage = dib.SizeImage(); CHECK(sizeImage == newDib->SizeImage());
    memcpy(newDib->GetBits(), dib.GetBits(), sizeImage);

    const auto sizeColors = colorCount * sizeof(RGBQUAD);
    memcpy(newDib->GetColors(), dib.GetColors(), sizeColors);
  }
  catch (TXGdi) {
    MessageBox(_T("Cannot open bitmap file"), GetApplication()->GetName(), MB_OK | MB_ICONERROR);
    return false;
  }
  SetupFromDib(newDib);
  return true;
}

Edit: I've now created a separate ticket for a related issue, that may cause this constructor to crash [bugs:#652]. The fix is easy, and should be applicable to the release branches.