Fix u32 to s32 in two functions in #17666

[digant73]

fixes #17730

Fix signature for the following utility functions implemented in #17666:

inline void block2d_copy_with_stride(u8* dst, const u8* src, u32 width, u32 height, u32 src_pitch, u32 dst_pitch, u8 src_stride, u8 dst_stride)

inline void block2d_copy(u8* dst, const u8* src, u32 width, u32 height, u32 src_pitch, u32 dst_pitch)

Variables in_pitch and out_pitch are defined as s32 but passed as u32 on those functions so breaking the arithmetic on pointers (e.g. src += src_pitch;) in case of negative pitch value.

Just a doubt about:

s32 in_pitch = REGS(ctx)->nv0039_input_pitch();
s32 out_pitch = REGS(ctx)->nv0039_output_pitch();

REGS(ctx)->nv0039_input_pitch() and REGS(ctx)->nv0039_output_pitch() return u32 but assigned to s32. Not sure if intended or a possible bug to fix. Apparently it seems intended (e.g. negative values are possible; otherwise this bugfix should not exist) although I don't understand why REGS-><xx>_pitch() have been defined as u32.

NOTE:

Similarly to the reported two bugged functions, I also see in #17666 the use of function clip_image(....). e.g.:

clip_image(dst.pixels, src.pixels, dst.clip_x, dst.clip_y, dst.clip_width, dst.clip_height, dst.bpp, src.pitch, dst.pitch);

Many of the passed variables have a different (and dangerous) type than what the function is expecting. E.g. src.pitch is u32 (while in_pitch was s32) while the function's signature is expecting an int (although on a 64bit arch int should be equivalent iirc to s64 so covering u32. Similar considerations for other arguments.
Just in case it could be something to review and fix to avoid a similar issue as the one fixed by this PR

[Megamouse]

A pitch is the number of bytes in a row (including padding).
That means by definition it's >= width and >= 0.
I highly doubt that it would ever be negative, that would make no sense.
And any image or memory block with a pitch of >= int32 max would have 4gb per line...

[Megamouse]

Meaning there's no bug.
If anything the variables should be u32

[digant73]

A pitch is the number of bytes in a row (including padding). That means by definition it's >= width and >= 0. I highly doubt that it would ever be negative, that would make no sense. And any image or memory block with a pitch of >= int32 max would have 4gb per line...

those have been also my first considerations (for that reasons I also provided some doubts and notes) but those changes restored the same arithmetic and have been confirmed in #17730 as fixing the issue.

[kd-11]

and have been confirmed in #17730 as fixing the issue.

Hiding the issue and fixing the issue are 2 different things. We're not interested in hiding bugs to sweep them under the rug. Now we'll need an explanation why it was underflowing before moving on.
If possible, add unit tests as well.

[kd-11]

This just hides the issue. It's either:

1. We cannot have negative pittch and something is underflowing.
2. We can pass negtive pitch to nv0039 and we do not correctly support it.

Note all the other logic in the file is explicitly written to support positive integers. The in and out pitch being s32 is a coincidence/bug here, those variables should also be u32 to match the register decoded output.

[kd-11]

My suggestion here is this:

• Check the actual negative values. If they're sensible and not random bitsoup, it could be really supported.
• If that's the case, then we need to write hardware tests and execute on PS3 to see what happens. Does it just reverse rows? What about columns? etc
• Once we have that info, we document it in the file and implement it.

That's pretty much how all the features are reversed and worked on.

[Megamouse]

well. it would kinda make sense if you want to mirror some memory

[digant73]

My suggestion here is this:

• Check the actual negative values. If they're sensible and not random bitsoup, it could be really supported.
• If that's the case, then we need to write hardware tests and execute on PS3 to see what happens. Does it just reverse rows? What about columns? etc
• Once we have that info, we document it in the file and implement it.

That's pretty much how all the features are reversed and worked on.

yes, I simply restored the previous (apparently not crashing) logic, received feedback as no more issue and shared info, doubts in order to request a cross check from devs for possible hidden bugs or not properly managed logic.
Scope of this kind of work (pseudo dev-tester) is to see issues with the eyes of a non developer (devs usually are biased and often never consider something as possible or that can happen -> they will never investigate without a trigger).
It's up to main developers the troubleshooting once someone highlighted some anomalies in data (unexpected negative values in this case) and code and to formulate the conclusions (real fix, hidden issue, unexpected logic that needs to be properly managed etc.).

I would say this PR reached its scope and likely deserve to be closed and replaced by a PR (from main developers) collecting the needed info for a possible specific bugfix