CyberBoardPBEM / cbwindows

CyberBoard Play by EMail system for Windows
MIT License
6 stars 4 forks source link

replace DibApi.* code #121

Closed wsu-cb closed 9 months ago

wsu-cb commented 1 year ago

The license status of the DibApi.* code is not clear, so we should probably replace it.

I have discovered that wxWidgets has a Microsoft-specific wxDIB class, but its comments state it only supports 24bpp and 32bpp. CB often uses 16bpp (RGB565), so wxDIB does not work for us. I have not discussed extending wxDIB to support RGB565 with the wxWidgets team, but I expect they would accept submissions supporting it if we want to go that route.

BUT: I haven't looked deeply into non-Win32 graphics systems, but I have the impression that the DIB concept does not have a clear equivalent in other systems. Therefore, I'm not very enthusiastic about going down the wxDIB path.

After a little testing, I think conversions between DDB and DIB don't happen very often, so the replacement code doesn't need to be very performant. My next line of investigation will be to see if wxImage, which is wxWidget's platform independent image format, can be used as a replacement for DIB.

On the other hand, the concern I have is how well wxWidgets supports palettes. If it doesn't, I might decide that, in the short term, the best thing to do will actually be to simply replace the DibApi code with private code. I doubt tracking down and integrating a different graphics library is worth the effort for code that in the long term should probably be removed due to non-portability.

wsu-cb commented 1 year ago

@DLLarson : Can you point me to examples of .gbx that use palettes?

DLLarson commented 1 year ago

Hi Bill,

I can send you a few privately but the best way to see if a CB file only supports palette based colors is to attempt to load the file with the most mature version of CB 2.x (which I've attached to this message.) That archive also has a generic game example that uses palettes.

If it loads into CB2, it uses palettes. CB 3.0 is the version that changed to 16 bit bitmaps for its canonical device independent bitmap (aka, DIB) format.

The huge tranche of games I "sent" you some time ago should have many that never upgraded to CB 3.x.

The historical source code repository (github private) has the commits that changed away from using palettes. The commit that made the final switch from palettes was b578483.

I did some digging in the wxWidgets source code and there is DIB support in the wxBMPHandler used by wxImage. It does support 16 bit pixels in the 5-6-5 format that CB uses. So I think that loading existing bitmap data should be good.

The question is should we use the DIB format for saving bitmaps going forward or change to something that wxWidgets prefers to use? I would lean toward the later if it ultimately makes things easier.

-Dale

cyberboardv200.zip

wsu-cb commented 1 year ago

I can send you a few privately but the best way to see if a CB file only supports palette based colors is to attempt to load the file with the most mature version of CB 2.x (which I've attached to this message.) That archive also has a generic game example that uses palettes.

If it loads into CB2, it uses palettes. CB 3.0 is the version that changed to 16 bit bitmaps for its canonical device independent bitmap (aka, DIB) format.

The huge tranche of games I "sent" you some time ago should have many that never upgraded to CB 3.x.

I expected that there would be some palette .gbxs in the test library, but I didn't how to readily distinguish them. If file version 2.x is the distinguishing feature, then that should make it relatively easy to track some down. Of course, if the attached .zip already has a palette .gbx, then that's even easier.

I did some digging in the wxWidgets source code and there is DIB support in the wxBMPHandler used by wxImage. It does support 16 bit pixels in the 5-6-5 format that CB uses. So I think that loading existing bitmap data should be good.

The question is should we use the DIB format for saving bitmaps going forward or change to something that wxWidgets prefers to use? I would lean toward the later if it ultimately makes things easier.

At this point, I have no specific opinion on the future save format. I will keep in mind that changing it is a valid choice.

The lack of support I found was: I took one of the CB CBitmap objects, read the HANDLE out of it, and initialized a wxBitmap with the HANDLE. Then I tried a variety of wx APIs that save a file as a .bmp, and in each case Windows could not read the resulting .bmp. At that point, I debugged the code some, and found that there was an assumption in the code that the data behind the handle in a wxDIB is a 24bpp. It seems reasonable that wxWidgets could have code to read files in a variety of formats, but that it would always convert them to some canonical format that doesn't match bitmaps we are generating. I can imagine writing some slightly more complicated code that would properly convert a CBitmap to a wxBitmap.

The palette issue feels more complicated to me, so I will probably try to handle the RGB565 issue first.

DLLarson commented 1 year ago

I took one of the CB CBitmap objects, read the HANDLE out of it, and initialized a wxBitmap with the HANDLE. Then I tried a variety of wx APIs that save a file as a .bmp, and in each case Windows could not read the resulting .bmp.

Is it possible I could get that trial code and give it a peek in the debugger?

I'm starting to look back at 250-ish pixel height limitation issue of Windoze owner draw list boxes now that wxWidgets is hooked in for access to virtual list boxes. So it's a good time to kick the proverbial wxWidgets bitmap handling tires.

-Dale

wsu-cb commented 1 year ago

Is it possible I could get that trial code and give it a peek in the debugger?

I have pushed the experiment to https://github.com/wsu-cb/cbwindows.git in branch wxdib-experiment.

wsu-cb commented 1 year ago

I was able to write code to convert at least one form of CBitmap to wxImage and then use wxImage::SaveFile() successfully. You can see the code in https://github.com/wsu-cb/cbwindows.git in branch replace-dibapi.

DLLarson commented 1 year ago

Hi Bill,

I was able to write code to convert at least one form of CBitmap to wxImage and then use wxImage::SaveFile() successfully.

I see this. It's kind of brute force but I can certainly understand why you did it that way. It appears that the wxWidgets BMP handler explicitly removed referencing 16 bit/pixel support in file include/wx/imagbmp.h line 27.

I also found some historical info about 16 bit format 555 vs 565 (this is what CB uses) in general. I'm linking it in here so we don't lose relevant knowledge.

https://www.virtualdub.org/blog2/entry_177.html

General information (include bottom-up vs top-down vertical layout.)

https://learn.microsoft.com/en-us/windows/win32/gdi/device-independent-bitmaps

I think more exploration is needed before we nail down the future bitmap storage format for CB. As a minimum it may make more sense to use PNG files for export rather than BMP's which are pretty bloated. Both formats are lossless.

I'm still digging into the runtime plumbing using your other test branch. By default CB writes out 24bit (rgb) DIB's since it's the most supported format in editors etc. But the CB DIB code also allows storing the bitmap in 16 bit/pixel 565 format. I tested that as well. Both versions of DIB output loads without error into various tools I tried.

We should entertain using 24bit bitmaps going forward (perhaps compressed using the PNG format). But I'm not sure how broadly the impact would be throughout CB's codebase. But we should keep the option in mind. I think it this day and age the 33% increase in uncompressed bitmaps should not be an issue.

We have more exploring to do...

-Dale

wsu-cb commented 1 year ago

I think more exploration is needed before we nail down the future bitmap storage format for CB. As a minimum it may make more sense to use PNG files for export rather than BMP's which are pretty bloated. Both formats are lossless.

Are you referring to the Save Map as .BMP or the internal bitmaps in the .gbx?

If you are referring to the Save Map operation, then I'm pretty sure wxWidgets supports a good variety of file formats. That wxImage::SaveFile() overload is documented to use the format corresponding to the file extension, so we might not need to do anything explicit ourselves except give the Save File dlg all the extensions we think are both useful and supported by wxWidgets.

We should entertain using 24bit bitmaps going forward (perhaps compressed using the PNG format). But I'm not sure how broadly the impact would be throughout CB's codebase. But we should keep the option in mind. I think it this day and age the 33% increase in uncompressed bitmaps should not be an issue.

If you are referring to the .gbx content rather than the Save Board as Bitmap command, then you already have the zlib compression applied to the bitmaps, so I assume we're already getting decent compression.

Also, assuming we are talking about .gbx internals, then my current idea of replacing DibApi.* with wxImage would automatically mean 24bpp graphics. (wxImage is always either 24bpp or 32bpp.)

DLLarson commented 1 year ago

Hi Bill,

I've fixed the operation of two of your "trials". It's a bug/omission in wxWidgets wxDIB code. I've created a patch for our wxWidgets submodule checkout version. You can apply the patch while in the root of wxWidgets with the command git am < 0001-Add-16bit-565-bpp-support-to-wxDIB-Save.patch. Then just rerun your build.

0001-Add-16bit-565-bpp-support-to-wxDIB-Save.patch

I'm looking at fixing the other example whose problem is in the same source code. It assumes only 24 or 32 bpp exist. It's blindly copying 16bbp (2 bytes per pixel) data on top of a target 3 byte/pixel target area. It generates a weird image that viewers can open but it's clearly wrong.

The wxDIB stuff is pretty incomplete but these bugs are just from doing what was required for the original author and no more-IMHO anyway.

-Dale

DLLarson commented 1 year ago

Are you referring to the Save Map as .BMP or the internal bitmaps in the .gbx?

Both actually--but certainly for saving board images, etc. to the file system. Nobody likes messing with BMP files. For internal storage things are not nearly as clear but we should keep an open mind.

-Dale

wsu-cb commented 1 year ago

I've fixed the operation of two of your "trials". It's a bug/omission in wxWidgets wxDIB code. I've created a patch for our wxWidgets submodule checkout version. You can apply the patch while in the root of wxWidgets with the command git am < 0001-Add-16bit-565-bpp-support-to-wxDIB-Save.patch. Then just rerun your build.

0001-Add-16bit-565-bpp-support-to-wxDIB-Save.patch

I'm looking at fixing the other example whose problem is in the same source code. It assumes only 24 or 32 bpp exist. It's blindly copying 16bbp (2 bytes per pixel) data on top of a target 3 byte/pixel target area. It generates a weird image that viewers can open but it's clearly wrong.

The wxDIB stuff is pretty incomplete but these bugs are just from doing what was required for the original author and no more-IMHO anyway.

As I said in https://github.com/CyberBoardPBEM/cbwindows/issues/121#issue-1878940331: if we want to use wxDIB, I think the wxWidgets team would accept bug-fixes/enhancements. However, wxDIB is Microsoft only, so I would prefer not to go down that path.

DLLarson commented 1 year ago

However, wxDIB is Microsoft only, so I would prefer not to go down that path.

I'm not suggesting that. I was just annoyed that the code was busted and I could see a fix. I would rather we did something more universal. I'm just not clear on the ramifications yet. For me the only way to gain confidence with wxWidgets is to dig in and see what issues we hit.

-Dale

wsu-cb commented 9 months ago

We should entertain using 24bit bitmaps going forward (perhaps compressed using the PNG format). But I'm not sure how broadly the impact would be throughout CB's codebase. But we should keep the option in mind. I think it this day and age the 33% increase in uncompressed bitmaps should not be an issue.

I have been experimenting with using 24bbp, and I think I would call the impact significant, but not enormous.

I also experimented with saving my Space Empires.gbx in various formats. It is 16MB at "Most Compression" in the existing RGB565 format. Using 24bpp and PNG format, the .gbx is 22M. Using 24bpp and BMP at "Most Compression", the .gbx is 19M. It took approximately twice as long to save the .gbx in BMP/Most (23sec) as PNG (13sec). However, a large portion of the time appears to be I/O, since the uncompressed BMP (size 139MB) actually took much longer (43sec)! I am still using a laptop with a rotating drive, so that's probably contributing to slow I/O. Also, since I was starting with an existing .gbx, the low bits of all the color channels are zero, which might not be representative of a .gbx containing newly-created tiles.

At the moment, I think I am inclined to store tiles using BMP+zlib rather than PNG since that produces at least competitive results without needing to change the user interface to remove the zlib compression control.

DLLarson commented 9 months ago

Hi Bill,

I also experimented with saving my Space Empires.gbx in various formats. It is 16MB at "Most Compression" in the existing RGB565 format.

That's a real nice test case.

I am still using a laptop with a rotating drive, so that's probably contributing to slow I/O. ... since the uncompressed BMP (size 139MB) actually took much longer (43sec)!

Yikes!

Time for an upgrade. ;) But it's also a good test case for the lower end impact. Historically, I've noticed that many board war gamers don't run the latest computer hardware. Not even close. They would rather spend their hard earned cash on more games.

Also, since I was starting with an existing .gbx, the low bits of all the color channels are zero, which might not be representative of a .gbx containing newly-created tiles.

True. With the lack of source detail the upscale would probably have lots of zero and one runs that compress well.

At the moment, I think I am inclined to store tiles using BMP+zlib rather than PNG since that produces at least competitive results without needing to change the user interface to remove the zlib compression control.

I'm fine with this. It's all working now and RGB565 is far from a dead format.

-Dale