Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GSOC 2012 Scaler Plugins #271

Open
wants to merge 133 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@singron
Copy link
Contributor

commented Aug 20, 2012

This adds a more modular and robust way for adding and managing scalers as plugins.

Features:

  • All scalers can be initialized to any PixelFormat as long as the corresponding Colormask was used to build it. This includes 4 byte-per-pixel formats
    ** (i.e. most scalers have a function that is templated over a few colormasks. New formats can be added with little code)
  • Scalers report how far "off the screen" they read. For instance, for a given pixel P, 2xSAI reads all pixels in the source image marked X. It would report that it reads 2 extra pixels in any direction.
    O|X|X|O
    X|P|X|X
    X|X|X|X
    O|X|X|O
  • PM2x and Edge2x/3x scalers are added.
  • Normal4x and AdvMame4x are added.
  • An optional extension to eliminate producing duplicate pixels is added. It is not mandatory for scalers or backends.
  • Graphics managers deriving SurfaceSdlGraphicsManager can be less complicated since the code handles the enabled graphics modes by default.

Current Issues:

  • Breaks some backends.
  • Edge scalers have bad performance, especially in debug builds (although the extension can make it bearable).
  • Downscaling and arbitrary scaling are not handled.

I've been playing around with subclassing SurfaceSdlGraphicsManager to handle 1/2 downscaling using some addCustomGraphicsMode member fuction. Let me know if you have any thought on this matter.

singron added some commits Apr 25, 2012

GRAPHICS/BASE: Adds definitions for scaler plugins
This includes a class for plugin implementations to inherit from and
code necessary for plugin managers to use scalers
GRAPHICS: Adds a nearest neighbor plugin
It's not used yet, but it compiles and is managed by the plugin manager.
BACKENDS: Changes scaler code to use plugin.
Much of the existing scaler code still exists and interferes.
This will have bugs
@sev-

This comment has been minimized.

Copy link

commented on graphics/scalerplugin.h in 631bba5 May 24, 2012

Please add doxygen comments to these methods

@sev-

This comment has been minimized.

Copy link

commented on graphics/scaler/normal.cpp in 631bba5 May 24, 2012

wouldn't it be better to move these to the .h file?

@sev-

This comment has been minimized.

Code formatting

@lordhoto

This comment has been minimized.

Copy link

commented on e23cc3e May 31, 2012

There some special case for the Aloss value on Mac OS X at line 465, this might apply here too. So I guess it would be safe to duplicate this check here or factor it out into a convertSDLPixelFormat function (if there's none already).

*(uint16 *)dstPtr = kMouseColorKey;
dstPtr += 2;
dstPtr += bytesPerPixel;
}
}

// Draw from [1,1] since AdvMame2x adds artefact at 0,0

This comment has been minimized.

Copy link
@lordhoto

lordhoto Aug 20, 2012

Member

This comment can be updated too.

}
}
#else
Normal1x<uint16>(srcPtr, srcPitch, dstPtr, dstPitch, width, height);

This comment has been minimized.

Copy link
@fuzzie

fuzzie Aug 21, 2012

Member

Why doesn't this case (no USE_SCALERS) check the bytesPerPixel?

@fuzzie

This comment has been minimized.

Copy link
Member

commented Aug 21, 2012

The dummy specializations in intern.h (like interpolate32_14_1_1) are empty functions in release builds, and give me a bunch of warnings about not returning a value.

@singron

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2012

fuzzie, I could not reproduce the warnings with g++ or clang++ using "./configure --enable-release". Did you use any extra or different options? I have added some return values anyway, but I would like to know so that I can detect these warnings myself.

@fuzzie

This comment has been minimized.

Copy link
Member

commented Aug 22, 2012

You probably need to manually define NDEBUG. If I try just plain g++ 4.7 on Debian with release/optimizations disabled (where presumably it doesn't optimize away the non-asserting version) then I get the same warnings, though. Thanks for the commits!

@lordhoto

This comment has been minimized.

Copy link
Member

commented Sep 20, 2012

So how's progress on the GFX_HALF issue?

@lordhoto

This comment has been minimized.

Copy link
Member

commented Oct 1, 2012

I have made some improvements for this over here: https://github.com/lordhoto/scummvm/commits/gsoc2012-scalers-cont It's still WIP though.

@sev-

This comment has been minimized.

Copy link
Member

commented May 6, 2013

LordHoto, any progress? Since 1.6.0 got branched, maybe it would be good to merge it and hammer out to the production quality?

@lordhoto

This comment has been minimized.

Copy link
Member

commented May 6, 2013

My current state of work is still at the branch. I'm not sure whether we should merge it and fix it in tree though. At least the WinCE backend will be broken, IIRC also Symbian. If someone has the time to finish this, then I don't mind someone taking it over and also cleaning up the rest of the code. I myself certainly don't have time for this right now.

I think a rough TODO list in no specific order is still:

  • Finish/Test 32 bit support
  • Clean up the code
  • Fix WinCE (i.e. check whether now everything we do in WinCE is possible with the new API and then continue from there)
  • Fix Symbian(?)
  • Go through all the SDL Surface output changes again and see whether nothing was missed/done wrong.
  • More testing
@sev-

This comment has been minimized.

Copy link
Member

commented Apr 27, 2014

No progress in 2 years. Sigh.

Is it possible to make these plugins optional, and let Symbian and WinCE ports (which a fairly dead anyway) to use Normal scalers only?

@digitall

This comment has been minimized.

Copy link
Member

commented Apr 27, 2014

@sev- : Actually I have progressed this in a basic way to stop bitrot.

I added an "other" buildset to the buildbot for testing Pull Requests and other complex changes prior to merge and producing early testing builds:
scummvm/scummvm-sites@85b2664

I used this to test Lordhoto's continued branch and finding that it was broken on more than Symbian and WinCE, I looked at patching a few basic errors which were causing breakage on some of the other ports.

@digitall

This comment has been minimized.

Copy link
Member

commented Apr 27, 2014

My branch with these minor corrective patches is here:
https://github.com/digitall/scummvm/tree/gsoc2012-scalers-cont

And the buildbot "other" builder is currently pointed there:
scummvm/scummvm-sites@5bc2f13

@digitall

This comment has been minimized.

Copy link
Member

commented Apr 27, 2014

Looking at the current buildbot status for this:
http://buildbot.scummvm.org/waterfall?category=changes;category=other

Apart from WinCE and Symbian, there are also compilation breakages in the GPH backend, thus breaking GP2X/GP2XWiz and Caanoo and Dingux, along with the Moto backend.

However, most of these are probably easily fixable and adaptable to the API changes... though this is pushing my capabilities. Any patches on my branch to fix this would be gratefully received.

@digitall

This comment has been minimized.

Copy link
Member

commented Apr 27, 2014

Most of the failures are related to the removal of the normal1x default scaler and the _scalerProc pointer for setting the active scaler IIRC i.e.
../../src-other/src/backends/graphics/gph/gph-graphics.cpp: In member function 'virtual void GPHGraphicsManager::setGraphicsModeIntern()':
../../src-other/src/backends/graphics/gph/gph-graphics.cpp:90: error: 'Normal1x' was not declared in this scope
../../src-other/src/backends/graphics/gph/gph-graphics.cpp💯 error: '_scalerProc' was not declared in this scope
../../src-other/src/backends/graphics/gph/gph-graphics.cpp: In member function 'virtual void GPHGraphicsManager::internUpdateScreen()':
../../src-other/src/backends/graphics/gph/gph-graphics.cpp:305: error: '_scalerProc' was not declared in this scope
../../src-other/src/backends/graphics/gph/gph-graphics.cpp:312: error: 'Normal1x' was not declared in this scope

Note that I have informed @djwillis about the GPH backend failures here a while back, but I haven't got a patch back from him as he is probably busy IRL :/

@digitall

This comment has been minimized.

Copy link
Member

commented Apr 27, 2014

As @lordhoto indicated, just getting these to compile is not the end of this as the TODOs he indicated will need addressing and then these could do with testing across the ports at least for basic operation.

To aid this, we already have builds for testing from the ports which do compile. See here:
http://buildbot.scummvm.org/snapshots/other/

@RichieSams RichieSams force-pushed the scummvm:master branch from 807d18e to 2ec3adf Sep 12, 2014

@digitall digitall force-pushed the scummvm:master branch from 2208eda to 5b5a2bc Oct 26, 2014

@sev-

This comment has been minimized.

Copy link
Member

commented Feb 12, 2016

Why not update it and merge in 1.9.0 and let the porters fix it (if they're still active).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.