SDL_FillRect Regression Bug In SDL_compat 1.2.68 With Zero Width Or Height

Hey everyone! We've got a potential issue to discuss regarding the SDL_FillRect function in SDL_compat 1.2.68. Two bug reports surfaced recently from users of openxcom, and these bugs seem specific to version 1.2.68 of SDL_compat. These issues weren't present when using SDL1.2 directly, nor on SDL_compat 1.2.52 (the version from Ubuntu 22.04, which we use for our AppImage build). Let's dive into the details!

The SDL_FillRect Context Loss

This report focuses on a strange behavior with SDL_FillRect. Users reported seeing stray pixels in a part of the game we call 'minibaseview'. After digging in, we discovered that SDL_FillRect appears to be losing its context when called with a width or height of 0. What's happening is that the origin (the rectangle's x, y coordinates) gets reset to the values from the last valid SDL_FillRect call. This is definitely not the behavior we expect, and it leads to some funky drawing errors.

Diving into the Pseudo-Code

To illustrate the problem, let's look at some simplified pseudo-code that mimics the affected area in our codebase:

// Using an existing surface defined as:
SDL_Surface *_surface;

// Draw a facility
SDL_Rect r;
SDL_LockSurface(_surface); // We use a wrapper, but in essence it boils down to this call.

// In the actual code we loop over multiple facilities which can have different sizes.
int color = 48;                      // Depends on a runtime value, but does not matter for this report.

r.x = 1;                             // Depends on a runtime value, but does not matter for this report.
r.y = 1;                             // Depends on a runtime value, but does not matter for this report.
r.w = 2;                             // Is a multiple of 2 depending on a runtime value. For this report the minimum value is important.
r.h = 2;                             // Is a multiple of 2 depending on a runtime value. For this report the minimum value is important
SDL_FillRect(_surface, &r, color+3); // We use a wrapper, but in essence it boils down to this call.
r.x++;
r.y++;
r.w--;                               // Width = 1 for the minimum value.
r.h--;                               // Height = 1 for the minimum value.
SDL_FillRect(_surface, &r, color+5); // We use a wrapper, but in essence it boils down to this call.
r.x--;
r.y--;
SDL_FillRect(_surface, &r, color+2); // We use a wrapper, but in essence it boils down to this call.

// This is the start of the context loss when using `SDLcompat 1.2.68`
r.x++;
r.y++;
r.w--;                               // Width = 0 for the minimum value.
r.h--;                               // Height = 0 for the minimum value.
SDL_FillRect(_surface, &r, color+3); // We use a wrapper, but in essence it boils down to this call.
// End of context loss

r.x--;
r.y--;
setPixel(r.x, r.y, color+1);         // A wrapper call that essentially set a pixel value for [r.x, r.y], which is offset by now since x,y are not pointing to the correct location.

// Following line is called after the loop over all facilities.
SDL_UnlockSurface(_surface);         // We use a wrapper, but in essence it boils down to this call.

Let's break down what's happening here. We're drawing a facility on a surface, and the size of this facility can vary. We're using SDL_FillRect to fill rectangles of different sizes. The crucial part is when we end up calling SDL_FillRect with a width and height of 0. This seems to be the trigger for the context loss in SDL_compat 1.2.68.

In this code snippet, we first initialize an SDL_Surface and then lock it using SDL_LockSurface. This is a necessary step to ensure that we can safely manipulate the surface's pixels. We then define an SDL_Rect structure r to represent the rectangle we want to fill. The coordinates and dimensions of this rectangle are determined by runtime values, but for the sake of this example, we'll focus on the minimum values that trigger the bug.

We start by setting the x and y coordinates of the rectangle to 1, and the width w and height h to 2. We then call SDL_FillRect to fill this rectangle with a specific color. After this, we decrement the width and height of the rectangle, effectively making it smaller. We call SDL_FillRect again with the updated dimensions. This process is repeated a few times, each time making the rectangle smaller.

The critical point is when we decrement the width and height to 0. When SDL_FillRect is called with a rectangle that has a width and height of 0, it appears to lose its context in SDL_compat 1.2.68. This means that the internal state of SDL_FillRect becomes corrupted, and subsequent calls to SDL_FillRect may not behave as expected.

In the code, after calling SDL_FillRect with a zero-sized rectangle, we attempt to set a pixel at a specific coordinate using a setPixel function. However, because SDL_FillRect has lost its context, the x and y coordinates used in setPixel are now incorrect, leading to the stray pixels observed in the minibaseview.

Finally, after the loop over all facilities is complete, we unlock the surface using SDL_UnlockSurface. This releases the lock on the surface, allowing other parts of the program to access and modify it.

The Consequences of Context Loss

After this call with zero width and height, the next time we try to set a pixel using setPixel(r.x, r.y, color+1), the coordinates r.x and r.y are no longer pointing to the correct location. This is because the internal state of SDL_FillRect has been corrupted, causing it to miscalculate the pixel address. As a result, we end up drawing a pixel in the wrong place, leading to the stray pixels that users have reported.

Mitigation Strategy

We've found a way to work around this issue in our code. We can simply avoid calling SDL_FillRect if we detect that the width or height is zero. This prevents the context loss from happening in the first place. However, we believe that this is a regression in SDL_compat 1.2.68 that's worth reporting, as it could affect other projects as well.

By not calling SDL_FillRect when the width or height is zero, we prevent the context loss and ensure that subsequent drawing operations are performed correctly. This workaround allows us to maintain the correct visual appearance of the minibaseview and avoid the stray pixels that were previously observed.

Why This Matters

This behavior is unexpected and can lead to visual glitches in applications using SDL_compat 1.2.68. It's crucial to understand this issue, especially if you're working with dynamic sizes and potentially ending up with zero-dimension rectangles. This bug could manifest in various ways, from minor graphical artifacts to more severe rendering problems, depending on how SDL_FillRect is used in the application.

Potential Impact on Other Projects

While we've identified this issue in openxcom, it's likely that other projects using SDL_compat 1.2.68 could be affected as well. Any application that relies on SDL_FillRect and handles rectangles with dynamic dimensions could potentially encounter this bug. This is particularly true for applications that frequently create and manipulate rectangles, such as games, graphical editors, and visualization tools.

The fact that this bug wasn't present in earlier versions of SDL_compat or in SDL1.2 itself suggests that it's a regression introduced in SDL_compat 1.2.68. This makes it even more important to address, as developers who have upgraded to this version may be unknowingly introducing the bug into their applications.

The Importance of Reporting Regressions

Reporting regressions like this is vital for maintaining the stability and reliability of software libraries. By bringing this issue to the attention of the SDL development team, we can help ensure that it's properly investigated and fixed in a future release. This not only benefits our project but also helps the broader SDL community by preventing other developers from encountering the same problem.

A Deeper Dive into SDL_FillRect

To fully understand the implications of this bug, it's helpful to have a solid understanding of how SDL_FillRect works. SDL_FillRect is a fundamental function in SDL for drawing filled rectangles on a surface. It takes an SDL_Surface pointer, an SDL_Rect pointer, and a color value as input. The function then fills the specified rectangle on the surface with the given color.

Under the hood, SDL_FillRect likely performs a series of pixel write operations to fill the rectangle. This involves calculating the memory address of each pixel within the rectangle and writing the appropriate color value to that address. The efficiency and correctness of this process are crucial for the overall performance and stability of SDL-based applications.

When SDL_FillRect is called with a zero-width or zero-height rectangle, it should ideally handle this case gracefully and either do nothing or return an error. However, in SDL_compat 1.2.68, it appears that this edge case is not being handled correctly, leading to the context loss issue.

Our Mitigation in Detail

Our mitigation strategy of not calling SDL_FillRect when the width or height is zero is a simple but effective way to avoid the bug. Before calling SDL_FillRect, we now check the dimensions of the rectangle. If either the width or height is zero, we skip the call. This prevents the context loss from occurring and ensures that subsequent drawing operations are performed correctly.

While this workaround resolves the issue for our specific use case, it's not a universal solution. Other applications may have different requirements and may need to handle zero-sized rectangles in a different way. Therefore, it's essential to address the underlying bug in SDL_compat 1.2.68 to provide a robust solution for all users.

Next Steps

We hope that this detailed explanation helps shed light on the issue. We've reported this to the SDL development team, and we're eager to see how they address it. In the meantime, we'll continue to use our mitigation strategy to ensure the stability of openxcom.

Mouse Jumping Issue - A Sneak Peek

As a side note, we've also encountered another issue in SDL_compat 1.2.68 related to mouse jumping. The mouse cursor seems to jump erratically under certain circumstances. We're still investigating the specifics of this issue, but we suspect it might be related to input handling in SDL_compat. We'll be posting another report with more details once we have a clearer understanding of the problem. So, stay tuned for that!

We are still investigating the specific circumstances surrounding the mouse jumping issue, as it doesn't happen consistently. We want to gather as much information as possible before submitting a separate bug report. This includes identifying the exact conditions that trigger the issue, the frequency with which it occurs, and any potential workarounds.

In the meantime, if any other developers have encountered similar mouse jumping issues in SDL_compat 1.2.68, we encourage them to share their experiences. This could help us to identify common patterns and narrow down the root cause of the problem.

Stay Tuned for Updates

That's all for now, folks! We'll keep you updated on the progress of both the SDL_FillRect regression and the mouse jumping issue. Thanks for reading, and happy coding!

We'll continue to monitor the SDL development team's response to these bug reports and provide any additional information or assistance that we can. We believe that by working together, we can help to improve the quality and stability of SDL and ensure that it remains a valuable tool for game developers and other software creators.

If you have any questions or comments about these issues, please feel free to share them in the discussion below. We're always happy to engage with the community and exchange ideas.