[Cogl] [PATCH] cogl-gst: Smooth out issues with layering code

[Neil Roberts]

Thanks for the patches.

I think it would be good to try and avoid duplicating the shader code
and to try to simplify the API a bit. It might be better to split the
function out into three like this:

void
cogl_gst_video_sink_set_first_layer (CoglGstVideoSink *sink,
                                     int first_layer);

void
cogl_gst_video_sink_set_default_sample (CoglGstVideoSink *sink,
                                        CoglBool default_sample);

void
cogl_gst_video_sink_setup_pipeline (CoglGstVideoSink *sink,
                                    CoglPipeline *pipeline);

I think we should separate out the operations into separate functions
like this because setting the custom start layer actually also affects
the default pipeline that you would get via
cogl_gst_video_sink_get_pipeline and otherwise it's not clear from the
API that this would happen. It should also be useful and possible to
modify the first layer for this default pipeline so it would be good to
make the API support that.

I think we don't need the ‘previous_layer’ parameter because we can just
make it so that it always modulates with the layer that comes before
first_layer. If the application wants to do anything more complicated
than that then it should just add its own layer on the end instead of
the default sampling code.

Currently the default sampling snippet code in CoglGST sets a replace
string to avoid redundantly generating texture sampling code for the
other layers. That would also stop it modulating with any previous
layers by default. We can fix this however by just making it set the
combine mode for all of its layers to REPLACE(PREVIOUS). I'm about to
attach a patch which does this.

I can't think of a reason why an application would need to set its own
name for the sampling function, so we can probably do without the
‘conversion_name’ parameter.

In order to avoid duplicating the shader code in two places, we can
change the interface for the renderers and replace the ‘init’ function
with something like this:

 void (* setup_pipeline) (CoglGstVideoSink *sink,
                          CoglPipeline *pipeline);

That way the cogl_gst_video_sink_setup_pipeline function can just call
this virtual function. cogl_gst_video_sink_get_pipeline can also use
this by just creating an empty pipeline to pass to it.

The code which generates the shader strings with g_strconcat could be
moved into the virtual setup_pipeline functions instead of being a long
if-statement. We could just drop the code in cogl-gst-shader in that
case.

It would be great if you could make this snippet generation work with
the SnippetCache struct. Perhaps you could store the generated snippets
in a linked list with along the first layer that was used for that
snippet so that it can avoid redundantly creating a new snippet if the
same first_layer has been used already.

There is a problem with the say you are using g_strconcat.
g_strdup_printf returns a newly allocated string which you are then
passing directly to g_strconcat. g_strconcat will make a copy of that
data so the original string returned by g_strdup_printf will be leaked.
However it looks like you are only using g_strconcat to avoid making a
really long line of code. In C you can split string constants by just
writing two string literals next to each other with no operator in
between like this:

 src = g_strdup_printf ("vec4 %s (vec2 UV) {\n"
                        "  return texture2D (video_sampler%i, UV);\n"
                        "}\n",
                        conversion_name,
                        start);

That is the same as if you wrote one single long string literal. The
concatenation is done during compilation so there are no extra strings
to leak.

We should remove the ‘deinit’ virtual from the renderer vtable because
none of them are using it and it's not clear what it's supposed to do.

Regards,
- Neil
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.