[PATCH] Fix presentation to not blur the image at window size resolution

[Eldray]

Currently the presentation code in Dolphin is severely flawed, because there is no way to reliably prevent it from blurring the rendered 3D image due to non-1:1 scaling.

In theory, specifying "window size" as the native resolution is supposed to do that, but in practice that's not guaranteed due to a design flaw in the presentation code.

This manifests for instance when playing Xenoblade Chronicles at 1920x1080, where a 1920x1079 to 1920x1080 scaling is performed, resulting in catastrophically blurring the image.

The reason is that coordinates are scaled using the y * getTargetHeight() / EFB_HEIGHT formula where getTargetHeight() is an integer.

Unfortunately, the fact that getTargetHeight() must be integer means that the size of the 3D rendering region ends up off-by-one.

This patch fixes the issue by storing the "getTargetHeight() / EFB_HEIGHT" value, and similarly for the width, explicitly as a floating-point value

The resulting code is also faster because the division is not performed every time.

The code in CalculateXYScale is also fixed to remove "- 1" operations that seem to be incorrect and don't seem to have any reason for being there.

[ExtremeDude2]

Sounds good, maybe you could post before and after pics?

Edit: Attempting to make a 64-bit for those who want it, will post back here soon.

Edit 2: I keep getting a "Patch format detection failed" error.

[neobrain]

I changed the scaling logic from floating points to integers on purpose some time ago to avoid certain issues... I'll have a look at your patch though.

EDIT:

The code in CalculateXYScale is also fixed to remove "- 1" operations that seem to be incorrect and don't seem to have any reason for being there.

This one is correct. I actually spotted it some time ago as well (was probably my own fault anyway), but apparently forgot fixing it...

This manifests for instance when playing Xenoblade Chronicles at 1920x1080, where a 1920x1079 to 1920x1080 scaling is performed, resulting in catastrophically blurring the image.

I don't quite see how using floats everywhere is supposed to fix that; could you look a bit further into the issue and explain why a value of 1079 is being used in the first place?

[Eldray]

The reason is that Wii games basically embed the actual picture, which has the "original XFB" size (s_XFB_height), into larger area, sized by the "original EFB" (EFB_HEIGHT) size.

These two sizes need to get mapped into the "emulator XFB size" (dst_rect.bottom - dst_rect.top) and "emulator EFB size" (aka s_target_height), which must be proportional to the original ones.

The old code mapped coordinates in this fashion:
y * s_target_height / EFB_HEIGHT

However, what we really want is this, because we want to match the XFB height of the real picture, not the EFB size which isn't related to display:
y * s_XFB_height / (dst_rect.bottom - dst_rect.top)

Obviously, if s_target_height must be an integer, then there is no way to set it such that the two expressions are equivalent, in general.

Thus, the old code computed the floating-point scaled coordinates wrong, and even worse s_target_height was rounded down to an integer, which resulted in the bound of the image being off by one, resulting in a non-1:1 scaling.

Hence, the patch replaces the expression with:
y * s_target_yscale
which is rounded for the integer case

Alternatively, you could use a rational number with the s_XFB_height / (dst_rect.bottom - dst_rect.top) value, but division is a very slow operation, so the floating point implementation is probably better.

EFBToScaledY is then set to round the value.

In fact, now that I've taken a further look, the EFBToScaledY function should probably be removed, and callers should use EFBToScaledYf instead, and then use floor/ceil/rounding depending on what they need the value for (i.e. use floor for a lower bound, ceil for an upper bound, round for an approximation).

Of course, all this reasoning holds for the x coordinate and width as well.
Also, the whole discussion assumes that internal resolution has been chosen to match the window size.

[neobrain]

Ok so far, one more question: At which place in the code is that "1920x1079 to 1920x1080 scaling" scaling you mentioned performed? I'd guess within Renderer::Swap (in the !UseXFB case), just making sure though.

[Eldray]

Yes.

BTW, enabling real XFB tends to cause other issues such as garbage on the borders of the image, and possibly misalignment, but I haven't investigated that.

[neobrain]

I still don't quite get why Renderer::GetTargetHeight() would evaluate to 1079 at that point though...

The thing is, I'm kinda uncomfortable with changing that stuff back to float since it really seems like a hack to hide the bad design that the code uses. I'd rather like to fix the underlying design issue instead of working around it.

[Eldray]

It's not getTargetHeight(), it's the height of targetRc, which is equal to EFBToScaledY(s_XFB_height), equal to s_XFB_height * s_target_height / EFB_HEIGHT.

Xenoblade uses s_XFB_height = 456, which means that a value of s_target_height = 1080 * 528 / 456 = 1250.52 would be needed to obtain 1080 in the formula.

Since s_target_height is rounded down to 1250, the formula gives 456 * 1250 / 528 = 1079.54, rounded down to 1079.

The fundamental problem is that the "s_target_height / EFB_HEIGHT" value needs to be 1080 / 456, but that cannot be expressed as an integer divided by EFB_HEIGHT = 528.

Just rounding either s_target_height up or the result of EFBToScaledY up might put a band-aid to it, but EFBToScaledYf would still return fundamentally wrong values, which means EFBToScaledY called with values in the middle of the picture could also return values off by one at certain points.

It really needs the scale to be represented either as floating point, or as an arbitrary rational number, rather than integer / EFB_HEIGHT.

[Parlane]

So do this:


456 * ((1080 * 528) / 456) / 528

Omg 1080 lol

Make a version of CalculateTargetScale that takes a multiplier value.

Code:

void Renderer::CalculateTargetScale(int x, int y, int &scaledX, int &scaledY, int multiplier)
{
    switch (g_ActiveConfig.iEFBScale)
    {
        case 3: // 1.5x
            scaledX = (multiplier * x*3) / 2;
            scaledY = (multiplier * y*3) / 2;
            break;
        case 4: // 2x
            scaledX = multiplier * x * 2;
            scaledY = multiplier * y * 2;
            break;
        case 5: // 2.5x
            scaledX =  (multiplier * x * 5) / 2 ;
            scaledY =  (multiplier * y * 5) / 2;
            break;
        case 6: // 3x
            scaledX = multiplier * x * 3;
            scaledY = multiplier * y * 3;
            break;
        case 7: // 4x
            scaledX = multiplier * x * 4;
            scaledY = multiplier * y * 4;
            break;
        default:
            scaledX = multiplier * x;
            scaledY = multiplier * y;
            break;
    };
}

// return true if target size changed
bool Renderer::CalculateTargetSize(int multiplier)
{
    int newEFBWidth, newEFBHeight;
    switch (s_LastEFBScale)
    {
        case 0: // fractional
            newEFBWidth = (int)(EFB_WIDTH * xScale);
            newEFBHeight = (int)(EFB_HEIGHT * yScale);
            break;
        case 1: // integral
            newEFBWidth = EFB_WIDTH * (int)ceilf(xScale);
            newEFBHeight = EFB_HEIGHT * (int)ceilf(yScale);
            break;
        default:
            CalculateTargetScale(EFB_WIDTH, EFB_HEIGHT, newEFBWidth, newEFBHeight, multiplier);
            break;
    }

    if (newEFBWidth != s_target_width || newEFBHeight != s_target_height)
    {
        s_target_width = newEFBWidth;
        s_target_height = newEFBHeight;
        return true;
    }
    return false;
}

[Eldray]

Yeah, but the XFB height depends on the game AFAIK, so you need to use variable numerator and denominator values for the scale.

Using rational numbers like that is a valid solution just as using floats, although it's probably more complex and slower.