search

libretro / mgba

mGBA Game Boy Advance Emulator
https://mgba.io/
Mozilla Public License 2.0
68 stars 73 forks source link

Add optional colour correction #150

Closed jdgleaver closed 5 years ago

jdgleaver commented 5 years ago

This PR adds optional colour correction, based upon Pokefan531's latest shader pack: https://forums.libretro.com/t/real-gba-and-ds-phat-colors/1540/174

It adds a new core option Color Correction, with four settings;

Here are some examples:

Shantae (USA) (GBC)-190613-141951

Shantae (USA) (GBC)-190613-142002

Wario Land II (USA, Europe) (SGB Enhanced) (GBC)-190613-142202

Wario Land II (USA, Europe) (SGB Enhanced) (GBC)-190613-142212

Golden Sun (USA, Europe)-190613-142422

Golden Sun (USA, Europe)-190613-142431

Wario Land 4 (USA, Europe)-190613-142334

Wario Land 4 (USA, Europe)-190613-142355

Notes:

andres-asm commented 5 years ago

does upstream have color correction? if it does wouldn't it be better to tap into that?

On Thu, Jun 13, 2019 at 8:56 AM jdgleaver notifications@github.com wrote:

This PR adds optional colour correction, based upon Pokefan531's latest shader pack: https://forums.libretro.com/t/real-gba-and-ds-phat-colors/1540/174

It adds a new core option Color Correction, with four settings;

-

OFF

GBA: Force GBA colour correction

GBC: Force GBC colour correction

Auto: Apply GBA colour correction when running GBA titles, GBC colour correction when running GBC titles, and disable colour correction for GB/SGB content (i.e. always do the correct thing!)

Here are some examples:

  • Color Correction: OFF

[image: Shantae (USA) (GBC)-190613-141951] https://user-images.githubusercontent.com/38211560/59437103-d2330980-8de8-11e9-9bd9-f3f7541d4f2f.png

  • Color Correction: Auto

[image: Shantae (USA) (GBC)-190613-142002] https://user-images.githubusercontent.com/38211560/59437111-d8c18100-8de8-11e9-9915-56f6cf8fcea0.png

  • Color Correction: OFF

[image: Wario Land II (USA, Europe) (SGB Enhanced) (GBC)-190613-142202] https://user-images.githubusercontent.com/38211560/59437173-f5f64f80-8de8-11e9-8ddc-85ed57ee21d2.png

  • Color Correction: Auto

[image: Wario Land II (USA, Europe) (SGB Enhanced) (GBC)-190613-142212] https://user-images.githubusercontent.com/38211560/59437190-fbec3080-8de8-11e9-9e12-c74ecd77c7c1.png

  • Color Correction: OFF

[image: Golden Sun (USA, Europe)-190613-142422] https://user-images.githubusercontent.com/38211560/59437227-0e666a00-8de9-11e9-96a6-84d7eb8e4d3d.png

  • Color Correction: Auto

[image: Golden Sun (USA, Europe)-190613-142431] https://user-images.githubusercontent.com/38211560/59437237-17573b80-8de9-11e9-92e2-41c010eac163.png

  • Color Correction: OFF

[image: Wario Land 4 (USA, Europe)-190613-142334] https://user-images.githubusercontent.com/38211560/59437140-e6770680-8de8-11e9-9a8d-fd8579ac346a.png

  • Color Correction: Auto

[image: Wario Land 4 (USA, Europe)-190613-142355] https://user-images.githubusercontent.com/38211560/59437154-eb3bba80-8de8-11e9-8d42-e6f9b2c224f2.png

Notes:

-

This is done purely as a post-processing stage inside libretro.c - i.e. it doesn't interfere with any upstream code that would affect the stand-alone versions.

It's implemented using a big old look-up table. This is wasteful of RAM (~128 kb) and requires a setup stage (~100 ms), but it means that the actual real-time colour conversion has a very small performance impact. (Also, when colour correction is disabled, no extra resources are used)

While GBC colour correction is essentially perfect, rounding errors mean that internal GBA colour correction will never be as accurate as when using the gba-color.glsl GPU shader. However: most users shouldn't notice the difference, and even these (slightly) inaccurate results look great, and are an order of magnitude better than uncorrected colours. Moreover, this feature predominantly targets platforms without shader support (such as the Wii), where playing GBA titles is currently a rather oversaturated/garish experience.

You can view, comment on, or merge this pull request online at:

https://github.com/libretro/mgba/pull/150 Commit Summary

  • Add optional colour correction

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/libretro/mgba/pull/150?email_source=notifications&email_token=AANEFUGQ2NCI2VRX66FLMILP2JGYBA5CNFSM4HXZ3TEKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GZKJXXA, or mute the thread https://github.com/notifications/unsubscribe-auth/AANEFUCPNUKCCBMM72W3MXDP2JGYBANCNFSM4HXZ3TEA .

inactive123 commented 5 years ago

It's reasonably self contained in libretro.c so it's not like this touches any surface area in the upstream core emulator code, and in case upstream has it, it would have to be carefully weighed if this implementation is faster than it. Performance matters in libretro cores, whether they are upstream or not, especially for older consoles, and libretro cores should strive for the best performance possible.

rz5 commented 5 years ago

@fr500 - mGBA on desktop does have an OpenGL multipass shader pipeline, if that's what you mean?

inactive123 commented 5 years ago

@rz5 But what jdgleaver implemented here aren't shaders, and the likes of 3DS doesn't have pixel shader support anyway, neither does Wii.

inactive123 commented 5 years ago

I don't see anything that is in upstream that covers this, and even if there is, this is likely to be more performant.

So unless I see evidence to the contrary, I see no reason to further stall this PR. Merging it.

endrift commented 5 years ago

Why did you remove frameskip, but not the setting for frameskip?

inactive123 commented 5 years ago

@endrift Yeah that might be worthwhile to add back. I'll bring this to @jdgleaver's attention.

jdgleaver commented 5 years ago

@endrift I didn't remove frameskip :)

The frameskip config code was originally duplicated - I just removed the redundant copies. I didn't think it was worth mentioning in the PR comment, since it was only a bit of housekeeping.

If you look at master, you'll see that frameskip is still present and correct:

https://github.com/libretro/mgba/blob/525d6538b036a38e3c744e10afd73c3910cdbe61/src/platform/libretro/libretro.c#L367-L371

https://github.com/libretro/mgba/blob/525d6538b036a38e3c744e10afd73c3910cdbe61/src/platform/libretro/libretro.c#L633-L638

endrift commented 5 years ago

Oh, huh, I wonder how it ended up getting duplicated.