From 82f6800442a3d9416bd129337375929dab5006af Mon Sep 17 00:00:00 2001 From: Robert Bragg Date: Wed, 8 Dec 2010 16:29:37 +0000 Subject: [PATCH] arbfp: Copy pipelines used as cache keys We are currently using a pipeline as a key into our arbfp program cache but because we weren't making a copy of the pipelines used as keys there were times when doing a lookup in the cache would end up trying to compare a lookup key with an entry key that would point to invalid memory. Note: the current approach isn't ideal from the pov that that key pipeline may reference some arbitrarily large user textures will now be kept alive indefinitely. The plan to improve on this is that we will have a mechanism to create a special "key pipeline" which will derive from the default Cogl pipeline (to avoid affecting the lifetime of other pipelines) and only copy state from the original pipeline that affects the arbfp program and will reference small dummy textures instead of potentially large user textures. --- cogl/cogl-pipeline-arbfp.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/cogl/cogl-pipeline-arbfp.c b/cogl/cogl-pipeline-arbfp.c index e5235c7d1..7a19ed52c 100644 --- a/cogl/cogl-pipeline-arbfp.c +++ b/cogl/cogl-pipeline-arbfp.c @@ -108,7 +108,11 @@ typedef struct _ArbfpProgramState { int ref_count; + /* XXX: only valid during codegen */ + CoglPipeline *arbfp_authority; + CoglHandle user_program; + /* XXX: only valid during codegen */ GString *source; GLuint gl_program; UnitState *unit_state; @@ -328,6 +332,11 @@ _cogl_pipeline_backend_arbfp_start (CoglPipeline *pipeline, "PARAM two = {2, 2, 2, 2};\n" "PARAM minus_one = {-1, -1, -1, -1};\n"); + /* At the end of code-gen we'll add the program to a cache and + * we'll use the authority pipeline as the basis for key into + * that cache... */ + arbfp_program_state->arbfp_authority = authority; + for (i = 0; i < n_layers; i++) { arbfp_program_state->unit_state[i].sampled = FALSE; @@ -928,6 +937,8 @@ _cogl_pipeline_backend_arbfp_end (CoglPipeline *pipeline, if (G_LIKELY (!(cogl_debug_flags & COGL_DEBUG_DISABLE_PROGRAM_CACHES))) { + CoglPipeline *key; + /* XXX: I wish there was a way to insert into a GHashTable * with a pre-calculated hash value since there is a cost to * calculating the hash of a CoglPipeline and in this case @@ -935,8 +946,24 @@ _cogl_pipeline_backend_arbfp_end (CoglPipeline *pipeline, * _cogl_pipeline_arbfp_backend_start so we could pass the * value through to here to avoid hashing it again. */ - g_hash_table_insert (ctx->arbfp_cache, pipeline, arbfp_program_state); + + /* XXX: Any keys referenced by the hash table need to remain + * valid all the while that there are corresponding values, + * so for now we simply make a copy of the current authority + * pipeline. + * + * FIXME: A problem with this is that our key into the cache + * may hold references to some arbitrary user textures which + * will now be kept alive indefinitly which is a shame. A + * better solution will be to derive a special "key + * pipeline" from the authority which derives from the base + * Cogl pipeline (to avoid affecting the lifetime of any + * other pipelines) and only takes a copy of the state that + * relates to the arbfp program and references small dummy + * textures instead of potentially large user textures. */ + key = cogl_pipeline_copy (arbfp_program_state->arbfp_authority); arbfp_program_state_ref (arbfp_program_state); + g_hash_table_insert (ctx->arbfp_cache, key, arbfp_program_state); if (G_UNLIKELY (g_hash_table_size (ctx->arbfp_cache) > 50)) { static gboolean seen = FALSE; @@ -947,6 +974,11 @@ _cogl_pipeline_backend_arbfp_end (CoglPipeline *pipeline, seen = TRUE; } } + + /* The authority is only valid during codegen since the program + * state may have a longer lifetime than the original authority + * it is created for. */ + arbfp_program_state->arbfp_authority = NULL; } if (arbfp_program_state->user_program != COGL_INVALID_HANDLE)