Bugged aspect ratio scaling in SDL?

Ask for help with ScummVM problems

Moderator: ScummVM Team

Post Reply
UrQuan
Posts: 12
Joined: Wed Oct 10, 2018 5:27 am

Bugged aspect ratio scaling in SDL?

Post by UrQuan » Sat Oct 20, 2018 11:07 pm

Hi again criezy. I have done some research here.
It is actually possible to change the interpolation done for the aspect ratio correction in SDL graphics modes by changing this line of code (for example using kSuperFastAndUglyAspectMode to get nearest neighbor interpolation).
"SuperFastAndUgly"

Beauty is in the eye of the beholder, as they say. . . Take a look at these two images: (download, the preview is too blurry)

FoA_ScummVM_1x_SDL_VeryFastAndGood.png: https://drive.google.com/open?id=1p2wpL ... 8sKlz3INTV
FoA_ScummVM_1x_SDL_SuperFastAndUgly.png: https://drive.google.com/open?id=1_TWAb ... MMzcPYKKHo

Which one was ugly again? :roll:



This was a worst-case scenario to illustrate how bad the "smearing" is potentially, at 1x and with the verb boxes in SCUMM. With nearest neighbor it is a bit mangled, but with bilinear it is just unspeakable.

kVeryFastAndGoodAspectMode, kFastAndVeryGoodAspectMode, kSlowAndPerfectAspectMode are all functionally the same. Not so "slow and perfect" after all, as it turns out.

I quickly added a 4x scaler myself for testing purposes, and ironically, at native 4x and at native 3x to a lesser extent, the dirty scaling doesn't look that terrible. I initially came upon this problem with the scaling artifacts that appear when you then scale to arbitrary resolutions from the initial graphics mode scaling.

This is the reason why this bug was never found or acknowledged. Up until recently you couldn't even change the size of the window manually in SDL, so anyone who didn't force scaling in drivers would never imagine there was a problem, like I didn't until now.



You said
However this is not going to be perfect due to image being scaled in two passes.
Well, I'm not sure about that dude. Look at this: (you can tell from the preview, but download if you don't believe me; they are the same)

KQV_DOSBox_4x_SDL_surfacenb.png: https://drive.google.com/open?id=1oKFEp ... EA9ny4wf7r
KQV_ScummVM_4x_SDL_SuperFastAndUgly.png: https://drive.google.com/open?id=1iH3RT ... oZgSEAj94f

Unless DOSBox *also* has this two-pass scaling thing, there doesn't seem to be a problem.



I actually traced this whole bilinear filtering thing back to the original commit, which was in fact the original commit to introduce aspect ratio correction itself in June of 2003 (!): https://github.com/scummvm/scummvm/comm ... c130f81af0

It came with a comment about nearest neighbor: a little bit faster, but looks ugly

I don't know if that made sense at the time (probably not), but looking at the git history, that meme seems to have lived on in various forms throughout the years . . .

. . . all the way to the current year with the macros:

Code: Select all

#define	kSuperFastAndUglyAspectMode	0	// No interpolation at all, but super-fast
#define	kVeryFastAndGoodAspectMode	1	// Good quality with very good speed
#define	kFastAndVeryGoodAspectMode	2	// Very good quality with good speed
#define	kSlowAndPerfectAspectMode	3	// Accurate but slow code
Hey, here's a petition: let's call 0 "crystal clear", and let's call everything else "spawn of Satan".

I will stop my rant now at the risk of angering some well-meaning developer. I could instead address the elephant in the room here: Was the aspect ratio scaling effectively bugged like this during all these years? Also, is that two-pass scaling still a problem like you said, criezy? (ScummVM SDL versus DOSBox SDL still look a tiny bit different on lower resolutions).

Here are several more screenshots for various comparisons: https://drive.google.com/open?id=1lQmoW ... IJwg7rzRet

and here is my suggested fix: Delete most of the blocks and basically anything that says "bilinear" from graphics/scaler/aspect.cpp. criezy, if you or anyone can try this:

Code: Select all

/* ScummVM - Graphic Adventure Engine
 *
 * ScummVM is the legal property of its developers, whose names
 * are too numerous to list here. Please refer to the COPYRIGHT
 * file distributed with this source distribution.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
 *
 */

#include "graphics/scaler/intern.h" 
#include "graphics/scaler/aspect.h" 

#ifdef USE_ARM_NEON_ASPECT_CORRECTOR 
#include <arm_neon.h> 
#endif 

void makeRectStretchable&#40;int &x, int &y, int &w, int &h&#41; &#123;
	/*
#if ASPECT_MODE != kSuperFastAndUglyAspectMode
   int m = real2Aspect&#40;y&#41; % 6;

   // Ensure that the rect will start on a line that won't have its
   // colors changed by the stretching function.
   if &#40;m != 0 && m != 5&#41; &#123;
	  y -= m;
	  h += m;
   &#125;

#if ASPECT_MODE == kVeryFastAndGoodAspectMode
   // Force x to be even, to ensure aligned memory access &#40;this assumes
   // that each line starts at an even memory location, but that should
   // be the case on every target anyway&#41;.
   if &#40;x & 1&#41; &#123;
	  x--;
	  w++;
   &#125;

   // Finally force the width to be even, since we blit 2 pixels at a time.
   // While this means we may sometimes blit one column more than necessary,
   // this should actually be faster than having the check for the
   if &#40;w & 1&#41;
	  w++;
#endif
#endif
*/
&#125;

/**
 * Stretch a 16bpp image vertically by factor 1.2. Used to correct the
 * aspect-ratio in games using 320x200 pixel graphics with non-qudratic
 * pixels. Applying this method effectively turns that into 320x240, which
 * provides the correct aspect-ratio on modern displays.
 *
 * The image would normally have occupied y coordinates origSrcY through
 * origSrcY + height - 1.
 *
 * However, we have already placed it at srcY - the aspect-corrected y
 * coordinate - to allow in-place stretching.
 *
 * Therefore, the source image now occupies Y coordinates srcY through
 * srcY + height - 1, and it should be stretched to Y coordinates srcY
 * through real2Aspect&#40;srcY + height - 1&#41;.
 */
template<typename ColorMask>
int stretch200To240&#40;uint8 *buf, uint32 pitch, int width, int height, int srcX, int srcY, int origSrcY&#41; &#123;
	int maxDstY = real2Aspect&#40;origSrcY + height - 1&#41;;
	int y;
	const uint8 *startSrcPtr = buf + srcX * 2 + &#40;srcY - origSrcY&#41; * pitch;
	uint8 *dstPtr = buf + srcX * 2 + maxDstY * pitch;

	for &#40;y = maxDstY; y >= srcY; y--&#41; &#123;
		const uint8 *srcPtr = startSrcPtr + aspect2Real&#40;y&#41; * pitch;

		if &#40;srcPtr == dstPtr&#41;
			break;
		memcpy&#40;dstPtr, srcPtr, sizeof&#40;uint16&#41; * width&#41;;

		dstPtr -= pitch;
	&#125;

	return 1 + maxDstY - srcY;
&#125;

int stretch200To240&#40;uint8 *buf, uint32 pitch, int width, int height, int srcX, int srcY, int origSrcY&#41; &#123;
	extern int gBitFormat;
	if &#40;gBitFormat == 565&#41;
		return stretch200To240<Graphics&#58;&#58;ColorMasks<565> >&#40;buf, pitch, width, height, srcX, srcY, origSrcY&#41;;
	else // gBitFormat == 555 
		return stretch200To240<Graphics&#58;&#58;ColorMasks<555> >&#40;buf, pitch, width, height, srcX, srcY, origSrcY&#41;;
&#125;

template<typename ColorMask>
void Normal1xAspectTemplate&#40;const uint8 *srcPtr, uint32 srcPitch, uint8 *dstPtr, uint32 dstPitch, int width, int height&#41; &#123;

	for &#40;int y = 0; y < &#40;height * 6 / 5&#41;; ++y&#41; &#123;

		if &#40;&#40;y % 6&#41; == 5&#41;
			srcPtr -= srcPitch;
		memcpy&#40;dstPtr, srcPtr, sizeof&#40;uint16&#41; * width&#41;;

		srcPtr += srcPitch;
		dstPtr += dstPitch;
	&#125;
&#125;

void Normal1xAspect&#40;const uint8 *srcPtr, uint32 srcPitch, uint8 *dstPtr, uint32 dstPitch, int width, int height&#41; &#123;
	extern int gBitFormat;
	if &#40;gBitFormat == 565&#41;
		Normal1xAspectTemplate<Graphics&#58;&#58;ColorMasks<565> >&#40;srcPtr, srcPitch, dstPtr, dstPitch, width, height&#41;;
	else
		Normal1xAspectTemplate<Graphics&#58;&#58;ColorMasks<555> >&#40;srcPtr, srcPitch, dstPtr, dstPitch, width, height&#41;;
&#125;

#ifdef USE_ARM_SCALER_ASM 
extern "C" void Normal2xAspectMask&#40;const uint8  *srcPtr,
	uint32  srcPitch,
	uint8  *dstPtr,
	uint32  dstPitch,
	int     width,
	int     height,
	uint32  mask&#41;;

/**
 * A 2x scaler which also does aspect ratio correction.
 * This is Normal2x combined with vertical stretching,
 * so it will scale a 320x200 surface to a 640x480 surface.
 */
void Normal2xAspect&#40;const uint8  *srcPtr,
	uint32  srcPitch,
	uint8  *dstPtr,
	uint32  dstPitch,
	int     width,
	int     height&#41; &#123;
	extern int gBitFormat;
	if &#40;gBitFormat == 565&#41; &#123;
		Normal2xAspectMask&#40;srcPtr,
			srcPitch,
			dstPtr,
			dstPitch,
			width,
			height,
			0x07e0F81F&#41;;
	&#125;
	else &#123;
		Normal2xAspectMask&#40;srcPtr,
			srcPitch,
			dstPtr,
			dstPitch,
			width,
			height,
			0x03e07C1F&#41;;
	&#125;
&#125;

#endif   // USE_ARM_SCALER_ASM 
Last edited by UrQuan on Sun Oct 21, 2018 8:41 pm, edited 4 times in total.

User avatar
criezy
ScummVM Developer
Posts: 546
Joined: Sat Sep 23, 2006 10:41 am
Location: West Sussex, UK

Re: Bugged aspect ratio scaling in SDL?

Post by criezy » Sun Oct 21, 2018 11:03 am

UrQuan wrote:You said
However this is not going to be perfect due to image being scaled in two passes.
Well, I'm not sure about that dude.
Right. I just checked that and the code actually behaves better than I remembered. The scaling is done in three passes and not in two passes:
  • First apply the graphics mode scaling (such as 2x or 3x).
  • Then apply the aspect ratio correction.
  • And finally apply the stretching to the screen/window resolution.
It is still not perfect (it depends what the last step does) but better than if the aspect ratio correction was done first.

My ultimate goal is to remove all this code altogether, as I indicated in the other thread you reference here, but it might take time. In the meantime I am not opposed to the code being changed. I am not in favour or always switching to the SuperFastAndUgly, but depending on the Graphics filter option it could either use SuperFastAndUgly or [VeryFastAndGood[/i]. If somebody wants to propose a pull request to that effect that would be welcome I thing. I may do that myself if I get bored, but not promise.

User avatar
criezy
ScummVM Developer
Posts: 546
Joined: Sat Sep 23, 2006 10:41 am
Location: West Sussex, UK

Post by criezy » Sun Oct 21, 2018 5:43 pm

OK, I have made a pull request for a proposed changed along the lines of what I suggested.

UrQuan
Posts: 12
Joined: Wed Oct 10, 2018 5:27 am

Re: Bugged aspect ratio scaling in SDL?

Post by UrQuan » Sun Oct 21, 2018 7:36 pm

criezy wrote: Right. I just checked that and the code actually behaves better than I remembered. The scaling is done in three passes and not in two passes:
  • First apply the graphics mode scaling (such as 2x or 3x).
  • Then apply the aspect ratio correction.
  • And finally apply the stretching to the screen/window resolution.
It is still not perfect (it depends what the last step does) but better than if the aspect ratio correction was done first.

My ultimate goal is to remove all this code altogether, as I indicated in the other thread you reference here, but it might take time. In the meantime I am not opposed to the code being changed. I am not in favour or always switching to the SuperFastAndUgly, but depending on the Graphics filter option it could either use SuperFastAndUgly or [VeryFastAndGood[/i]. If somebody wants to propose a pull request to that effect that would be welcome I thing. I may do that myself if I get bored, but not promise.
There is something interesting in DOSBox that we should consider once you implement arbitrary aspect ratio correction code: While surfacepp in DOSBox is otherwise equivalent to the pixel-perfect stretch mode in ScummVM, I realized now with all this testing that it actually only scales the image in integer multiples, so 4x would be 1280x1000, not 1280x960; sacrificing accuracy for better scaling. It never even crossed my mind that this would help, but it makes sense. I'll see if I can do some testing for you on how that would look in ScummVM, or how much better it is than just 1.2x vertical resolution.


Regarding the filtering option in general, is there any chance to make it less harsh? I just went through all the DOSBox output modes, and the one's that use bilinear filtering are way less harsh than ScummVM, but especially ScummVM OpenGL is just too blurry at the moment: https://drive.google.com/open?id=13_M_G ... YZTuQh8hvy

Edit: I read that Bilinear filtering is rather accurate until the scaling of the texture gets below half or above double the original size of the texture. I guess that's the reason the OpenGL bilinear filtering is so blurry. The "source" image it uses is the unscaled 320x200, unlike SDL which might have the graphics mode scaling first (but from 1x to fullscreen it looks similar to OpenGL).


One more thing while I'm at it:

ScummVM SDL in-game menu: https://drive.google.com/open?id=1SAVD0 ... GYBKVQXxs8
ScummVM OpenGL in-game menu: https://drive.google.com/open?id=1iR8tl ... Dn6TLouQzc

Note that the graphics mode scaling doesn't scale the in-game menu on OpenGL nor on SDL, whereas with arbitrary scaling SDL *does* scale it, and OpenGL does not. The latter is obviously preferable as you can see from the screenshots.


Thanks for you help so far.

UrQuan
Posts: 12
Joined: Wed Oct 10, 2018 5:27 am

Re: Bugged aspect ratio scaling in SDL?

Post by UrQuan » Wed Oct 24, 2018 6:55 pm

1280x960 (4x): https://drive.google.com/open?id=1lbd9Z ... stGFgtEy0J
1280x1000 (4x pixel-perfect): https://drive.google.com/open?id=1wk87f ... FS4QlbE1QM

It is pixel-perfect indeed, as advertised. 4x nearest neighbor is already "good enough" to where there is not a massive difference ultimately, but yeah, it is completely equivalent to non-aspect ratio corrected basically.


To explain exactly what is going on with the overlay scaling: The overlay along with the modern theme cursor (as opposed to the classic theme cursor) and the OSD, do *not* scale arbitrarily on OpenGL, and neither on SDL with graphics mode scaling. Unfortunately, with stretching on SDL they *do* scale. The overlay graphics and text don't scale nicely with nearest neighbor like the game itself game does.

Here's a video: https://www.youtube.com/watch?v=TVSy6lxmJnY

How painful to fix?

Post Reply