OpenGL: Fix lifetime issue with uniform/sampler "locs" for thin3d pipelines

Common/GPU/OpenGL/GLRenderManager.h

@@ -99,12 +99,23 @@ struct GLRProgramFlags {
 	bool useClipDistance2 : 1;
 };
 
+// Unless you manage lifetimes in some smart way,
+// your loc data for uniforms and samplers need to be in a struct
+// derived from this, and passed into CreateProgram.
+class GLRProgramLocData {
+public:
+	virtual ~GLRProgramLocData() {}
+};
+
 class GLRProgram {
 public:
 	~GLRProgram() {
 		if (program) {
 			glDeleteProgram(program);
 		}
+		if (locData_) {
+			delete locData_;
+		}
 	}
 	struct Semantic {
 		int location;
@@ -127,6 +138,8 @@ public:
 	std::vector<Semantic> semantics_;
 	std::vector<UniformLocQuery> queries_;
 	std::vector<Initializer> initialize_;
+
+	GLRProgramLocData *locData_;
 	bool use_clip_distance[8]{};
 
 	struct UniformInfo {
@@ -148,6 +161,7 @@ public:
 		}
 		return loc;
 	}
+
 	std::unordered_map<std::string, UniformInfo> uniformCache_;
 };
 
@@ -438,13 +452,14 @@ public:
 	// not be an active render pass.
 	GLRProgram *CreateProgram(
 		std::vector<GLRShader *> shaders, std::vector<GLRProgram::Semantic> semantics, std::vector<GLRProgram::UniformLocQuery> queries,
-		std::vector<GLRProgram::Initializer> initializers, const GLRProgramFlags &flags) {
+		std::vector<GLRProgram::Initializer> initializers, GLRProgramLocData *locData, const GLRProgramFlags &flags) {
 		GLRInitStep step{ GLRInitStepType::CREATE_PROGRAM };
 		_assert_(shaders.size() <= ARRAY_SIZE(step.create_program.shaders));
 		step.create_program.program = new GLRProgram();
 		step.create_program.program->semantics_ = semantics;
 		step.create_program.program->queries_ = queries;
 		step.create_program.program->initialize_ = initializers;
+		step.create_program.program->locData_ = locData;
 		step.create_program.program->use_clip_distance[0] = flags.useClipDistance0;
 		step.create_program.program->use_clip_distance[1] = flags.useClipDistance1;
 		step.create_program.program->use_clip_distance[2] = flags.useClipDistance2;

Common/GPU/OpenGL/thin3d_gl.cpp

@@ -261,6 +261,11 @@ bool OpenGLShaderModule::Compile(GLRenderManager *render, ShaderLanguage languag
 	return true;
 }
 
+struct PipelineLocData : GLRProgramLocData {
+	GLint samplerLocs_[MAX_TEXTURE_SLOTS]{};
+	std::vector<GLint> dynamicUniformLocs_;
+};
+
 class OpenGLInputLayout : public InputLayout {
 public:
 	OpenGLInputLayout(GLRenderManager *render) : render_(render) {}
@@ -285,9 +290,10 @@ public:
 		if (program_) {
 			render_->DeleteProgram(program_);
 		}
+		// DO NOT delete locs_ here, it's deleted by the render manager.
 	}
 
-	bool LinkShaders();
+	bool LinkShaders(const PipelineDesc &desc);
 
 	GLuint prim = 0;
 	std::vector<OpenGLShaderModule *> shaders;
@@ -296,12 +302,14 @@ public:
 	AutoRef<OpenGLBlendState> blend;
 	AutoRef<OpenGLRasterState> raster;
 
+	// Not owned!
+	PipelineLocData *locs_ = nullptr;
+
 	// TODO: Optimize by getting the locations first and putting in a custom struct
 	UniformBufferDesc dynamicUniforms;
-	GLint samplerLocs_[MAX_TEXTURE_SLOTS]{};
-	std::vector<GLint> dynamicUniformLocs_;
 	GLRProgram *program_ = nullptr;
 
+
 	// Allow using other sampler names than sampler0, sampler1 etc in shaders.
 	// If not set, will default to those, though.
 	Slice<SamplerDef> samplers_;
@@ -1133,10 +1141,10 @@ Pipeline *OpenGLContext::CreateGraphicsPipeline(const PipelineDesc &desc, const
 	}
 	if (desc.uniformDesc) {
 		pipeline->dynamicUniforms = *desc.uniformDesc;
-		pipeline->dynamicUniformLocs_.resize(desc.uniformDesc->uniforms.size());
 	}
+
 	pipeline->samplers_ = desc.samplers;
-	if (pipeline->LinkShaders()) {
+	if (pipeline->LinkShaders(desc)) {
 		// Build the rest of the virtual pipeline object.
 		pipeline->prim = primToGL[(int)desc.prim];
 		pipeline->depthStencil = (OpenGLDepthStencilState *)desc.depthStencil;
@@ -1206,7 +1214,7 @@ ShaderModule *OpenGLContext::CreateShaderModule(ShaderStage stage, ShaderLanguag
 	}
 }
 
-bool OpenGLPipeline::LinkShaders() {
+bool OpenGLPipeline::LinkShaders(const PipelineDesc &desc) {
 	std::vector<GLRShader *> linkShaders;
 	for (auto shaderModule : shaders) {
 		if (shaderModule) {
@@ -1237,35 +1245,38 @@ bool OpenGLPipeline::LinkShaders() {
 	semantics.push_back({ SEM_POSITION, "a_position" });
 	semantics.push_back({ SEM_TEXCOORD0, "a_texcoord0" });
 
+	locs_ = new PipelineLocData();
+	locs_->dynamicUniformLocs_.resize(desc.uniformDesc->uniforms.size());
+
 	std::vector<GLRProgram::UniformLocQuery> queries;
 	int samplersToCheck;
 	if (!samplers_.is_empty()) {
 		for (int i = 0; i < (int)std::min((const uint32_t)samplers_.size(), MAX_TEXTURE_SLOTS); i++) {
-			queries.push_back({ &samplerLocs_[i], samplers_[i].name, true });
+			queries.push_back({ &locs_->samplerLocs_[i], samplers_[i].name, true });
 		}
 		samplersToCheck = (int)samplers_.size();
 	} else {
-		queries.push_back({ &samplerLocs_[0], "sampler0" });
-		queries.push_back({ &samplerLocs_[1], "sampler1" });
-		queries.push_back({ &samplerLocs_[2], "sampler2" });
+		queries.push_back({ &locs_->samplerLocs_[0], "sampler0" });
+		queries.push_back({ &locs_->samplerLocs_[1], "sampler1" });
+		queries.push_back({ &locs_->samplerLocs_[2], "sampler2" });
 		samplersToCheck = 3;
 	}
 
 	_assert_(queries.size() <= MAX_TEXTURE_SLOTS);
 	for (size_t i = 0; i < dynamicUniforms.uniforms.size(); ++i) {
-		queries.push_back({ &dynamicUniformLocs_[i], dynamicUniforms.uniforms[i].name });
+		queries.push_back({ &locs_->dynamicUniformLocs_[i], dynamicUniforms.uniforms[i].name });
 	}
 	std::vector<GLRProgram::Initializer> initialize;
 	for (int i = 0; i < MAX_TEXTURE_SLOTS; ++i) {
 		if (i < samplersToCheck) {
-			initialize.push_back({ &samplerLocs_[i], 0, i });
+			initialize.push_back({ &locs_->samplerLocs_[i], 0, i });
 		} else {
-			samplerLocs_[i] = -1;
+			locs_->samplerLocs_[i] = -1;
 		}
 	}
 
 	GLRProgramFlags flags{};
-	program_ = render_->CreateProgram(linkShaders, semantics, queries, initialize, flags);
+	program_ = render_->CreateProgram(linkShaders, semantics, queries, initialize, locs_, flags);
 	return true;
 }
 
@@ -1287,7 +1298,7 @@ void OpenGLContext::UpdateDynamicUniformBuffer(const void *ub, size_t size) {
 
 	for (size_t i = 0; i < curPipeline_->dynamicUniforms.uniforms.size(); ++i) {
 		const auto &uniform = curPipeline_->dynamicUniforms.uniforms[i];
-		const GLint &loc = curPipeline_->dynamicUniformLocs_[i];
+		const GLint &loc = curPipeline_->locs_->dynamicUniformLocs_[i];
 		const float *data = (const float *)((uint8_t *)ub + uniform.offset);
 		switch (uniform.type) {
 		case UniformType::FLOAT1:

GPU/GLES/ShaderManagerGLES.cpp

@@ -204,7 +204,7 @@ LinkedShader::LinkedShader(GLRenderManager *render, VShaderID VSID, Shader *vs,
 		flags.useClipDistance0 = true;
 	}
 
-	program = render->CreateProgram(shaders, semantics, queries, initialize, flags);
+	program = render->CreateProgram(shaders, semantics, queries, initialize, nullptr, flags);
 
 	// The rest, use the "dirty" mechanism.
 	dirtyUniforms = DIRTY_ALL_UNIFORMS;