feat(screenshot): Add threaded JPEG/PNG screenshots without game stalls

[bobtista]

Summary

Replaces the old BMP screenshot with compressed JPEG screenshots that don't stall the game, and adds PNG support.

Closes #1555
Closes #106 ... sort of

Adds a new screenshot function using the stb_image_write library with background threading:

• F12 - Compressed JPEG screenshot (no stalling, ~600KB files)
• Ctrl+F12 - Lossless PNG screenshot

Notes

• stb_image-write is public domain licensed, single-header, zero compilation needed
• Captures frame buffer on main thread (~1-2ms)
• Spawns detached thread for JPEG/PNG compression and disk I/O
• JPEG quality configurable via Options.ini (default 80)
• Works for both Generals and Zero Hour

Testing

• Screenshots save correctly with F12 (jpg) and Ctrl+F12 (png)
• No game stalling during screenshot
• Files appear in correct directory with sequential numbering
  

[Stubbjax]

Some initial thoughts:

• Keep variants of the same function to a single hotkey; instead toggle image format via a new setting in Options.ini
• Apply image compression / quality via a new setting in Options.ini
• Can we consolidate this logic in core instead of repeating the implementation twice?

[xezon]

Agree with Stubbjax.

JPG 90 is big file. Better make it default 80.

Replace BMP screenshot with PNG screenshot. PNG is lossless compressed and always better than BMP.

Make F12 take JPG 80 screenshot. Make CTRL+F12 take PNG screenshot. Make JPG Quality adjustable.

Remove the old BMP code(s) and only use the new code for screenshot.

[xezon]

Regarding Github formatting:

When you write

Addresses #1555

Then it will not close this report when this is merged.

Please read up on it here:
https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#closing-multiple-issues

[bobtista]

Some initial thoughts:

• Keep variants of the same function to a single hotkey; instead toggle image format via a new setting in Options.ini
• Apply image compression / quality via a new setting in Options.ini
• Can we consolidate this logic in core instead of repeating the implementation twice?

RE moving logic to core, I moved what I could to core - but there are a lot more files that need to be moved to core before this can be moved there eg WWVegas/WW3D2/*

[xezon]

Needs rebase.

[bobtista]

Needs rebase.

Done

[Skyaero42]

Looks good to me

[xezon]

Spawning new thread for every image is quite an expensive approach. It would be better to have a screenshot processing thread sleeping and wake up when new work is ready. Unfortunately with c++98 we do not have good sync primitives for that so maybe the thread spawn is ok for now.

[xezon]

I am confused about the byte assignments. Here it assigns BGR and in 16 bit it assigns RGB. In the GenTool code its the exact inverse for both.

[bobtista]

both of my branches produce RGB output, and both of GenTool’s produce BGR output, each is internally consistent, they just target different destinations.

[xezon]

Ok so the 32 bit code path is little-endian specific. It will not work correctly on big-endian. That is why it is so confusing.

Try:

const unsigned int* srcLine = reinterpret_cast<const unsigned int*>(data->pixelData + y * width * 4);

for (x = 0; x < width; ++x)
{
    const unsigned int argb = srcLine[x];
    image[index + 0] = (unsigned char)((argb >> 16) & 0xFF); // r
    image[index + 1] = (unsigned char)((argb >> 08) & 0xFF); // g
    image[index + 2] = (unsigned char)((argb >> 00) & 0xFF); // b
}

This is then also in line with how the 16 bit version does it.

[xezon]

I dont think @feature fits either. This talks about about why a quality setting is capped at 95, not about adding a new feature.

[xezon]

Style: Use ++x, ++y, because that is what it is supposed to do

[xezon]

Prefer reinterpret_cast for pointer casts.

[xezon]

It still offers quality 1. What is the use case for this low quality level?

[stephanmeesters]

This appeared to work and is a bit simpler:

diff --git a/Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp b/Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp
index 7d3e84791..d4dbe7c07 100644
--- a/Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp
+++ b/Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp
@@ -171,7 +171,6 @@ void W3D_TakeCompressedScreenshot(ScreenshotFormat format, Int jpegQuality)
 		return;
 	}
 
-	unsigned int y;
 	unsigned int width = surfaceDesc.Width;
 	unsigned int height = surfaceDesc.Height;
 
@@ -179,11 +178,7 @@ void W3D_TakeCompressedScreenshot(ScreenshotFormat format, Int jpegQuality)
 	// operations are done on the screenshot thread to keep the main thread cheap.
 	const unsigned int bytesPerPixel = is32Bit ? 4 : 2;
 	unsigned char* pixels = new unsigned char[bytesPerPixel * width * height];
-
-	for (y = 0; y < height; y++)
-	{
-		memcpy(pixels + y * width * bytesPerPixel, (const unsigned char*)lrect.pBits + y * lrect.Pitch, width * bytesPerPixel);
-	}
+	memcpy(pixels, lrect.pBits, bytesPerPixel * width * height);
 
 	surfaceCopy->Unlock();
 	surfaceCopy->Release_Ref();

[xezon]

Will this work correctly in regards to lrect.Pitch ?

[stephanmeesters]

I think so because lrect.Pitch * height equals bytesPerPixel * width * height, we're copying all the channels

[greptile-apps]

Success message races write This message is shown as soon as the worker thread is created, before stbi_write_jpg or stbi_write_png has written the file. If the Screenshots directory cannot be created, the path is invalid, or the file write fails, the worker only logs the failure while the player has already been told the screenshot was captured. The success message should be tied to the worker's write result, or changed to say the capture was queued.

Prompt To Fix With AI

This is a comment left during a code review.
Path: Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp
Line: 211

Comment:
**Success message races write** This message is shown as soon as the worker thread is created, before `stbi_write_jpg` or `stbi_write_png` has written the file. If the `Screenshots` directory cannot be created, the path is invalid, or the file write fails, the worker only logs the failure while the player has already been told the screenshot was captured. The success message should be tied to the worker's write result, or changed to say the capture was queued.

How can I resolve this? If you propose a fix, please make it concise.