[gstreamer-bugs] [Bug 600195] dynamic fragment shader filter and variables parser/loader

Sat Apr 24 03:25:44 PDT 2010

https://bugzilla.gnome.org/show_bug.cgi?id=600195
  GStreamer | gst-plugins-gl | git

--- Comment #10 from Filippo Argiolas <fargiolas at gnome.org> 2010-04-24 10:25:39 UTC ---
(In reply to comment #9)
> > --- Comment #8 from Filippo Argiolas <fargiolas at 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

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.