After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 572767 - Should error out instead of doing nothing if an OpenGL feature is not present
Should error out instead of doing nothing if an OpenGL feature is not present
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-gl
0.10.x
Other Linux
: Normal major
: 0.10.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-02-22 18:38 UTC by Sebastian Dröge (slomo)
Modified: 2012-02-18 20:05 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Sebastian Dröge (slomo) 2009-02-22 18:38:36 UTC
Hi,
currently the OpenGL effects, etc don't error out if the framebuffer object OpenGL extension is not present but instead they do nothing. This should be changed to return GST_FLOW_ERROR from the chain function and do GST_ELEMENT_ERROR().
Comment 1 Julien Isorce 2009-08-04 20:20:46 UTC
Hi,

Those kind of checks (glewinit, framebuffer object OpenGL extension, GLSL etc..) are made in gstgldisplay.c.
I could make call gst_gl_display_new(gstelement) instead of just gst_gl_display_new().
Then I would be able to call GST_ELEMENT_ERROR (gstelement, RESOURCE, NOT_FOUND, ("no framebuffer object OpenGL extension"), (NULL)) inside gstgldisplay.c.
In this way we do not need to change anything else (no need to return GST_FLOW_ERROR, see glimagesink)

Note that in a gl flow (for example: glupload ! glfilters ! glimagesink)
the gl element that calls gst_gl_display_new is always the first one.

I think it would be enough for now, at least we could handle most of the basic checks (because actually they are handle silentely, I mean no messages are posted to the bus or no signals)

Any comments before I start the changes ?


Comment 2 Sebastian Dröge (slomo) 2009-08-05 04:45:47 UTC
GST_ELEMENT_ERROR() is not enough, you should also make sure to return an ERROR flow return from any chain function.
Comment 3 Olivier Crête 2009-08-05 04:58:16 UTC
Should'nt the null->ready state change fail instead ?
Comment 4 Julien Isorce 2009-08-05 06:49:38 UTC
here: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/sys/xvimage/xvimagesink.c

around:

/* Handle window deletion by posting an error on the bus */
GST_ELEMENT_ERROR (xvimagesink, RESOURCE, NOT_FOUND, "Output window was closed"), (NULL));

I do not see any ERROR flow return after.
Am I missing something or is it a particular case ?
Comment 5 Filippo Argiolas 2009-08-05 08:11:03 UTC
IMHO, this could be solved in the following way.

1. gldisplay should provide some kind of "feature checking" infrastructure:
  - a flag structure (GstGLFeatureFlags flags)
  - a bunch of feature checks in gldisplay init function, something like:

     if (GLEW_ARB_shading_language_100) flags |= GST_GL_FEATURE_GLSL;
     if (some more complex check) flags |= GST_GL_FEATURE_FBO;

  - some public utility method for feature checking (gst_gl_feature(s)_available?) that just check the flags structure
  - replace all "if (GLEW_stuff)" calls with custom flags checking

2. all base elements (upload, filter, download) could then check needed features (glfilter should provide a vmethod for custom subclasses dependencies) in the prepare_output_buffer function return FLOW_ERROR on fail

3. Maybe we could manually handle state changes and do the checks in NULL->READY as Olivier suggested, but 1 and 2 should be enough for now.


Note that we could just use glew everywhere but this way we could have some more complex custom check if needed and eventually get rid of glew in the future.



Comment 6 Julien Isorce 2011-11-24 15:25:07 UTC
commit ec027145586ff1cbb528ff0c77e824a8d1c4ef7b
Author: Julien Isorce <julien.isorce@gmail.com>
Date:   Thu Nov 24 16:02:32 2011 +0100

    feature checking: error out instead of doing nothing if an OpenGL feature is not present
    
    Fix bug #572767