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 600195 - dynamic fragment shader filter and variables parser/loader
dynamic fragment shader filter and variables parser/loader
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-gl
git master
Other All
: Normal enhancement
: 0.10.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-10-31 03:36 UTC by Luc
Modified: 2012-02-18 20:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstglshadervariables and gstglshaderfilter (9.27 KB, application/x-gzip)
2009-10-31 03:36 UTC, Luc
  Details
test application (5.86 KB, text/x-csrc)
2009-10-31 04:04 UTC, Luc
  Details
fix some warnings, remove some useless includes, indent and build a real patch (66.17 KB, patch)
2009-11-05 14:30 UTC, Julien Isorce
none Details | Review
git diff -p --cached (51.96 KB, patch)
2010-05-16 21:58 UTC, Luc
none Details | Review
git diff -p --cached (50.55 KB, patch)
2011-06-07 02:32 UTC, Luc
none Details | Review

Description Luc 2009-10-31 03:36:41 UTC
Created attachment 146618 [details]
gstglshadervariables and gstglshaderfilter

gst/gl/gstglfiltershader.c:

Implements a plugin allowing:

- To load a simple fragment shader filter and to set uniform variables through
object properties.

- To replace the shader or to set uniform variables during pipeline execution
through object properties.

usage: glshader location=<shader_file> preset=<variables_file> vars=<variables>


gst-libs/gst/gl/gstglshadervariables.c:

Implements a parser for GLSL variable definitions that can also be used for
other gstglfilters (eg: to set a convolution kernel or a threshold value)
Comment 1 Luc 2009-10-31 04:04:11 UTC
Created attachment 146619 [details]
test application
Comment 2 Luc 2009-10-31 04:13:42 UTC
helloworld usage:

mkfifo fifo
./helloworld rtsp://cam <shader> [ <preset> ]

Example:
load a shader: echo "l myshader.fs" > fifo
load a preset: echo "p myshader.init" > fifo 
set one or many variables: echo "v float f[3]=float[3](1,2,3);" > fifo

Note: Preset file is (re)loaded when shader is changed. Maybe it would be better trying to load a ".init" file automatically when a shader is loaded.
Comment 3 Julien Isorce 2009-11-01 11:47:50 UTC
Thx for those features that come with the attachements. We will look into it asap.
Comment 4 Julien Isorce 2009-11-05 14:30:44 UTC
Created attachment 147010 [details] [review]
fix some warnings, remove some useless includes, indent and build a real patch

First, I tested it your filter and it's pretty cool and usefull.
But I think gstglshadervariables should be put in a GObject and 
some cleanup has to be donne (for example there are some exit(1))
and a lot of commented code.
You could start from the attached patch
Comment 5 Filippo Argiolas 2009-11-05 15:10:02 UTC
Review of attachment 147010 [details] [review]:

First of all, thanks Julien for making a real patch, it's a lot easier to review now :)
And obviously thanks Luc for contributing back your code upstream!
I took a quick overall look at the proposed changes, didn't have the time to test it yet but I have a few concerns.

I generally like the idea of a general purpose filter to use for shader developing.
I'd rename it to gstglfiltershaderdebug or gstglshaderdebug or something like this. IMHO it is only useful though, in a designing initial phase, where you want to test different code paths and need to frequently change the shader. When you're happy with your result you can just hardcode your shader in the new plugin source.
With this use case in mind, it is really worth to ship and maintain such complex parser code? How much uniforms did you happen to use in your shaders?
I mean if you have say up to 10-20 parameters in you shader and you want to have them dynamically changeable at runtime you can still bind each one to a gobject property with little effort.
What I'm trying to say is that I don't completely see the use case we are aiming to with that parser, I see it as useful just for the designing, debugging phase.
That doesn't necessary mean it's not useful at all, we could include it and enable it by default only in the uninstalled setup so that's available to developers who wants to use it. So, I'm not refusing the patches, I'd just like to better understand the problem they are trying to solve.
Comment 6 Luc 2009-11-05 17:28:05 UTC
The main purposes for plugin glshader are:

- To be able to specify shaders and uniform variables initial values in the command line, without having to (re)compile any gst-plugin.

- To be able to change shaders or uniform variables values at runtime from a gstreamer application using g_object_set, without having to (re)compile any gst-plugin.

Sure you can use it as base in a design phase for a plugin where you must absolutely add some extra code on the C side.

But when no C code is required i dont see the advantage to build a GStreamer plugin for every shader. More, not everybody knowing or learning GLSL is able to do it or want to invest time for it.

On the other hand it would be nice to be able to modify variables for existing filters, and to store shaders in /usr/share/gstreamer instead of hardcoding them.

Im still working on glshader. Now you can also specify a vertex shader, and i may add some functionalities as and when i understand better what could be needed (for example concerning vertex attributes initialisation, or concerning any other resolvable C side requirements). 

I would prefer to have reused the 3DLabs open source GLSL parser from their glsl-validator. But 3DLabs became ZiiLabs and i didn't find the validator nor the sourcecode. Anyway i dont know if it was GPL compatible.

The parser code is not very complex as the variable declaration syntax is not:
   "type name[arraysize]=type[arraysize](value[,value]);"

There's a lot of copy/paste with minor changes for each data type. It took me one day and a few bugfixes. I wont dare to say it's yet totally bugfree, but normally it will not require a lot of maintenance (if any) since syntax is not subject to change. 

gst_gl_shadervariables can also be used as interface to store and/or to load binary representations of uniform variable sets, using struct gst_gl_shadervariable_desc and function gst_gl_shadervariable_set() 

I could still implement automatic type casting, getting the variable types in the shader source, but it is not crucial.

I'll post here a cleaner and more documented patch asap.

Thank you !
Comment 7 Julien Isorce 2009-11-16 13:45:36 UTC
(In reply to comment #6)
> I'll post here a cleaner and more documented patch asap.

Ok nice !
Comment 8 Filippo Argiolas 2010-04-23 09:29:16 UTC
I'm willing to move shaders into single files and load them at run time.
I looked for the 3dlabs parser mentioned above (http://3dshaders.com/home/index.php?option=com_weblinks&catid=13&Itemid=33) and seems quite simple to me. It just load files into a string with no validation nor uniform loading.

Luc any news about the cleaner patch? I still think the code in your patch is unnecessarily complicated: as bugfree as it can be, it is difficult to read and understand.
Comment 9 Luc 2010-04-24 07:40:14 UTC
> --- Comment #8 from Filippo Argiolas <fargiolas@gnome.org> 2010-04-23 09:29:16 UTC ---
> I'm willing to move shaders into single files and load them at run time.
> I looked for the 3dlabs parser mentioned above
> (http://3dshaders.com/home/index.php?option=com_weblinks&catid=13&Itemid=33)
> and seems quite simple to me. It just load files into a string with no
> validation nor uniform loading.

Why do you need a parser to load files without validation nor uniform
loading ? open() and read() do the same job.

> Luc any news about the cleaner patch?

I'm in Thailand since a few months, i just started to update my gstreamer
and gst-plugins-gl sources the other day because i need to finish Kifu.
But I'm in holidays and payed work allowing me to stay longer is my
priority.  

> I still think the code in your patch is unnecessarily complicated:

The parser is not a patch it is a new feature , only the glue code is; and the glue code is simple.

It is not because you dont need to do what's described in the two firsts paragraphs of comment #6, that the parser is unnecessary. Say you dont need it here and now.

The parser is not so complicated (although the source is not yet
documented), it just parse a string for uniform variables declarations and
initialisation values and passes them to the shader.

>  as bugfree as it can be, it is difficult to read and
> understand.

It is undocumented low-level code, so yes it takes time and energy to read and
understand it with a brute approach.

To understand the easy way, i suggest you to write a test application calling gst_gl_shadervariables_parse(0,"uniform int test=(int)10;",0) from gstglshadervariables.c and use a debugger and 'step into' to see what happends.

Maybe I'm gonna add comments so that you just feel better and waste less time and energy while trying to understand something you dont need to.. (but is it really necessary to waste OUR time and energy before theres a real need ?)

I feel like i'm wasting time and energy here and now :)

Regards from the kingdom of smile,

:7üC:
Comment 10 Filippo Argiolas 2010-04-24 10:25:39 UTC
(In reply to comment #9)
> > --- Comment #8 from Filippo Argiolas <fargiolas@gnome.org> 2010-04-23 09:29:16 UTC ---
> > I'm willing to move shaders into single files and load them at run time.
> > I looked for the 3dlabs parser mentioned above
> > (http://3dshaders.com/home/index.php?option=com_weblinks&catid=13&Itemid=33)
> > and seems quite simple to me. It just load files into a string with no
> > validation nor uniform loading.
> 
> Why do you need a parser to load files without validation nor uniform
> loading ? open() and read() do the same job.

Uh, obviously I don't need it, I was just pointing you to that since you mentioned you wasn't able to find it anymore. Also from your comment it seemed that if that parser was still available you wouldn't have had to write a new one from scratch, hence I said that parser doesn't parse anything, it just loads the shader into a string.

> > Luc any news about the cleaner patch?
> 
> I'm in Thailand since a few months, i just started to update my gstreamer
> and gst-plugins-gl sources the other day because i need to finish Kifu.
> But I'm in holidays and payed work allowing me to stay longer is my
> priority.  

Sure, no hurry, I just wanted to know if you were still available and willing to work on this.

> > I still think the code in your patch is unnecessarily complicated:
> 
> The parser is not a patch it is a new feature , only the glue code is; and the
> glue code is simple.

I'm not sure if you ever worked on maintaining a free software project. People wanting to add cool feature or to scratch their itch come every day.
They write their cool code and then disappear and the maintainer is left with more code to keep track of. I'm not talking particularly about you so no offense. I'm just saying that sometimes a documented patch really helps the people who will have to look at your code in the future.

> It is not because you dont need to do what's described in the two firsts
> paragraphs of comment #6, that the parser is unnecessary. Say you dont need it
> here and now.

I don't have a strong opinion on this. Maybe I'm a sligthly biased towards considering it an overkill. Again, no offense, really. Anyway I really have little time to dedicate to gst-plugins-gl lately, I'll leave the decision to Julien ;)

> The parser is not so complicated (although the source is not yet
> documented), it just parse a string for uniform variables declarations and
> initialisation values and passes them to the shader.
> 
> >  as bugfree as it can be, it is difficult to read and
> > understand.
> 
> It is undocumented low-level code, so yes it takes time and energy to read and
> understand it with a brute approach.

Sure, maybe it's just that I don't have so much time to do a proper review. Again, I'll just pass the ball to Julien.

> To understand the easy way, i suggest you to write a test application calling
> gst_gl_shadervariables_parse(0,"uniform int test=(int)10;",0) from
> gstglshadervariables.c and use a debugger and 'step into' to see what happends.

Are you serious?

> Maybe I'm gonna add comments so that you just feel better and waste less time
> and energy while trying to understand something you dont need to.. (but is it
> really necessary to waste OUR time and energy before theres a real need ?)

Again I'm pretty sure your code works fine and doesn't have any bug.
But that's just not enough. I cannot believe you think your patch is ready to be committed as is.
Would you at least split your big patch into single self contained commits?
For example, all the new functions you added to gstglshader are ready to be committed. Also could you move the shader loading code into gstglshader so that it's reusable by other plugins? Remove all that commented code? Add at least a comment to each function to explain what's supposed to do?

There is a thing that puzzles me without looking at the code:
"type name[arraysize]=type[arraysize](value[,value]);"
Why do you need the type cast after the "="? can the type of the rvalue be different from the variable type you just declared? Sorry if it is something silly... maybe I'm just too tired to see the reason.

> I feel like i'm wasting time and energy here and now :)

Same for me :)
 
> Regards from the kingdom of smile,

Regards
Comment 11 Luc 2010-04-24 14:23:12 UTC
(In reply to comment #10)

> > Why do you need a parser to load files without validation nor uniform
> > loading ? open() and read() do the same job.
> 
> Uh, obviously I don't need it, I was just pointing you to that since you
> mentioned you wasn't able to find it anymore. Also from your comment it seemed
> that if that parser was still available you wouldn't have had to write a new
> one from scratch

Oh i remember now, thanks ! But it was before writing one from scratch, it's coming too late :)

> > I'm in Thailand since a few months, i just started to update my gstreamer
> > and gst-plugins-gl sources the other day because i need to finish Kifu.
> > But I'm in holidays and payed work allowing me to stay longer is my
> > priority.  
> 
> Sure, no hurry, I just wanted to know if you were still available and willing
> to work on this.
> 

I'm still alive and i still need it :)

> I'm not sure if you ever worked on maintaining a free software project. People
> wanting to add cool feature or to scratch their itch come every day.

Fortunately i never ran into it, since i'm usually alone to maintain my free software projects :)

> They write their cool code and then disappear and the maintainer is left with
> more code to keep track of.

Everybody disappear one day.

> I'm not talking particularly about you so no
> offense. 

Oh don't worry i lost my ego long time ago :)

> I'm just saying that sometimes a documented patch really helps the
> people who will have to look at your code in the future.
> 

I know it's true, and i didn't do it. Bitching is welcome, i deserve it :)

> > To understand the parser the easy way, i suggest you to write a test application calling
> > gst_gl_shadervariables_parse(0,"uniform int test=(int)10;",0) from
> > gstglshadervariables.c and use a debugger and 'step into' to see what happends.
> 
> Are you serious?

Unfortunately yes :) I cannot see a quicker way to totally understand the parser right now.

> 
> > Maybe I'm gonna add comments so that you just feel better and waste less time
> > and energy while trying to understand something you dont need to.. (but is it
> > really necessary to waste OUR time and energy before theres a real need ?)
> 
> Again I'm pretty sure your code works fine and doesn't have any bug.

I would not bet on "doesn't have any bug", but I did not see a bug after the last I did fix; and since it has not been commited i have no user feedback.

> But that's just not enough. I cannot believe you think your patch is ready to
> be committed as is.

For beta testing and waiting for motivating feedback, it seems me it could have been commited 'as is' without demotivating bitching when i did post it. It doesn't break existing plugin functionalities. Now in addition to adding comments and cleaning code, i need time to adapt it for the current plugin version.

> Would you at least split your big patch into single self contained commits?
> For example, all the new functions you added to gstglshader are ready to be
> committed. Also could you move the shader loading code into gstglshader so that
> it's reusable by other plugins? Remove all that commented code? Add at least a
> comment to each function to explain what's supposed to do?

Sure, i will do this.

> 
> There is a thing that puzzles me without looking at the code:
> "type name[arraysize]=type[arraysize](value[,value]);"
> Why do you need the type cast after the "="? can the type of the rvalue be
> different from the variable type you just declared? Sorry if it is something
> silly... maybe I'm just too tired to see the reason.

It is just strict syntax. Maybe it was easier to implement(?) or my ATI driver shader compiler require the same explicit type cast after the affectation operator and i mirrored the behaviour(?)... I made this choice for one of those reasons or both but i cannot remember exactly today.

> > Regards from the kingdom of smile,
> 
> Regards

Sincerly
Comment 12 Luc 2010-05-16 21:58:26 UTC
Created attachment 161186 [details] [review]
git diff -p --cached

The parser in gstglshadervariables.c is now somewhat cleaned, slightly more documented and functions reordered for easier reading (however git indenting made it more difficult to read; maybe you should use ident again with other parameters before trying to)

A vertex shader can now also be specified in the command line or set at runtime using g_object_set(), in addition to the fragment shader and the uniform variables. To be continued...

Sorry Julien, i did not start from your attachement: your "fix some warnings, remove some useless includes" is lost.
Comment 13 Tobias Mueller 2010-09-30 20:48:00 UTC
Reopening as the updated patch has been provided.
Comment 14 Luc 2011-06-07 02:32:15 UTC
Created attachment 189373 [details] [review]
git diff -p --cached

This patch includes other files missing in the previous one.
Comment 15 Luc 2011-06-07 04:37:45 UTC
I did setup a git repository for backup, and so that people I dont know can also use my work before you decide what you do with it.

Since it seems it doesn't break anything and that i'm able to maintain it, you could alternatively grant me write access to your repository. 

-----------------------------------------------------------------
You can easily include my plugin in your repository with:
    git remote add glfitershader git://git.miprosoft.com/gst-plugins-gl
    git pull glfiltershader master

I will not use this branch for the development version.

-----------------------------------------------------------------
People can also browse my repository at http://git.miprosoft.com

Or download it with
    git clone git://git.miprosoft.com/gst-plugins-gl

In the case my repository is outdated, it can be updated with the official one:
    git remote add upstream git://anongit.freedesktop.org/gstreamer/gst-plugins-gl.git
    git pull upstream master
Comment 16 Julien Isorce 2011-11-17 16:43:25 UTC
commit c711633f4de7bcded15bbbfa16ebf49cfe45638f
Author: Luc Deschenaux <luc.deschenaux@freesurf.ch>
Date:   Thu Nov 17 17:36:44 2011 +0100

    glshader: add dynamic fragment shader filter
    
    Also add fragment shader parser
    Fix bug #600195