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

No textures and depth issues on Raspberry Pi 3 (GLES 2) #1665

Open
dankcushions opened this issue Nov 20, 2017 · 84 comments
Open

No textures and depth issues on Raspberry Pi 3 (GLES 2) #1665

dankcushions opened this issue Nov 20, 2017 · 84 comments

Comments

@dankcushions
Copy link
Contributor

I presume it's to do with the shader cache changes but haven't had a chance to bisect yet. Is this RPI specific or all GLES2?

@fzurita
Copy link
Contributor

fzurita commented Nov 20, 2017

I have several devices where GLES 2.0 still works correctly. My gut guess would be that it's the YUV shader changes that are breaking the RPI. Second possibility would be the shader "performance improvement" that we had a little while back.

@cptbananas
Copy link

Dunno if this adds any insight - but when loading, a message "INIT NOISE TEXTURES" appears, which did not appear before these glitches started happening.

also - issue started no earlier than this month (nov 2017)

@gonetz
Copy link
Owner

gonetz commented Nov 21, 2017

Quite recent. Have you tried to enable legacy blending?

@fzurita
Copy link
Contributor

fzurita commented Nov 21, 2017

@dankcushions do you have access to the GLideN64 log file? Do you see any errors there?

@dankcushions
Copy link
Contributor Author

Have you tried to enable legacy blending?

i already had it enabled.

do you have access to the GLideN64 log file? Do you see any errors there?

i will check these out tonight!

@cptbananas
Copy link

A user on rpi forum asserts the following regarding log file in association with this issue:

"the glide log shows these two lines constantly repeating: Async color buffer copies are not supported on GLES2
LOD emulation not possible on this device"

@loganmc10
Copy link
Contributor

I don't have my Raspberry Pi anymore but I'll try and get the GLES2 version running on my laptop to see if I can reproduce this tomorrow

@cptbananas those 2 warnings are not related to this issue, it is just warning about a couple features that aren't supported on the Raspberry Pi

@fzurita
Copy link
Contributor

fzurita commented Nov 22, 2017

@loganmc10 I have tried it in a couple GLES 2.0 devices and I didn't have this issue. This may be RPI specific, maybe their GPU drivers specific.

@loganmc10
Copy link
Contributor

Probably the case, the GPU drivers on the Raspberry Pi are garbage. On Linux I can actually disable certain extensions so I'll try and "simulate" the RPI as close as possible, but it may be a driver bug

@gizmo98
Copy link
Contributor

gizmo98 commented Nov 22, 2017

I have no problem with vc4 mesa driver. So the vc4 firmware blob has a problem or a recent change is not compatible with VC hacks/macros. I will bisect if i have some time.

@loganmc10
Copy link
Contributor

Ah ok good to know, does RetroPie have plans to start using the Mesa driver?

@joolswills
Copy link
Contributor

As an option, but not yet.

@johnmanko
Copy link

@fzurita
Copy link
Contributor

fzurita commented Nov 23, 2017

If anyone wants to try to bisect this, here are 6 commits to check:

  • Before shader optimization: a2a96f3
  • After shader optimization: 231463e
  • Before Intel fixes: 5455604
  • After Intel fixes: a380782
  • Before YUV color space conversion: 77ffe61
  • After YUV color space conversion: adc2b54

@psyke83
Copy link
Contributor

psyke83 commented Nov 28, 2017

This seems be be the offending commit causing the plugin to fail on Pi: c5c0fbe

@fzurita
Copy link
Contributor

fzurita commented Nov 28, 2017

That's a good find. That would imply that the issue should be fixed if you disable shader storage. Have you tried that?

@Metalwario64
Copy link

Metalwario64 commented Nov 29, 2017

I've just checked mupen64plus.cfg, and EnableShadersStorage was already set to "False". If you were referring to something else, then I apologize.

@fzurita
Copy link
Contributor

fzurita commented Nov 29, 2017

Yeah, that's what I meant. It may take some debugging work an actual device to see what's wrong.

It seems to work fine in other GLES 2.0 devices that don't support shader storage.

@psyke83
Copy link
Contributor

psyke83 commented Nov 29, 2017

Looking at bad commit, I see a possible inconsistency in shader cache behaviour - it tries to load/save the cache depending on whether the feature is supported, without looking at whether we want the feature to be used via the current configuration. This patch fixes the segfault and doesn't show any graphical distortion when applied to c5c0fbe:

diff --git a/src/Graphics/OpenGLContext/GLSL/glsl_ShaderStorage.cpp b/src/Graphics/OpenGLContext/GLSL/glsl_ShaderStorage.cpp
index 821bd3b..58869b8 100644
--- a/src/Graphics/OpenGLContext/GLSL/glsl_ShaderStorage.cpp
+++ b/src/Graphics/OpenGLContext/GLSL/glsl_ShaderStorage.cpp
@@ -114,7 +114,7 @@ bool ShaderStorage::saveShadersStorage(const graphics::Combiners & _combiners) c
        if (!_saveCombinerKeys(_combiners))
                return false;

-       if (!gfxContext.isSupported(graphics::SpecialFeatures::ShaderProgramBinary))
+       if (!CombinerInfo::get().isShaderCacheSupported())
                // Shaders storage is not supported, but we saved combiners keys.
                return true;

@@ -263,7 +263,7 @@ bool ShaderStorage::_loadFromCombinerKeys(graphics::Combiners & _combiners)
        if (opengl::Utils::isGLError())
                return false;

-       if (gfxContext.isSupported(graphics::SpecialFeatures::ShaderProgramBinary))
+       if (CombinerInfo::get().isShaderCacheSupported())
                // Restore shaders storage
                return saveShadersStorage(_combiners);

@@ -273,7 +273,7 @@ bool ShaderStorage::_loadFromCombinerKeys(graphics::Combiners & _combiners)

 bool ShaderStorage::loadShadersStorage(graphics::Combiners & _combiners)
 {
-       if (!gfxContext.isSupported(graphics::SpecialFeatures::ShaderProgramBinary))
+       if (!CombinerInfo::get().isShaderCacheSupported())
                // Shaders storage is not supported, load from combiners keys.
                return _loadFromCombinerKeys(_combiners);

However, applying this against the latest master commit doesn't resolve the issue of missing textures.

@psyke83
Copy link
Contributor

psyke83 commented Nov 29, 2017

Update: the above issue with the shader cache may not be directly related to this issue (but perhaps needs to be fixed also?). That commit introduced a segmentation fault.

This commit is actually what introduced corrupted textures: 77ffe61
More debugging is needed, as it can't be cleanly reverted.

@gonetz
Copy link
Owner

gonetz commented Nov 29, 2017

77ffe61 Can't introduce corrupted textures. It splits gDPInfo::OtherMode::textureConvert 3bit field on 3 1bit fields. gDPInfo::OtherMode::textureConvert was not used for rendering, and this commit does not change the situation.

Next commit, adc2b54 do use these bits. @fzurita tested in on GLES2 devices, and this commit was included to master.
adc2b54 introduces new shader code. This code includes conversions from float to int and back and probably it causes problem on RPI. From the other side, this new code YUV_Convert is rarely used. Most of games should never call it. You may try to force disable any calls of it:
in glsl_CombinerProgramBuilder.cpp, in ShaderFragmentReadTex0 change

"  if (uBiLerp[0] != 0)																\n"
"    readtex0 = readTex(uTex0, vTexCoord0, uFbMonochrome[0], uFbFixedAlpha[0]);		\n"
"  else																		\n"
"    readtex0 = YUV_Convert(uTex0, vTexCoord0, uTextureConvert[0], uTextureFormat[0], readtex0);	\n"

by
" readtex0 = readTex(uTex0, vTexCoord0, uFbMonochrome[0], uFbFixedAlpha[0]); \n"

Do the same for ShaderFragmentReadTex1

@psyke83
Copy link
Contributor

psyke83 commented Nov 29, 2017

@gonetz

You're right - adc2b54 is the actual commit causing the problem. I misidentified it due to the shader cache from the previous build being loaded each time.

Unfortunately, your proposed change was not enough to fix the texture corruption - there's no change to the texture corruption after applying this change - and I made sure to delete the shader cache before testing.

diff --git a/src/Graphics/OpenGLContext/GLSL/glsl_CombinerProgramBuilder.cpp b/src/Graphics/OpenGLContext/GLSL/glsl_CombinerProgramBuilder.cpp
index ae8c2e7..19d314d 100644
--- a/src/Graphics/OpenGLContext/GLSL/glsl_CombinerProgramBuilder.cpp
+++ b/src/Graphics/OpenGLContext/GLSL/glsl_CombinerProgramBuilder.cpp
@@ -1167,10 +1167,7 @@ public:
                        m_part =
                                "  nCurrentTile = 0; \n"
                                "  lowp vec4 readtex0;                                                                                                                          \n"
-                               "  if (uBiLerp[0] != 0)                                                                                                                         \n"
-                               "    readtex0 = readTex(uTex0, vTexCoord0, uFbMonochrome[0], uFbFixedAlpha[0]);         \n"
-                               "  else                                                                                                                                                         \n"
-                               "    readtex0 = YUV_Convert(uTex0, vTexCoord0, uTextureConvert[0], uTextureFormat[0], readtex0);        \n"
+                               " readtex0 = readTex(uTex0, vTexCoord0, uFbMonochrome[0], uFbFixedAlpha[0]); \n"
                                ;
                } else {
                        if (config.video.multisampling > 0) {
@@ -1205,10 +1202,7 @@ public:
                        m_part =
                                "  nCurrentTile = 1; \n"
                                "  lowp vec4 readtex1;                                                                                                                          \n"
-                               "  if (uBiLerp[1] != 0)                                                                                                                         \n"
-                               "    readtex1 = readTex(uTex1, vTexCoord1, uFbMonochrome[1], uFbFixedAlpha[1]);         \n"
-                               "  else                                                                                                                                                         \n"
-                               "    readtex1 = YUV_Convert(uTex1, vTexCoord1, uTextureConvert[1], uTextureFormat[1], readtex0);        \n"
+                               " readtex1 = readTex(uTex1, vTexCoord1, uFbMonochrome[1], uFbFixedAlpha[1]); \n"
                                ;
                } else {
                        if (config.video.multisampling > 0) {

I tried applying this change against the head of this commit, and current master - doesn't help in either case.

@gonetz
Copy link
Owner

gonetz commented Nov 29, 2017

@psyke83 Hmm. readTex function changed too by this commit. Previously it used either hardware bilinear filtering or shader filter3point. Later I added shader for bilinear filtering. May be it causes the issue. Try to revert readTex to old state: in class ShaderReadtex : public ShaderPart

m_part =	"lowp vec4 readTex(in sampler2D tex, in mediump vec2 texCoord, in lowp int fbMonochrome, in lowp int fbFixedAlpha)	\n"
"{																			\n"
"  lowp vec4 texColor = texture2D(tex, texCoord); 							\n"
"  if (fbMonochrome == 1) texColor = vec4(texColor.r);						\n"
"  else if (fbMonochrome == 2) 												\n"
"    texColor.rgb = vec3(dot(vec3(0.2126, 0.7152, 0.0722), texColor.rgb));	\n"
"  if (fbFixedAlpha == 1) texColor.a = 0.825;								\n"
"  return texColor;															\n"
"}																			\n"
;

If TextureFilter is really the problem, you will get textures, but only unfiltered, because hardware biliner filtering is disabled.

@psyke83
Copy link
Contributor

psyke83 commented Nov 29, 2017

Unfortunately, I see no visual difference when readTex is changed - with or without the previous change that forces readTex in instead of of YUV_Convert.

P.S. I noticed the attribution in the comments of that commit to twinaphex; that's interesting, as I think that lr-mupen64plus may have similar graphical corruption when the gliden64 option is selected in the core options. I don't have it installed this moment to confirm for certain though.

@gonetz
Copy link
Owner

gonetz commented Nov 29, 2017

@psyke83
Ok

  • Stash your changes or put them to a commit.
  • Switch to 77ffe61. Rebuild everything. Be sure that everything works as before. It will be our starting point. Backup Textures.cpp somewhere.
  • Switch back to your changes. Replace Textures.cpp by the one from backup. That way you will revert almost all changes made in adc2b54 If it will not help - I have no more ideas what can be wrong.

If it will help - need to carefuly check what was changed in Textures.cpp and return these changes step by step until you will find what breaks texturing.

@psyke83
Copy link
Contributor

psyke83 commented Nov 29, 2017

I won't have time to return to testing for a few hours, but so far, this is what gets textures to display when applied against adc2b54 or master:

diff --git a/src/Graphics/OpenGLContext/GLSL/glsl_CombinerProgramBuilder.cpp b/src/Graphics/OpenGLContext/GLSL/glsl_CombinerProgramBuilder.cpp
index 69ec065..39f00a3 100644
--- a/src/Graphics/OpenGLContext/GLSL/glsl_CombinerProgramBuilder.cpp
+++ b/src/Graphics/OpenGLContext/GLSL/glsl_CombinerProgramBuilder.cpp
@@ -947,7 +947,7 @@ public:
 				"  name = vec4(iconvert)/255.0;										\\\n"
 				"  }																\n"
 				;
-			if (config.texture.bilinearMode == BILINEAR_3POINT) {
+			if (1) {
 				// 3 point texture filtering.
 				// Original author: ArthurCarvalho
 				// GLSL implementation: twinaphex, mupen64plus-libretro project.
@@ -1102,10 +1102,7 @@ public:
 			m_part =
 				"  nCurrentTile = 0; \n"
 				"  lowp vec4 readtex0;																\n"
-				"  if (uBiLerp[0] != 0)																\n"
 				"    readtex0 = readTex(uTex0, vTexCoord0, uFbMonochrome[0], uFbFixedAlpha[0]);		\n"
-				"  else																				\n"
-				"    readtex0 = YUV_Convert(uTex0, vTexCoord0, uTextureConvert[0], uTextureFormat[0], readtex0);	\n"
 				;
 		} else {
 			if (config.video.multisampling > 0) {
@@ -1140,10 +1137,7 @@ public:
 			m_part =
 				"  nCurrentTile = 1; \n"
 				"  lowp vec4 readtex1;																\n"
-				"  if (uBiLerp[1] != 0)																\n"
 				"    readtex1 = readTex(uTex1, vTexCoord1, uFbMonochrome[1], uFbFixedAlpha[1]);		\n"
-				"  else																				\n"
-				"    readtex1 = YUV_Convert(uTex1, vTexCoord1, uTextureConvert[1], uTextureFormat[1], readtex0);	\n"
 				;
 		} else {
 			if (config.video.multisampling > 0) {
@@ -1537,7 +1531,7 @@ public:
 				"  return vec4(iconvert)/255.0;										\n"
 				"  }																\n"
 			;
-			if (config.texture.bilinearMode == BILINEAR_3POINT) {
+			if (1) {
 				m_part +=
 					"uniform mediump vec2 uTextureSize[2];										\n"
 					// 3 point texture filtering.
@@ -1591,9 +1585,7 @@ public:
 			m_part +=
 					"lowp vec4 readTex(in sampler2D tex, in mediump vec2 texCoord, in lowp int fbMonochrome, in lowp int fbFixedAlpha)	\n"
 					"{																			\n"
-					"  lowp vec4 texColor;														\n"
-					"  if (uTextureFilterMode == 0) texColor = texture2D(tex, texCoord);		\n"
-					"  else texColor = TextureFilter(tex, texCoord);							\n"
+					"  lowp vec4 texColor = texture2D(tex, texCoord); 							\n"
 					"  if (fbMonochrome == 1) texColor = vec4(texColor.r);						\n"
 					"  else if (fbMonochrome == 2) 												\n"
 					"    texColor.rgb = vec3(dot(vec3(0.2126, 0.7152, 0.0722), texColor.rgb));	\n"

Using TextureFilter in readTex() as you earlier suggested didn't work, and for some reason, the standard bilinear filtering code path needed to be forced, or else the texture problem persisted.

P.S. YUV_Convert does cause issues, but for the test case (Super Mario 64), it only seems to affect a few minor elements such as the star transition effect, missing "PRESS START", incorrect shadow rendering on characters. The majority of textures do display if that code is left in.

@fzurita
Copy link
Contributor

fzurita commented Nov 29, 2017

Are you able to view GLSL compiler errors generated by the driver?

@loganmc10
Copy link
Contributor

They should get printed out into gliden64.log I believe

@psyke83
Copy link
Contributor

psyke83 commented Nov 29, 2017

Ah, I figured it printed debug to stdout. These lines are repeated in my log:

$ cat /home/pi/.local/share/mupen64plus/gliden64.log | sort | uniq
Async color buffer copies are not supported on GLES2
LOD emulation not possible on this device
OpenGL Error: invalid operation (502)Async color buffer copies are not supported on GLES2
ShaderCacheSupported?Async color buffer copies are not supported on GLES2

@fzurita
Copy link
Contributor

fzurita commented Dec 14, 2017

@psyke83 After looking closely at commit fzurita@f71d714 I don't see why it's necessary.

The plugin should be able to generate a bunch of shaders from the keys file just fine even in GLES 2.0 where shader storage is not supported. Your back trace is crashing in a strange spot that doesn't make much sense.

It's crashing specifically in this line:

_combiners[pCombiner->getKey()] = pCombiner; in glsl_ShaderStorage.cpp. Somehow, either the CombinerKey object is not valid or the map is unable to generate a temporary CombineryKey object with an empty constructor.

@gonetz
Copy link
Owner

gonetz commented Dec 14, 2017

Hmm,
CombinerKey getKey() const override {return CombinerKey();}
looks like architectural error. It can cause troubles in shader storage
_combiners[pCombiner->getKey()] = pCombiner;
I'll clear this code.

I already forgot my own code :(
CombinerProgramImpl::getKey() returns correct key.
"Somehow, either the CombinerKey object is not valid or the map is unable to generate a temporary CombineryKey object with an empty constructor." - may be this is the case. Better change getKey() to avoid that possibility.

@gonetz
Copy link
Owner

gonetz commented Dec 14, 2017

Please test again from ea00991

@psyke83
Copy link
Contributor

psyke83 commented Dec 14, 2017

No difference, unfortunately. Perhaps this is an issue with GCC 6 on Pi, as I'm using Raspbian stretch & noticed some build warnings related to the CombinerKey function.

Here's the build log: https://gist.github.com/psyke83/d3a06a1bd3d06e52cb45b930d3039c4a

@gonetz
Copy link
Owner

gonetz commented Dec 14, 2017

I see no warnings. There are some notes:
note: parameter passing for argument bla-bla-bla will change in GCC 7.1
You may ignore that.

@gonetz
Copy link
Owner

gonetz commented Dec 14, 2017

If std::map::operator[] does not work right with your compiler, you may replace
_combiners[pCombiner->getKey()] = pCombiner;
by
_combiners.insert(std::pair<CombinerKey, graphics::CombinerProgram *>(pCombiner->getKey(), pCombiner));

@psyke83
Copy link
Contributor

psyke83 commented Dec 15, 2017

That appears to have worked. I'm no longer seeing any segmentation faults upon successive emulator launches, and the GLideN64.xxx.GLES.keys files are being created properly in the shader cache folder. Thanks!

@fzurita
Copy link
Contributor

fzurita commented Dec 15, 2017

@gonetz that is very interesting, that should work the same...

@gonetz
Copy link
Owner

gonetz commented Dec 15, 2017

std::map::insert

Extends the container by inserting new elements, effectively increasing the container size by the number of elements inserted.

Because element keys in a map are unique, the insertion operation checks whether each inserted element has a key equivalent to the one of an element already in the container, and if so, the element is not inserted, returning an iterator to this existing element (if the function returns a value).
std::map::operator[]

If k matches the key of an element in the container, the function returns a reference to its mapped value.

If k does not match the key of any element in the container, the function inserts a new element with that key and returns a reference to its mapped value. Notice that this always increases the container size by one, even if no mapped value is assigned to the element (the element is constructed using its default constructor).

@fzurita Do you see the difference? insert(pair(key,value)) updates map only if there is no element with this key yet. map[key] = value always updates the map: either insert new element or updates existing one.

I agree, that for GLideN64 code it must be the same because the code does m_combiners.find(key) first.
@psyke83 could you test return value of insert? that is:

auto res = _combiners.insert(std::pair<CombinerKey, graphics::CombinerProgram *>(pCombiner->getKey(), pCombiner));
if (!res.second)
  LOG(LOG_ERROR, "Attempt to insert duplicated shader with key: %lx\n", m_pCurrent->getKey().getMux());

@johnmanko
Copy link

johnmanko commented Dec 15, 2017

deleted - answered my own question. :)

@psyke83
Copy link
Contributor

psyke83 commented Dec 16, 2017

@gonetz,

I tried adding the logging, but no relevant logging appears in the gliden64.log when launching Majora's Mask.

Notes:

  • I had to substitute m_pCurrent with pCombiners in your snippet above, as the former is not declared in the scope of src/Graphics/OpenGLContext/GLSL/glsl_ShaderStorage.cpp;
  • Removing the !res.second check (just to ensure it will log non-duplicates at all) shows:
Attempt to insert duplicated shader with key: 150c937f
Attempt to insert duplicated shader with key: 15fc9378
Attempt to insert duplicated shader with key: 1ffc93f8
Attempt to insert duplicated shader with key: 350ce37f
Attempt to insert duplicated shader with key: 350cf37f
Attempt to insert duplicated shader with key: 35fcf378
Attempt to insert duplicated shader with key: 35fcff7e
Attempt to insert duplicated shader with key: 3f1677ff
Attempt to insert duplicated shader with key: ff0f92ff
Attempt to insert duplicated shader with key: ff0f93ff
Attempt to insert duplicated shader with key: ff17f3ff
Attempt to insert duplicated shader with key: ff2dfeff
Attempt to insert duplicated shader with key: ff2fffff
Attempt to insert duplicated shader with key: fffcf279
Attempt to insert duplicated shader with key: fffcf3f8
Attempt to insert duplicated shader with key: fffdf638
Attempt to insert duplicated shader with key: fffdf6fb
Attempt to insert duplicated shader with key: fffdfe38
Attempt to insert duplicated shader with key: fffdfff8
Attempt to insert duplicated shader with key: fffe7638
Attempt to insert duplicated shader with key: fffe793c
Attempt to insert duplicated shader with key: fffff238
Attempt to insert duplicated shader with key: fffff3f8
Attempt to insert duplicated shader with key: fffff7f8
Attempt to insert duplicated shader with key: fffffbf8
Attempt to insert duplicated shader with key: fffffbfd
Attempt to insert duplicated shader with key: fffffe38
Attempt to insert duplicated shader with key: fffffff8
  • Here's the actual contents of the generated GLideN64.1f7c6dac.GLES.keys file (Majora's Mask):
0x00000004
0x00000000
0x00000021
0x1812fe25fffffbfd
0x18ff97ffff2dfeff
0x19119623ff2fffff
0x19fffffffffdf6fb
0x19fffffffffe793c
0x1a119604fffffff8
0x1a1197fffffffe38
0x1a11fe04fffff3f8
0x1a11fe04fffff7f8
0x1a11fffffffff238
0x1a127e60ff17f3ff
0x1a127e60fffff3f8
0x1a127e60fffffbf8
0x1a127ea0fffff3f8
0x1a127ffffffff238
0x1a12fe043f1677ff
0x1a167e6035fcf378
0x1a167e6035fcff7e
0x1a209c03ff0f93ff
0x1a20ac04ff0f92ff
0x1a20ac04ff0f93ff
0x1a25266015fc9378
0x1a262a60150c937f
0x1a26a0041ffc93f8
0x1aff9604fffdfff8
0x1aff97fffffdfe38
0x1afffe60fffcf3f8
0x1afffffffffdf638
0x1afffffffffe7638
0x1b267e60350cf37f
0x1b272c60350ce37f
0x1dfffffffffcf279
0x1ffffffffffe793c

@gonetz
Copy link
Owner

gonetz commented Dec 16, 2017

@psyke83

Attempt to insert duplicated shader with key: 150c937f

I set wrong format for 64bit integer. Right code:

		auto res = m_combiners.insert(std::pair<CombinerKey, graphics::CombinerProgram *>(m_pCurrent->getKey(), m_pCurrent));
		if (!res.second)
			LOG(LOG_ERROR, "Attempt to insert duplicated shader with key: 0x%llx\n", m_pCurrent->getKey().getMux());

I put this code instead of
m_combiners[m_pCurrent->getKey()] = m_pCurrent;

I removed old files in shaders folder and run Zelda MM intro with mupen64plus. Shaders storage option enabled. No "duplicated shader" errors in log file.
I stopped emulation and removed GLideN64.e5af6c41.OpenGL.shaders to force load shaders from .key file. I run the game again, and again there is no "duplicated shader" errors in log file.

Here's the actual contents of the generated GLideN64.1f7c6dac.GLES.keys file

It is correct. I got the same content.

So, on my system the code works as expected. I suggest you to take latest master,
replace
m_combiners[m_pCurrent->getKey()] = m_pCurrent;
by

		auto res = m_combiners.insert(std::pair<CombinerKey, graphics::CombinerProgram *>(m_pCurrent->getKey(), m_pCurrent));
		if (!res.second)
			LOG(LOG_ERROR, "Attempt to insert duplicated shader with key: 0x%llx\n", m_pCurrent->getKey().getMux());

make clear rebuild, clear mupen64plus shaders folder, remove old gliden64.log and run again.

If you will reproduce the problem after that, something is bad with gcc you are using. My only guess is that std::map implementation is broken.

@psyke83
Copy link
Contributor

psyke83 commented Dec 16, 2017

Testing against your latest master branch at commit 509ee7e (as with my previous test), and I still can't trigger any duplicate logs.

My testing procedure:

  • Rebuild library with patch (below).
  • Remove shader cache & log.
  • Launch emulator, wait for Zelda MM intro to play & then exit.
  • Stop emulator, observe logs/files (not expecting duplicate errors yet; but, keep in mind that only the .keys file is generated on Pi even with EnableShaderStorage=true set, presumably due to hardware limitations):
$ cat /home/pi/.local/share/mupen64plus/gliden64.log
Async color buffer copies are not supported on GLES2
LOD emulation not possible on this device
$ ls /home/pi/.cache/mupen64plus/shaders/
GLideN64.1f7c6dac.GLES.keys
  • Remove log but leave shader cache untouched.
  • Re-launch emulator and wait for Zelda MM intro, then exit.
  • No duplicate shader logging appears in log:
$ cat /home/pi/.local/share/mupen64plus/gliden64.log
Async color buffer copies are not supported on GLES2
LOD emulation not possible on this device

Complete diff:

diff --git a/src/Combiner.cpp b/src/Combiner.cpp
index f724cc0..5b3380b 100644
--- a/src/Combiner.cpp
+++ b/src/Combiner.cpp
@@ -11,6 +11,7 @@
 #include "PluginAPI.h"
 #include "RSP.h"
 #include "Graphics/Context.h"
+#include "Log.h"

 using namespace graphics;

@@ -295,7 +296,9 @@ void CombinerInfo::setCombine(u64 _mux )
        } else {
                m_pCurrent = Combiner_Compile(key);
                m_pCurrent->update(true);
-               m_combiners[m_pCurrent->getKey()] = m_pCurrent;
+               auto res = m_combiners.insert(std::pair<CombinerKey, graphics::CombinerProgram *>(m_pCurrent->getKey(), m_pCurrent));
+               if (!res.second)
+                       LOG(LOG_ERROR, "Attempt to insert duplicated shader with key: 0x%llx\n", m_pCurrent->getKey().getMux());
        }
        m_bChanged = true;
 }

This is the gcc version that I'm using which is the default on Raspbian stretch (all packages up to date):

$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/arm-linux-gnueabihf/6/lto-wrapper
Target: arm-linux-gnueabihf
Configured with: ../src/configure -v --with-pkgversion='Raspbian 6.3.0-18+rpi1' --with-bugurl=file:///usr/share/doc/gcc-6/README.Bugs --enable-languages=c,ada,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-6 --program-prefix=arm-linux-gnueabihf- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-libitm --disable-libquadmath --enable-plugin --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-6-armhf/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-6-armhf --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-6-armhf --with-arch-directory=arm --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --with-target-system-zlib --enable-objc-gc=auto --enable-multiarch --disable-sjlj-exceptions --with-arch=armv6 --with-fpu=vfp --with-float=hard --enable-checking=release --build=arm-linux-gnueabihf --host=arm-linux-gnueabihf --target=arm-linux-gnueabihf
Thread model: posix
gcc version 6.3.0 20170516 (Raspbian 6.3.0-18+rpi1)

I'll try installing & setting gcc-4.9 as the default and see if a full rebuild remedies the issue. I'll also try reverting to jessie entirely if the stretch gcc 4.9 exhibits the same issue. Thanks for the assistance and I'll keep you updated.

@fzurita
Copy link
Contributor

fzurita commented Dec 16, 2017

Maybe it's some kind of memory corruption issue. The Valgrind program would give a clue if it is. I'll try running it against the latest master.

@psyke83
Copy link
Contributor

psyke83 commented Dec 20, 2017

Just to confirm, rebuilding just the GlideN64 plugin against stretch's gcc 4.9 resolves the segmentation fault with the shader cache, so @gonetz's guess about gcc 6 being bad is likely to be true. I'll try to do more research and see if it's a problem specific to gcc 6.3.0 or perhaps Raspbian's changes against upstream.

@fzurita,
I can't get valgrind to launch mupen64plus correctly, but then again, even if I try to test simple binaries such as "ls", valgrind hangs with a lot of repeating errors such as:

pi@retropie-stretch:~/RetroPie-Setup $ valgrind ls
==6481== Memcheck, a memory error detector
==6481== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==6481== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info
==6481== Command: ls
==6481==
--6481-- WARNING: Serious error when reading debug info
--6481-- When reading debug info from /lib/arm-linux-gnueabihf/ld-2.24.so:
--6481-- Ignoring non-Dwarf2/3/4 block in .debug_info
--6481-- WARNING: Serious error when reading debug info
--6481-- When reading debug info from /lib/arm-linux-gnueabihf/ld-2.24.so:
--6481-- Last block truncated in .debug_info; ignoring
==6481== Conditional jump or move depends on uninitialised value(s)
==6481==    at 0x401B230: index (in /lib/arm-linux-gnueabihf/ld-2.24.so)
==6481==
==6481== Conditional jump or move depends on uninitialised value(s)
==6481==    at 0x401B234: index (in /lib/arm-linux-gnueabihf/ld-2.24.so)
==6481==
==6481== Conditional jump or move depends on uninitialised value(s)
==6481==    at 0x4008B8C: expand_dynamic_string_token (in /lib/arm-linux-gnueabihf/ld-2.24.so)
==6481==
==6481== Conditional jump or move depends on uninitialised value(s)
==6481==    at 0x401B6E0: strlen (in /lib/arm-linux-gnueabihf/ld-2.24.so)
==6481==
==6481== Conditional jump or move depends on uninitialised value(s)
==6481==    at 0x401B6E4: strlen (in /lib/arm-linux-gnueabihf/ld-2.24.so)
==6481==
---
==6481== Use of uninitialised value of size 4
==6481==    at 0x401C194: memcpy (in /lib/arm-linux-gnueabihf/ld-2.24.so)
==6481==
==6481== Use of uninitialised value of size 4
==6481==    at 0x401C16C: memcpy (in /lib/arm-linux-gnueabihf/ld-2.24.so)
==6481==
==6481== Use of uninitialised value of size 4
==6481==    at 0x401C410: memcpy (in /lib/arm-linux-gnueabihf/ld-2.24.so)
==6481==

I have to kill -9 the valgrind process as it doesn't respond to control codes or a regular KILL signal. Perhaps I'll revert back to jessie to see if valgrind has the same behaviour.

@fzurita
Copy link
Contributor

fzurita commented Dec 20, 2017

Valgrind is very intensive. It's possible that the Raspberry pi just can't keep up.

@psyke83
Copy link
Contributor

psyke83 commented Feb 26, 2018

Following up on our previous discussion - I haven't been able to get valgrind working on the Raspberry Pi or get much further in debugging the issue.

Would this change possibly be superior to using the std::map::insert? It also avoids the crash on the Pi:

diff --git a/src/Combiner.cpp b/src/Combiner.cpp
index f724cc0..0141f6c 100644
--- a/src/Combiner.cpp
+++ b/src/Combiner.cpp
@@ -295,7 +295,7 @@ void CombinerInfo::setCombine(u64 _mux )
        } else {
                m_pCurrent = Combiner_Compile(key);
                m_pCurrent->update(true);
-               m_combiners[m_pCurrent->getKey()] = m_pCurrent;
+               m_combiners.emplace(m_pCurrent->getKey(), m_pCurrent);
        }
        m_bChanged = true;
 }

The next release of RetroPie will be based on Raspbian stretch, so if you prefer not to move away from std::map::operator[], I can propose patching this in the RetroPie build system - but you're going to get bug reports from non-RetroPie Pi users, as new Pi firmware updates are only being released for stretch and more people will be using this distribution version soon.

psyke83 added a commit to psyke83/RetroPie-Setup that referenced this issue Mar 16, 2018
Crash seems reproducible only on gcc 6.3.0 via Rasbian stretch, but the
workaround should not negatively impact other systems.

See: gonetz/GLideN64#1665
joolswills pushed a commit to joolswills/RetroPie-Setup that referenced this issue Apr 7, 2018
joolswills pushed a commit to joolswills/RetroPie-Setup that referenced this issue Apr 7, 2018
Crash seems reproducible only on gcc 6.3.0 via Rasbian stretch, but the
workaround should not negatively impact other systems.

See: gonetz/GLideN64#1665
@paradadf
Copy link
Contributor

Is this still and issue? Sorry, I can‘t compile it to test it myself.

@loganmc10
Copy link
Contributor

The Raspberry Pi still has an ugly hack in place to fix some depth issues.

https://github.com/gonetz/GLideN64/blob/master/src/Graphics/OpenGLContext/opengl_ContextImpl.cpp#L315-L319

Someone could try removing that code and see if there are still issues. Maybe that workaround isn't needed anymore

@gizmo98
Copy link
Contributor

gizmo98 commented Apr 25, 2018

@loganmc10
Verified. The ugly fix is still necessary.

pjft pushed a commit to pjft/RetroPie-Setup that referenced this issue Mar 5, 2019
pjft pushed a commit to pjft/RetroPie-Setup that referenced this issue Mar 5, 2019
Crash seems reproducible only on gcc 6.3.0 via Rasbian stretch, but the
workaround should not negatively impact other systems.

See: gonetz/GLideN64#1665
rodvieirasilva pushed a commit to rodvieirasilva/RetroPie-Setup that referenced this issue Mar 29, 2019
rodvieirasilva pushed a commit to rodvieirasilva/RetroPie-Setup that referenced this issue Mar 29, 2019
Crash seems reproducible only on gcc 6.3.0 via Rasbian stretch, but the
workaround should not negatively impact other systems.

See: gonetz/GLideN64#1665
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests