From: Greg A. <st...@ga...> - 2013-03-23 05:31:20
|
Hi - I checked out the bazaar repository, rev 5924, and built it and discovered two separate issues that caused Stellarium to crash on startup on my computer. I'm new to Stellarium development (and to Bazaar) so I hope I'm not doing this wrong... First, the new solar system shadows are enabled by default, but if they are initialized without StelRenderer::areFloatTexturesSupported() then that triggers an assert failure in StelRenderer::createTexture(void*,QSize,TextureDataFormat,TextureParams&). I believe I saw this before other users because I am using unaccelerated (software rendered) MesaGL. My proposed remedy is to disable solar system shadows (regardless of the config parameter) if float textures are not supported. Second, StelQGLGLSLShader.uniformStorage can be unaligned, both because its start position may be unaligned and because it may contain members (such as bool) which would introduce misalignment in subsequent members. I believe I saw this before other users because I am running on ARM, which gives a fatal SIGBUS for unaligned memory access. My proposed remedy is to enforce a minimum alignment in uniformStorage corresponding to sizeof(void*), which should be harmless on most platforms, a slight performance improvement on some, and the difference between crashing-or-not on a few. Note that I use the GCC __attribute__((aligned(n))) extension. I noticed that the similar GCC extension __attribute__((packed)) was already used in Stellarium, so hopefully that is not forbidden... As an aside, I am working on the development branch of Stellarium because I am attempting to improve the performance on my new laptop that does not have accelerated GL drivers. At first glance, the new build from Bazaar appears to be about 400% slower than the 0.11.4 that I had been working with, so that is where my focus will be if I have any future contributions. And thanks -- Stellarium is simply the best!! Even at 1fps, I find I can't live without it. Patches below... Cheers, - Greg === modified file 'src/core/modules/SolarSystem.cpp' --- src/core/modules/SolarSystem.cpp 2013-02-22 11:43:47 +0000 +++ src/core/modules/SolarSystem.cpp 2013-03-23 03:45:48 +0000 @@ -1035,7 +1035,7 @@ sharedPlanetGraphics.lazyInit(renderer); - if(StelApp::getInstance().getRenderSolarShadows() && sharedPlanetGraphics.shadowPlanetShader) + if(StelApp::getInstance().getRenderSolarShadows() && sharedPlanetGraphics.shadowPlanetShader && renderer->areFloatTexturesSupported()) { StelTextureNew* shadowInfo = computeShadowInfo(renderer); === modified file 'src/core/renderer/StelQGLGLSLShader.cpp' --- src/core/renderer/StelQGLGLSLShader.cpp 2012-09-17 18:12:57 +0000 +++ src/core/renderer/StelQGLGLSLShader.cpp 2013-03-23 04:31:25 +0000 @@ -22,9 +22,17 @@ #include <stdint.h> +#define ALIGN_UNIFORM_SIZE(x) (((x)+(UNIFORM_ALIGNMENT-1)) & ~(UNIFORM_ALIGNMENT-1)) + int StelQGLGLSLShader::UNIFORM_SIZES[UniformType_max] = - {0, sizeof(float), sizeof(Vec2f), sizeof(Vec3f), - sizeof(Vec4f), sizeof(Mat4f), sizeof(bool), sizeof(int)}; + {0, + ALIGN_UNIFORM_SIZE(sizeof(float)), + ALIGN_UNIFORM_SIZE(sizeof(Vec2f)), + ALIGN_UNIFORM_SIZE(sizeof(Vec3f)), + ALIGN_UNIFORM_SIZE(sizeof(Vec4f)), + ALIGN_UNIFORM_SIZE(sizeof(Mat4f)), + ALIGN_UNIFORM_SIZE(sizeof(bool)), + ALIGN_UNIFORM_SIZE(sizeof(int))}; StelQGLGLSLShader::StelQGLGLSLShader(StelQGL2Renderer* renderer, bool internal) : StelGLSLShader() === modified file 'src/core/renderer/StelQGLGLSLShader.hpp' --- src/core/renderer/StelQGLGLSLShader.hpp 2012-09-17 02:26:58 +0000 +++ src/core/renderer/StelQGLGLSLShader.hpp 2013-03-23 04:30:47 +0000 @@ -32,6 +32,11 @@ //! Used to allow pointer casts to/from 2D float array for matrix uniform storage/upload. typedef float RAW_GL_MATRIX [4][4]; +// Minimum alignment for uniform members, both for efficiency and to +// avoid SIGBUS on some platforms (like ARM). +#define UNIFORM_ALIGNMENT sizeof(void*) + + //! QGL based StelGLSLShader implementation, used by the QGL2 renderer backend. //! //! @note This is an internal class of the Renderer subsystem and should not be used elsewhere. @@ -394,7 +399,7 @@ //! The data is then uploaded by uploadUniforms(). //! //! @see uploadUniforms, clearUniforms, pushUniformStorage - unsigned char uniformStorage [UNIFORM_STORAGE]; + unsigned char uniformStorage [UNIFORM_STORAGE] __attribute__((aligned(UNIFORM_ALIGNMENT))); //! Pointer to uniformStorage[uniformStorageUsed]. Avoids breaking strict aliasing. void* uniformStoragePointer; |
From: Alexander W. <ale...@gm...> - 2013-03-23 06:34:40
|
Hi! 2013/3/23 Greg Alexander <st...@ga...> > I checked out the bazaar repository, rev 5924, and built it and > discovered two separate issues that caused Stellarium to crash on startup > on my computer. I'm new to Stellarium development (and to Bazaar) so I > hope I'm not doing this wrong... > I think this was you on IRC :) > > First, the new solar system shadows are enabled by default, but if they > are initialized without StelRenderer::areFloatTexturesSupported() then > that triggers an assert failure in > StelRenderer::createTexture(void*,QSize,TextureDataFormat,TextureParams&). > I believe I saw this before other users because I am using unaccelerated > (software rendered) MesaGL. My proposed remedy is to disable solar system > shadows (regardless of the config parameter) if float textures are > not supported. > ok. I'm apply this patch - it's a logical and nothing broken :) > > Second, StelQGLGLSLShader.uniformStorage can be unaligned, both because > its start position may be unaligned and because it may contain members > (such as bool) which would introduce misalignment in subsequent members. > I believe I saw this before other users because I am running on ARM, > which gives a fatal SIGBUS for unaligned memory access. My proposed > remedy is to enforce a minimum alignment in uniformStorage corresponding > to sizeof(void*), which should be harmless on most platforms, a slight > performance improvement on some, and the difference between > crashing-or-not on a few. Note that I use the GCC > This patch works for the unaccelerated mode (GL1) but it broken the accelerated mode (GL2). > __attribute__((aligned(n))) extension. I noticed that the similar GCC > extension __attribute__((packed)) was already used in Stellarium, so > hopefully that is not forbidden... > Well, you should use #ifdef defined(__GNUC__) for GCC extension because Stellarium build via clang too. :) > As an aside, I am working on the development branch of Stellarium because > I am attempting to improve the performance on my new laptop that does not > have accelerated GL drivers. At first glance, the new build from Bazaar > appears to be about 400% slower than the 0.11.4 that I had been working > with, so that is where my focus will be if I have any future > contributions. > You can register at launchpad and create a branch for Stellarium - I think this will be better way for coding and applies patches. > And thanks -- Stellarium is simply the best!! Even at 1fps, I find I > can't live without it. > Thanks! -- With best regards, Alexander |
From: Ferdinand M. <kii...@gm...> - 2013-03-23 17:15:26
|
Hi, Thanks for the alignment fixes. I have no experience on ARM so I didn't take that into account. As for the GCC extension: you could also align manually by creating a bigger array and increasing pointer to the nearest greater multiple of alignment size, which doesn't depend on compiler. I'm not sure if that's guaranteed to be safe everywhere, though. Don't have time right now, but will check later. FM On 3/23/13, Alexander Wolf <ale...@gm...> wrote: > Hi! > > 2013/3/23 Greg Alexander <st...@ga...> > >> I checked out the bazaar repository, rev 5924, and built it and >> discovered two separate issues that caused Stellarium to crash on startup >> on my computer. I'm new to Stellarium development (and to Bazaar) so I >> hope I'm not doing this wrong... >> > > I think this was you on IRC :) > > >> >> First, the new solar system shadows are enabled by default, but if they >> are initialized without StelRenderer::areFloatTexturesSupported() then >> that triggers an assert failure in >> StelRenderer::createTexture(void*,QSize,TextureDataFormat,TextureParams&). >> I believe I saw this before other users because I am using unaccelerated >> (software rendered) MesaGL. My proposed remedy is to disable solar >> system >> shadows (regardless of the config parameter) if float textures are >> not supported. >> > > ok. I'm apply this patch - it's a logical and nothing broken :) > > >> >> Second, StelQGLGLSLShader.uniformStorage can be unaligned, both because >> its start position may be unaligned and because it may contain members >> (such as bool) which would introduce misalignment in subsequent members. >> I believe I saw this before other users because I am running on ARM, >> which gives a fatal SIGBUS for unaligned memory access. My proposed >> remedy is to enforce a minimum alignment in uniformStorage corresponding >> to sizeof(void*), which should be harmless on most platforms, a slight >> performance improvement on some, and the difference between >> crashing-or-not on a few. Note that I use the GCC >> > > This patch works for the unaccelerated mode (GL1) but it broken the > accelerated mode (GL2). > > > >> __attribute__((aligned(n))) extension. I noticed that the similar GCC >> extension __attribute__((packed)) was already used in Stellarium, so >> hopefully that is not forbidden... >> > > Well, you should use #ifdef defined(__GNUC__) for GCC extension because > Stellarium build via clang too. :) > > >> As an aside, I am working on the development branch of Stellarium because >> I am attempting to improve the performance on my new laptop that does not >> have accelerated GL drivers. At first glance, the new build from Bazaar >> appears to be about 400% slower than the 0.11.4 that I had been working >> with, so that is where my focus will be if I have any future >> contributions. >> > > You can register at launchpad and create a branch for Stellarium - I think > this will be better way for coding and applies patches. > > >> And thanks -- Stellarium is simply the best!! Even at 1fps, I find I >> can't live without it. >> > > Thanks! > > -- > With best regards, Alexander > |
From: Greg A. <st...@ga...> - 2013-03-23 20:42:33
|
Hi - Thanks for the response. I seem to always be walking away from the computer at the wrong time to interact on IRC. :) As for the alignment patch...I just noticed, the setUniformValue_() functions use "+= sizeof(type)", instead of referring to the UNIFORM_SIZES array, so my previous patch was broken. Is there another issue you are aware of that broke GL2 for you? Anyways, we can avoid the gcc extension by defining uniformStorage[] as a smaller array of elements of typed void*, which should be aligned appropriately everywhere. As an aside, I do not think the use of uniformStoragePointer is necessary under strict aliasing rules. The rule says that if you are executing a read of a given type at a given address, then the compiler may assume the value was written as the same type (which is followed in this code). The strict aliasing rule is not concerned with the type of the variable, only the type of the memory accesses. Plus I'm a factoring junky. :) So for the sake of conversation, I put this patch in lp:~launchp-t/stellarium/slowcomputer (rev 5938) It might be overkill. It is definitely more correct than my previous patch. If there is a situation I have not found where uniformStorage needs to be packed (for example if it is passed directly as a single unit), then it is wrong. I certainly won't be offended Cheers, - Greg On Sat, Mar 23, 2013 at 01:34:33PM +0700, Alexander Wolf wrote: > Hi! > > 2013/3/23 Greg Alexander <st...@ga...> > > > I checked out the bazaar repository, rev 5924, and built it and > > discovered two separate issues that caused Stellarium to crash on startup > > on my computer. I'm new to Stellarium development (and to Bazaar) so I > > hope I'm not doing this wrong... > > > > I think this was you on IRC :) > > > > > > First, the new solar system shadows are enabled by default, but if they > > are initialized without StelRenderer::areFloatTexturesSupported() then > > that triggers an assert failure in > > StelRenderer::createTexture(void*,QSize,TextureDataFormat,TextureParams&). > > I believe I saw this before other users because I am using unaccelerated > > (software rendered) MesaGL. My proposed remedy is to disable solar system > > shadows (regardless of the config parameter) if float textures are > > not supported. > > > > ok. I'm apply this patch - it's a logical and nothing broken :) > > > > > > Second, StelQGLGLSLShader.uniformStorage can be unaligned, both because > > its start position may be unaligned and because it may contain members > > (such as bool) which would introduce misalignment in subsequent members. > > I believe I saw this before other users because I am running on ARM, > > which gives a fatal SIGBUS for unaligned memory access. My proposed > > remedy is to enforce a minimum alignment in uniformStorage corresponding > > to sizeof(void*), which should be harmless on most platforms, a slight > > performance improvement on some, and the difference between > > crashing-or-not on a few. Note that I use the GCC > > > > This patch works for the unaccelerated mode (GL1) but it broken the > accelerated mode (GL2). > > > > > __attribute__((aligned(n))) extension. I noticed that the similar GCC > > extension __attribute__((packed)) was already used in Stellarium, so > > hopefully that is not forbidden... > > > > Well, you should use #ifdef defined(__GNUC__) for GCC extension because > Stellarium build via clang too. :) > > > > As an aside, I am working on the development branch of Stellarium because > > I am attempting to improve the performance on my new laptop that does not > > have accelerated GL drivers. At first glance, the new build from Bazaar > > appears to be about 400% slower than the 0.11.4 that I had been working > > with, so that is where my focus will be if I have any future > > contributions. > > > > You can register at launchpad and create a branch for Stellarium - I think > this will be better way for coding and applies patches. > > > > And thanks -- Stellarium is simply the best!! Even at 1fps, I find I > > can't live without it. > > > > Thanks! > > -- > With best regards, Alexander > ------------------------------------------------------------------------------ > Everyone hates slow websites. So do we. > Make your web apps faster with AppDynamics > Download AppDynamics Lite for free today: > http://p.sf.net/sfu/appdyn_d2d_mar > _______________________________________________ > Stellarium-pubdevel mailing list > Ste...@li... > https://lists.sourceforge.net/lists/listinfo/stellarium-pubdevel |