[Cogl] [PATCH] Include CoglGst

[Neil Roberts]

This is looking really cool, thanks for the patch! I have a few comments
which I noted while reading through it. I don't have that much
experience with GStreamer so it's mostly just commenting on the Cogl
parts. It might be good to get a GStreamer expert (maybe Damien) to look
through it too.

There doesn't seem to be any way to destroy a CoglGstVideoPlayer. We
should probably convert this to either a CoglObject or a GObject. I
think a GObject would probably make more sense.

The GLSL shaders are using the gl_* names for the builtins. This won't
work on GLES2 or GL3. Instead it should be using the portable cogl_*
names like cogl_color_out. That also means you can use the same bit of
code for the vertex and fragment shader because in that case the name
for the incoming texture coordinate is the same in both cases
(cogl_tex_coord0_in). There is a list of the names in the documentation:

http://is.gd/tw21Vz

I was expecting that instead of directly writing to gl_FragColor from
the shaders that the sink adds to the pipeline, it would just declare a
function like cogl_sample_video(tex2D coord). That would return the
colour for the video at the given location. The application would then be
free to call that function from its own snippet and sample from random
locations. CoglGstVideoSink can also then add a default snippet to the
last layer that it creates which just calls this function and puts the
result in cogl_color_out. The application is then free to replace that
snippet by adding a subsequent layer which replaces that the previous
snippet.

One problem with that approach currently is that if a subsequent layer
replaces a previous layer then the code for the previous layer won't
actually be generated which would mean the cogl_sample_video() function
wouldn't actually get declared. A solution we discussed previously would
be to add a snippet hook location that is intended just for global
declarations, perhaps called COGL_SNIPPET_HOOK_FRAGMENT_DECLARATIONS.
The code for that snippet would always get generated and only the
declarations section would be used. We can worry about that for a
separate patch though I guess.

If we do that then we could probably get away without having a property
for the hook location in CoglGstVideoSink. The sink could just always
add the function to both the vertex shader and the fragment shader and
we could rely on the compiler to trim out the code if the application
isn't actually using it.

The names for the variables containing the declarations for the snippet
code are called *_pre. This is a bit confusing because they are attached
to the declarations section of the snippet rather than the ‘pre’
section. There is also a section called ‘pre’ but the code for that
doesn't go in the global scope so it isn't appropriate for those
declarations.

It would be good if the example didn't just constantly redraw on idle.
Is there any way to get a notification from GStreamer when the new frame
is ready? It would be good if it could redraw only after a new frame is
ready and it has also received a sync notification from the last frame.
That would mean using cogl_onscreen_add_frame_callback().

It doesn't seem right that cogl-gst-video-player is directly dumping
errors to stderr. Maybe we should just leave that up to the
application and not bother listening for bus messages? Or at the very
least it should use g_warning or similar instead.

Is it using GStreamer 0.10 instead of 1.0? I thought Clutter-GST had
been updated to GStreamer 1?

cogl_gst_init() doesn't look like it's doing the right thing. If it is
just a wrapper around gst_init, maybe we should just leave it out and
let people init GST themselves? If we do want to keep the function it
should at least call g_error instead of fprintf and abort so that the
error message will go to the right location.

The private symbols in cogl-gst-video-player-private.h and
cogl-gst-video-sink-private.h need to be prefixed with an underscore.
Any symbols that begin with ‘cogl_gst_’ will be exported out of the
shared library and we want to avoid that for private symbols.

It is a bit strange to have a static variable in the header. Perhaps
cogl_gst_seek_flags should just be a #define?

Instead of calling free(), you should be calling g_free for memory
allocated by GStreamer because it will potentially use a different
allocation mechanism (although in practice this will probably never
happen).
• cogl-gst-video-player.c:52
• cogl-basic-video-player.c:34

cogl-gst-shaders.h probably shouldn't be installed, should it? The
GLSL code should be an internal implementation detail and should leak
out the API. Maybe should even be in a .c file instead of a .h file?

There's a missing backslash on one of the arguments to
libcogl_gst_la_LDFLAGS in the Makefile.am. Presumably this was copied
from a mistake in the Makefile.am for CoglPango.

Where a cogl-gst-*.h header is included it should be #include
<cogl-gst/cogl-gst-blah.h> instead of #include
<cogl/cogl-gst/cogl-gst-blah.h>. Otherwise I think it won't work when
it is included from an external application and it also breaks
out-of-tree builds.

In the cogl-basic-video example, please try to put ‘static’ in front
of the declarations that are internal to the source. Otherwise it will
generate a warning.

We can't use C99 features in most of the Cogl code because we support
compiling on the totally broken Visual Studio compiler. In
cogl-basic-video.c the GMainLoop* variable is declared in the middle
of the code. This is a C99 feature and it generates a warning.

There are tabs in the patch and it looks like you have set your tab
size to 2. Unfortunately that means in everyone else's editor the code
looks very broken. We will need to replace all of the tabs with 2
spaces.

It would be good to have some documentation. In particular it would be
nice to have a description of the expected usage model for adding your
own customisation to the sink's pipeline. We could leave that until a
separate patch once the API is finalised though I guess.

Sorry for the long email!

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.