rsx: Fix cropped and pillar boxed display buffer - #19133
Conversation
| // NOTE: This is decided from the display buffer and the AV config alone, never from whatever render | ||
| // target happens to be in the surface store. Keying it off the surface makes the output geometry | ||
| // change between frames and the picture visibly pulses. | ||
| if (const u32 row_width = buffer_pitch / avconfig.get_bpp(); |
There was a problem hiding this comment.
This seems like a hack.
You just increase the width to whatever the pitch suggests as long as it still fits inside the resolution.
You should try to find out why the width is too small in the first place and why it ultimately ends up changing the aspect ratio.
Also. I don't think you understand what this code does, otherwise you wouldn't leave all these bloated AI comments here. This violates our AI rules.
It's nice to show enthusiasm, just keep in mind that AI is a tool for your own productivity, not for barfing out random suggestions.
There was a problem hiding this comment.
what you see here is only a minimal fraction of work and I agree the logs on the code could appear wrong/non sense (they are there according to AI disclosure above). Analysis has been done instrumenting logs and comparing results (width etc. were logged, detailed for all games tested). Just waiting kd-11 for a sentence.
|
Another hardware claim. Cannot be merged without accompanying tests. |
kd-11
left a comment
There was a problem hiding this comment.
Overall, it seems like an obvious hack. But ofc it could also be a hardware bug that went unnoticed and a game made use of a broken display definition at release.
Chain:
- cellVideoOutConfigure -> 720p
- sys_rsx_context_attribute -> attr_id=0x104 to set the display out parameters
The claim made here is that the video out scanner ignores the 'width' value and just reads the entire pitch. A few things need to be checked.
- Is it really reading beyond 'width' defined for the display buffer?
- Is it clamping to pitch / bpp?
- What happens if pitch is too small? We assume that it crops, but we don't have real data
|
I saved the session history with AI. Here attached, if it can provide you some of the required information |
Expected LLM output. This is inverse reasoning to justify the change - it is not giving a good reason why the change is suggested, it only knows that suggesting the fix makes the problem go away and makes up the reasoning and risk profile to support the known-good finding. You should never ask AI to check its own reasoning like this, it will likely not give you anything meaningful. |
I never suggest fix. I provide symptoms (image stretched on the right, works on vulkan but not ogl etc.) and regressing PR / commit, evaluate and choose the proposal, run the proposal / instrumented code, provide results and new symptoms (e.g. this is working but gt6 is regressed with those symptoms etc.), evaluate the new proposals and reiterate the process. No-regression tests are made on each proposal to provide new symptoms etc. |
AI Disclosure:
Fix regression introduced by the PR #5427). The regression was introduced in commit c80c7f0.
The regression is present on both Vulkan and OpenGL.
This PR should fix that regression.
fixes #18155
NOTE: marking the PR as draft for review from kd-11
AI Summary
Bug.
Games that under-declare their display buffer width were presented cropped and pillarboxed. Brütal Legend registers a display buffer of 1152x720 with a pitch of 5120 (= 1280 px per row) while rendering the full 1280 columns. flip() took the declared 1152 as the presentation width, which had two effects: the source rect covered only the left 1152 columns, cutting the right edge of the frame, and the source aspect ratio became 1152/720 = 1.6 instead of 16:9, so aspect_convert_region pillarboxed the result.
Origin.
Regression from c80c7f0 ("rsx: Typo fix", Dec 2018), which inverted
to >, removing the clamp that had been compensating for these buffers. The 2020 present refactor (5e0ca4c) then consolidated the cropping semantics as std::min(surface_width, info->width).
Fix.
In GLGSRender::flip and VKGSRender::flip, recover the full row width from the pitch before the frame-size clamp:
Bounded by the video mode so buffers with genuine pitch padding are untouched, and self-consistent buffers (pitch == width * bpp, the normal case) never trigger it.
Note on the approach.
The clamp is deliberately not restored in its original form. Deriving the presentation width from the render target found in the surface store makes the output geometry depend on transient state and vary between frames — that produced visible pulsing in GT6 during development. Computing it from the display buffer and AV configuration alone makes it frame-invariant.
Brutal Legend Demo at 720p