VideoSoftware: Implement xfb copy filter (Deflickering/Brightness)

[phire]

The Copy filter does 3 things.

1) Resolve the Multisampled framebuffer. Games can specify 7 coefficients, the 3 samples from the current pixel, and two samples from the pixels above and below.
Since dolphin hasn't implemented Multisampling, even in video software, I haven't implemented this fully.

When games aren't using the multisampling mode, the 3 middle coefficients all point at the current pixel, while the upper/lower coefficients also point at a single upper or lower pixel.

2) Deflicker filter. The game does a vertical blur across 3 lines, blending the odd and even fields together which minimises the flickering effect inherent to 480i consoles on interlaced TVs.
This is why 99.9% of games render all 480 lines each frame even when rendering at 60fps.

The filter is counter-productive for progressive displays, which is why dolphin hasn't implemented it before now, though it does blur away the dither pattern.

When we implement this in the hardware backends, we can add an enhancement which points all 7 coefficients at the middle pixel, forcing the deflicker filter off.

3) Brightness filter. You are meant to choose coefficients that add up to 64. But some games cleverly break this rule to create a brightness filter. Resident Evil 4 uses this to implement a global brightness slider option, while other (Rogue Squadron 3?, mario galaxy?) games use it to implement a fade to black effect.

If it wasn't for 3, there would be little reason to implement this, except so users of melee/brawl could see the deflicker on/off option in action.

---

This change is

[lioncash]

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.

---

Source/Core/VideoCommon/BPMemory.h, line 1027 [r1] (raw file):

  BitField<44, 6, u64> g;

  void getCoefficients(u8 *filterCoefficients)

GetCoefficients

Also, you can just return a std::array.

---

Comments from Reviewable

[JMC47]

I can't test this. Software renderer only shows a green screen even after following the instructions.

[phire]

Correct wrapping behaviour:
There is optional clamping at the top and bottom of the copy region (see the UPE_copy struct)

If you don't enable clamping, 0 - 1 will wrap around to 1023, which isn't backed by any memory and returns garbage.

Thanks to Extrems for doing some hardware testing.

[shuffle2]

Cool to see it implemented, finally.

[sepalani]

Is there a reason for const std::array<u8, 7> filterCoefficients not being const-reference?

[lioncash]

Review status: 0 of 5 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

Source/Core/VideoBackends/Software/EfbInterface.cpp, line 554 [r3] (raw file):

          //
          //         In our implementation, the garbage just so happens to be the top or bottom row.
          //         Statically, that could happen.

Should this be 'Statistically'?

---

Source/Core/VideoCommon/BPMemory.h, line 1027 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

GetCoefficients

Also, you can just return a std::array.

Still needs to be `GetCoefficients()` (also it can be made const qualified as well).

---

Source/Core/VideoCommon/BPMemory.h, line 1027 [r1] (raw file):

Previously, degasus (Markus Wick) wrote…

What do you think about a pointer to u8[7], or better, u8[7] as return value.

A std::array is better in this situation, IMO.

---

Source/Core/VideoCommon/BPMemory.h, line 1039 [r3] (raw file):

      };
  }

unnecessary newline?

---

Comments from Reviewable

[degasus]

What do you think about std:array<u8, 8>? So it will use exactly one register: https://godbolt.org/g/TbA0LM

[lioncash]

@degasus I'm not particularly sure what you're asking, but if you mean modify GetCoefficients so it returns 8 elements instead of 7, then I can only really say "please don't micro-optimize like that", by adding completely unnecessary elements to the array. GetCoefficients means 'get the coefficients' not 'get the coefficients and a dummy entry'. That also makes it look like a potential off-by-one bug to anyone not familiar.

That test isn't particularly indicative of a gain either, since it's literally evaluating the induction variable results and eliding the loop entirely (since it knows the fixed constants at that point). It's also using a looping context where it wouldn't be used to begin with.

You'd typically get something like this instead. We currently compile under O2 as well, so even then, we'd still not get that level of optimization to begin with.

[phire]

I consolted godbolt and when you make the code more complex, std::array<u8, 8> actually compiles worse on gcc (but identical on clang).

However, passing by constant reference is faster and produces identical code for both on both compilers.

[phire]

For refrence, here is a version that uses our BitField class: https://godbolt.org/g/LAj294

GCC doesn't optimise the unpacking as much, but clang produces the same result.

[Fallcrest]

@phire When you mention Mario Galaxy, does this PR fix the missing roar effects? (Issue 8327)

[phire]

It was theorized by @JMC47 that it might at the time this commit message was written. However later testing shows otherwise.

[Fallcrest]

@phire So there is no current explanation for the missing effect? It is so odd that something that minor has escaped finding for so long. Are there any other effects missing from other games that might be related?

[JMC47]

Can this be rebased and merged? Would be nice to update the current oldest issue in dolphin :D

[RisingFog]

This PR still need to be rebased.

[Buddybenj]

Any update on this?

[Buddybenj]

Is there any chance this will be rebased?

[badkarma12]

Can this PR be closed now that #6369 is merged?