Software: Fix out of bounds accesses in CopyRegion

[Pokechu22]

Fixes issue 11393 (I haven't actually tested the game, only fifologs).

All of the Rogue Squadron 2 fifologs segfualt (per the buildbot log (lines 309, 1624, 2939, and 4509)), starting in 609a17a (hybrid XFB). This also happens on Windows (where it fails with Critical error detected c0000374 (heap corruption))

The problem is that left and top make no sense for a width by height array; they only make sense in a larger array where from which a smaller part is extracted. EncodeXFB already handles the extraction, so CopyRegion's only use is to resize the image. The rects in ScaleTexture have left and top as 0 (from TextureConfig::GetRect as called by TextureCacheBase::ScaleTextureCacheEntryTo), so this changes nothing for them. I'm not entirely sure why Rectangle is used here at all; a Size or something like that (which I don't think currently exists) would be better.

This is a draft to check for regressions with FifoCI (like #9315).

[Pokechu22]

Hmm, per fifoci, my changes have shifted a lot of things in addition to fixing Rogue Squadron 2. Some changed horizontally (e.g. simpsons-game) and some changed vertically (sms-gc). As for RS2, rs2-glass looks incorrectly centered compared to on OGL (probably the same issue), and the HUD looks incorrectly colored and is missing blue dots in the minimap for rs2-skybox (OGL) and rs2-bumpmapping (OGL). rs2-zfreeze shows missing triangles, which is probably the same issue as rs3-bumpmapping (#9315 (comment)). I'm also not sure what's up with the messed up first frames (not a new issue, and an software renderer only thing).

I did notice that there is a case for textures that uses a nonzero top and left, too, which might explain some of the shifts (I'm not entirely sure what all this code does or how it's supposed to work, though):

dolphin/Source/Core/VideoCommon/TextureCacheBase.cpp

Lines 1942 to 1956 in b4b2a12

	srcrect.left = src_x;
	srcrect.top = src_y;
	srcrect.right = (src_x + src_width);
	srcrect.bottom = (src_y + src_height);
	dstrect.left = dst_x;
	dstrect.top = dst_y;
	dstrect.right = (dst_x + dst_width);
	dstrect.bottom = (dst_y + dst_height);
	// We may have to scale if one of the copies is not internal resolution.
	if (srcrect.GetWidth() != dstrect.GetWidth() || srcrect.GetHeight() != dstrect.GetHeight())
	{
	g_renderer->ScaleTexture(stitched_entry->framebuffer.get(), dstrect, entry->texture.get(),
	srcrect);
	}

[lioncash]

This include doesn't seem necessary

[Pokechu22]

Yep, it's a leftover from testing. Though it's worth noting that both uses of CopyRegion start with vectors and use data() for it, so there isn't any strong reason to use raw pointers here (apart from performance maybe, but it's the software renderer...)

[lioncash]

It was likely to keep it from needing to depend on a single container type, making it more flexible, since it can copy to any contiguous region of memory.

[Pokechu22]

Similarly... why does ScaleTexture make copies into and out of vectors? Wouldn't something like

  const SWTexture* software_source_texture = static_cast<const SWTexture*>(src_texture);
  SWTexture* software_dest_texture = static_cast<SWTexture*>(dst_framebuffer->GetColorAttachment());

  CopyRegion(software_source_texture->GetData(), src_rect, software_dest_texture->GetData(), dst_rect);

work? (Ignoring issues with the rectangles themselves).

[Pokechu22]

Oh, it's because GetData() returns a pointer to u8 while this is supposed to operate on a pointer to Pixel (or something else) with a larger size. It still seems awkward to make a copy; is that actually required?

[lioncash]

degasus, Stenzek, or some other graphic dev would be way more qualified to answer the why here than me. But at a glance, I agree.

[Pokechu22]

Hmm, interesting. The shifting issue I had before was due to previous use of std::round in CopyRegion, which I had removed. I guess it makes sense (you'd want to round to the nearest pixel, not the previous pixel). With that, a modified version of CopyRegion to not go out of bounds works fine and only has changes for the (previously broken) RS2 tests. Oddly, that happens regardless of whether CopyRegion respects top and left or not; ignoring them produces no changes from not ignoring them. Perhaps none of the test cases actually hit dst_x/dst_y being nonzero. It's probably better to still support regions other than the full buffer in CopyRegion though, so I'll keep it.

[dolphin-emu-bot]

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

• rs2-bumpmapping on sw-lin-mesa: diff
• rs2-glass on sw-lin-mesa: diff
• rs2-skybox on sw-lin-mesa: diff
• rs2-zfreeze on sw-lin-mesa: diff

_{^{automated-fifoci-reporter}}

[iwubcode]

Whoops, sorry about that. LGTM!