Gamma Error in Picture Scaling

[Hooman]

I wonder if this might be related to the OP2Editor minimap color bug:
Gamma Error in Picture Scaling

The effect sure looks similar, and the algorithm used is the simple linear scaling one this post says is wrong. Though that leaves the question as to why the minimap colors are only wrong with standard Windows bitmaps, and not with the custom OP2 format images. Was a different scaling algorithm somehow used in that case? I assumed it was the OP2Editor backend code running for both, but perhaps the Windows API or similar was used for the image scaling through another code path.

[leeor_net]

There's a technique that I use in my current version of the map editor -- not sure if it's correct. I take the RGB values of every pixel in the tile and average them to yield the final color then save that to a color table. Not sure if this is what is considered a linear interpolation but it yields the results I generally expect.



This is what I get when using said technique. As a note, I'm using 32-bit PNG's from the original converted BMP's.

[Hooman]

That's the same technique used by the OP2Editor backend. It's also the algorithm that article is suggesting is wrong. Though I do admit it's always seemed to produce good looking results when I've tried it. Though there could be reasons for that. In particular, the linked article uses a rather oddly constructed artificial image to prove his point.

Btw, the PNGs might have a bit of color correction that might alter them slightly from the original BMPs. Though I'd be surprised if the change was enough to notice, or to alter how the scaled down images look.

Perhaps there's just an as yet undiscovered bug in OP2Editor, or possibly the front end which is causing the graphical glitch.

[lordpalandus]

If it is in every major image manipulation program then, the conclusions I draw from this revelation are:

1. There could be a design limitation, in the computer hardware itself, to capturing image data, and each image manipulation program tries to get around this limitation in different ways.

2. It is possible that every operating system has this "bug" as they may have all originated from the same source that created the very first operating system, and thus all have this limitation hard-coded.

3. It is possible that the first image manipulation program had this limitation, and all other image manipulation programs are built on the basic code foundation of that original image manipulation program and thus has the same limitations in all image manipulation programs.

4. The original format, the first format, used to store image data could have this fault, and thus all other formats inherit the fault from the original.

I draw these conclusions because if it was an isolated problem only found in some image manipulation programs, then you could chalk it up to poor programming. But, it is found in every image manipulation program, even the two most popular and powerful ones, have this problem. Thus, I think there is a deeper issue with image manipulation or image formatting that is creating this issue.

Also, there is the idea of not reinventing the wheel, and thus it is conceivable that the limitation was introduced early on in the days of computers and coding, and never got addressed because people didn't want to reinvent/innovate on image manipulation, figuring that all the code in original formats of OSs, Image Manipulation or Image Capture, was needed.

[Vagabond]

Hooman, that is a pretty interesting article.

The scaling artifact also occurs on the Greeworld tileset in the mapper BTW.

If I recall, I was unable to compiled OP2Editor, but later you were able to compile by placing the project in Release mode. I haven't stepped through the code to see where the scaling is happening. Is fixing this bug something you want to persue, or would you like to let it ride until a new mapper comes around?

I think the next step for me would be to try and compile a release version with a PDB file intact. If we can debug, then we could try to track down the lines feeding the scaled version of the BMP with both the Outpost 2 specific format and the normal BMP format and see where the values diverge (Assuming the bug actually exists inside the OP2Editor codebase).

-Brett

[Hooman]

@lordpalandus: I think you're overthinking this a bit.

The problem is that "everyone" tends to select a simple well known obvious algorithm, which relies on an assumption which doesn't quite reflect reality. The assumption being that color values and perceived color intensity are linear. They're not. Though it is a reasonable approximation for many cases. For the range of color values a computer has, it's not too far from linear, thus the error shouldn't be too far off in most cases. The example in the article makes use of the difference to construct images so as to exaggerate the error, making it more obvious.

@Vagabond: I would like to fix the error. I'll probably have to spend some time debugging to figure out where the error is. I had assumed in was in the OP2Editor C++ code originally, though it's possible the VB code for the frontend might handle things differently than I expect. I would be good to compare how the code runs with both a BMP file, and the custom OP2 format to see what code branches run.

[leeor_net]

My guess is it's the front end. I mean.. it's written in VB6. To go further with my guess, I'm supposing that it's making assumptions about the graphics format and data that are no longer correct after changing it.

Semi on-topic and more to address lordpalandus, the problem with image data on most modern computers is that we usually work in an 8-bits per channel color space... e.g., 24bit images. This is enough to approximate real colors. When it comes to producing accurate images, gamma comes into play. It's actually a really complicated subject, one that I looked into and decided I didn't care enough to understand it fully. Mostly you don't need to worry about it but it does have a dramatic effect when using PBR lighting models in shader based dynamic lighting algorithms.

Which brings me back to my first point --- I seriously doubt this is a gamma issue and instead assume that it's an issue with the code assuming what kind of graphics data it's getting instead of checking it and adjusting itself to what it's actually got (e.g., trying to process a 24-bit image as an 8-bit image, this simply won't be correct at all).

[Vagabond]

Hooman,

I spent some time on this problem today. VS2017 can compile OP2Editor successfully in ReleaseMinSize and Debug. Both compilations allow Mapper2.exe to properly execute and load a map. However, whenever I try to attach the VS2017 debugger to Mapper2.exe, Mapper2.exe will crash when attempting to load a map. The program appears to work fine on load, but after a map is selected, it crashes.

VS2017 will indicate symbols are loaded properly when the PDB file is placed next to op2editor.dll. However, the presence of the PDB file does not affect whether it crashes or not, as the crash occurs whenever debugging is attached.

I'd bet that if the bug is from OP2Editor, it is coming from the function CTileSet::CalcMiniMapColors. By manipulating the values of R, B, and B in this function, I am able to change the color of the minimap in the mapper. I haven't figured out what the combo needs to be in order to fix the minimap.

I wonder if we need some sort of switch here that changes how the minimap is displayed based on if the BMP is Outpost 2 specific or generic.

The repo currently contains a post build event that copies op2editor.dll into the program files for debugging. This forces VS2017 to run in Administrator mode to work. This should probably be documented on the ReadMe or the post build event removed from the project. (Your welcome for its existence.)

There are 6 different solution configurations available in VS2017 to build. What do you think of dropping that down to just Debug and Release? Perhaps renaming ReleaseMinSize to just Release.

I'm going to leave it here for now, but let me know if you want help on anything else in general, committing to the repository or testing as I know you may not be able to test on Linux. Hopefully this helps.

Code: [Select]

// **TODO** Remove
void CTileSet::CalcMiniMapColors(int startTile, int endTile)
{
	int tileByteSize = scanlineByteWidth*headInfo.width;	// Note: assume square tiles
	int tilePixelSize = headInfo.width*headInfo.width;
	int pixelDataOffset;
	RGBQUAD *rgbQuad;
	int red, green, blue;
	int tileNum, x, y;
	int temp;

	// Calculate minimap color of all tiles in given range
	for (tileNum = startTile; tileNum <= endTile; tileNum++)
	{
		// Initialize color components
		red = green = blue = 0;

		// Calculate the minimap color of this tile (sum over all pixels in tile)
		for (y = 0; y < headInfo.width; y++)	// Note: assume square tiles
		{
			for (x = 0; x < headInfo.width; x++)
			{
				// Add the color components of this pixel
				pixelDataOffset = tileNum*tileByteSize + 
									y*scanlineByteWidth + 
									((x*headInfo.bitDepth) >> 3);
				switch(headInfo.bitDepth)
				{
				case 8:
					temp = pixelData[pixelDataOffset];
					rgbQuad = &bmInfo->bmiColors[temp];

					// FIX: flip R and B colors around (they are stored backwards!)
					//red += rgbQuad->rgbRed;
					red += rgbQuad->rgbBlue;
					green += rgbQuad->rgbGreen;
					//blue += rgbQuad->rgbBlue;
					blue += rgbQuad->rgbRed;
					break;
				case 16: // Assume 5/5/5 for RGB components **TODO** test this code
					temp = *(short*)(pixelData+pixelDataOffset);
					blue += (temp & 0x1F) << 3;
					green += (temp & 0x3E0) >> 2;
					red += (temp & 0x7C00) >> 7;
					break;
				default:
					// Do nothing. Pixels will appear black
					break;
				}
			}
		}

		// Scale color components by the number of pixels in the tile
		red /= tilePixelSize;
		green /= tilePixelSize;
		blue /= tilePixelSize;
		// Store the tile minimap color
		rgbQuad = &miniMapColors[tileNum];
		rgbQuad->rgbRed = red;
		rgbQuad->rgbGreen = green;
		rgbQuad->rgbBlue = blue;
		rgbQuad->rgbReserved = 0;
	}
}

Thanks,
Brett

EDIT
-----
Pulled out some extraneous info and improved readability.

[leeor_net]

I think it's safe to assume that you found the culprit.

Two issues I see here -- the 16 bit code is wrong (it mentions 5bit color channels when it's actually 5/6/5) and it doesn't account for 24 bit color depths (8bit per channel). The 'test this code' comment kinda gives it away that this was never tested.

[Hooman]

My original assumption was also that it was a format issue.

There are 6 different solution configurations available in VS2017 to build. What do you think of dropping that down to just Debug and Release? Perhaps renaming ReleaseMinSize to just Release.

Agreed. That would be a useful simplification.

The repo currently contains a post build event that copies op2editor.dll into the program files for debugging. This forces VS2017 to run in Administrator mode to work. This should probably be documented on the ReadMe or the post build event removed from the project. (Your welcome for its existence.)

I can see that being useful. What if the post build step had some text output to indicate Administrator mode is necessary for that step to succeed? Or maybe Administrator privileges could be tested, and either do the operation, or warn that Administrator mode is necessary for that step. That might make failure a little more graceful.


My initial suspect was also CTileSet::CalcMiniMapColors. I investigated this function, including the suspicious comment, back when the issue was first announced months ago. My finding was the suspicious code wasn't even running. It's using the 8-bit case.

Maybe we should start by verifying if the 8-bit case is used for both sets of bitmaps? Assuming it is, maybe we should check the palette structure. Maybe one uses RGB and the other uses BGR. That can actually happen. Though I think I already tested that with no success. Another possiblity is 24-bit entries versus 32-bit entries. Failing that, perhaps the palette length is wrong.

Also, the loop would be more efficient if the case statement was done outside of the loop, and we had two separate loop structures. Slightly more code, but less branching for the inner loop.

[leeor_net]

I don't think BMP can ever be 32-bit? As far as I know there's no alpha channel so they're basically stuck at 24-bit.

Using PNG or TGA would be 32-bit.

This is making me think that we should use a well tested library (like what was used for the map preview generator Vagabond built) to load the images / operate on the image data?

[Hooman]

To clarify, the comment was about the palette entries. The palette of an 8-bit bitmap generally uses 32-bit entries. Some definitions include an Alpha channel, others just mark that byte as reserved and must be 0. Some old OS/2 bitmaps used 24-bit palette entries.

A bit off topic, but BMP can support 32-bit per pixel as well. The original OS/2 format supposedly didn't include past 24-bit, but the contemporary Windows format allows it.


Perhaps there is a good argument to use a 3rd party library. Mostly though, I wanted to avoid dependencies. They're generally a pain to manage with C++. There are often licensing restrictions. Plus OP2 didn't use a general library, and so it can only load a limited subset of bitmaps. The OP2Editor bitmap code was based off of how OP2 supports loading BMP files.

[Vagabond]

Based on comments in here, I did a little more work.

There is a pull request to remove the extra build configurations here: https://github.com/OutpostUniverse/OP2Utility/pull/27. I marked Hooman as reviewer as this is his codebase and I didn't want to change anything on my own.

There is a way to check if someone has admin permissions via the command prompt (https://superuser.com/questions/667607/check-if-current-command-prompt-was-launched-as-the-administrator)

I can try adding the following to the post build event:

• If in administrator mode, copy op2editor.dll to op2mapper install directory
• If not in admin mode, issue a debug comment on build saying Launch Visual Studio in Administrator mode if you wish to automatically copy op2editor.dll into the OP2Mapper install directory for testing.
• • Add a small blurb to the Readme saying that a post build event exists requiring admin mode to copy op2editor.dll into the Mapper install directory. However, the code may be compiled outside of admin mode.
  
  If this sounds good to Hooman, I'll take a crack at it.
  
  I took a quick shot at refactoring the function to remove the switch statement. Unfortunately, I failed due to lack of understanding of what is going on. However, while trying to write out what confused me here, it made some more sense to me. I'll look at refactoring it again in the near future.
  
  In order to test if the tilesets are processing through case 8 or case 16, I refactored case: 8 to read:
  
  
  Code: [Select]

  case 8:
  	temp = pixelData[pixelDataOffset];
  	rgbQuad = &bmInfo->bmiColors[temp];
  
  	// FIX: flip R and B colors around (they are stored backwards!)
  	//red += rgbQuad->rgbRed;
  	red += 0; //rgbQuad->rgbBlue;
  	green += 0; //rgbQuad->rgbGreen;
  	//blue += rgbQuad->rgbBlue;
  	blue += 0;// rgbQuad->rgbRed;
  	break;
  

  
  My goal here was to verify that both the new textures and original OP2 specific textures processed through case 8. When using both sets of tilesets, a black minimap was produced. I think this verifies that both sets of tilesets are using case 8, so the bug is at least not in the case 16 section. I still don't think we have verified for sure the bug is contained in case 8 though, we just know it isn't in case 16. Maybe someone else could verify this test logic looks good though as I'm struggling with the logic of this function in general.
  
  Thanks,
  Brett

[Hooman]

Heh, I was thinking of testing case 8 in exactly the same way.

Ok, so we know it's not the case 16 code then. You're right though, in that it might not be the case 8 code either. It might be somewhere else.

If the problem is in the case 8 code, perhaps it's an issue with the palette format. The loading code should just copy a chunk of data from disk to memory, so if it makes a wrong assumption about the palette format, the lookup of color components will be wrong. That could potentially cause the issue we are seeing.

Maybe we should compare the in memory palettes from the two bitmap formats and see if they are identical. We should check both the length, and the format (RGB, BGR, RGBA, BGRA, etc.).

It might also help to know which tiles have different minimap colors between the two formats.


Nice find on the administrator stuff. Didn't realize there was no standard way to check which works across all relevant versions of Windows. An approximation could still be helpful, though I'm wondering how nicely this will play with post build events. Do they support multiline batch file like decision making?