[Cogl] [PATCH 2/2] Fix removing layers when the pipeline is not the owner

[Robert Bragg]

Nice catch!

This looks good to land to me.

Reviewed-by: Robert Bragg <robert at linux.intel.com>

thanks,
- Robert

On Thu, May 24, 2012 at 12:54 PM, Neil Roberts <neil at linux.intel.com> wrote:
> If cogl_pipeline_remove_layer is called on a copied pipeline to remove
> a parent layer then it will still end up calling
> _cogl_pipeline_remove_layer_difference on the layer. This function
> was directly trying to remove the layer from the pipeline's list of
> layer differences. However in the child pipeline the layer isn't in
> the list because it is unchanged from its parent. The function had an
> assertion to verify that this situation wasn't hit so in a debug build
> it would just bail out.
>
> This patch removes the assertion and changes it to only remove the
> layer if it is owned by the pipeline. Otherwise it just sets the
> COGL_PIPELINE_STATE_LAYERS difference as normal and decrements the
> number of layers. This will cause it to successfully remove the layer
> because either it is the last layer in which case it will be ignored
> after n_layers is decreased or if it is in the middle of the list then
> the subsequent layers will all be shifted down so there will be a
> replacement layer difference.
> ---
>  cogl/cogl-pipeline.c              |   22 +++++++++++++++-------
>  tests/conform/test-conform-main.c |    2 +-
>  2 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/cogl/cogl-pipeline.c b/cogl/cogl-pipeline.c
> index ef38df5..0c2dc9d 100644
> --- a/cogl/cogl-pipeline.c
> +++ b/cogl/cogl-pipeline.c
> @@ -1377,8 +1377,6 @@ _cogl_pipeline_remove_layer_difference (CoglPipeline *pipeline,
>                                         CoglPipelineLayer *layer,
>                                         CoglBool dec_n_layers)
>  {
> -  _COGL_RETURN_IF_FAIL (layer->owner == pipeline);
> -
>   /* - Flush journal primitives referencing the current state.
>    * - Make sure the pipeline has no dependants so it may be modified.
>    * - If the pipeline isn't currently an authority for the state being
> @@ -1394,13 +1392,23 @@ _cogl_pipeline_remove_layer_difference (CoglPipeline *pipeline,
>                                     NULL,
>                                     !dec_n_layers);
>
> -  layer->owner = NULL;
> -  cogl_object_unref (layer);
> +  /* We only need to remove the layer difference if the pipeline is
> +   * currently the owner. If it is not the owner then one of two
> +   * things will happen to make sure this layer is replaced. If it is
> +   * the last layer being removed then decrementing n_layers will
> +   * ensure that the last layer is skipped. If it is any other layer
> +   * then the subsequent layers will have been shifted down and cause
> +   * it be replaced */
> +  if (layer->owner == pipeline)
> +    {
> +      layer->owner = NULL;
> +      cogl_object_unref (layer);
>
> -  pipeline->differences |= COGL_PIPELINE_STATE_LAYERS;
> +      pipeline->layer_differences =
> +        g_list_remove (pipeline->layer_differences, layer);
> +    }
>
> -  pipeline->layer_differences =
> -    g_list_remove (pipeline->layer_differences, layer);
> +  pipeline->differences |= COGL_PIPELINE_STATE_LAYERS;
>
>   if (dec_n_layers)
>     pipeline->n_layers--;
> diff --git a/tests/conform/test-conform-main.c b/tests/conform/test-conform-main.c
> index 02f428f..aa2f746 100644
> --- a/tests/conform/test-conform-main.c
> +++ b/tests/conform/test-conform-main.c
> @@ -59,7 +59,7 @@ main (int argc, char **argv)
>   ADD_TEST (test_depth_test, 0);
>   ADD_TEST (test_color_mask, 0);
>   ADD_TEST (test_backface_culling, TEST_REQUIREMENT_NPOT);
> -  ADD_TEST (test_layer_remove, TEST_KNOWN_FAILURE);
> +  ADD_TEST (test_layer_remove, 0);
>
>   ADD_TEST (test_sparse_pipeline, 0);
>
> --
> 1.7.3.16.g9464b
>
> _______________________________________________
> Cogl mailing list
> Cogl at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/cogl