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 638108 - Add parsing library to g-v-e's new format with configurable properties
Add parsing library to g-v-e's new format with configurable properties
Status: RESOLVED OBSOLETE
Product: gnome-video-effects
Classification: Other
Component: general
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: GNOME video effects maintainers
GNOME video effects maintainers
Depends on: 626533
Blocks:
 
 
Reported: 2010-12-27 13:49 UTC by Luciana Fujii
Modified: 2021-07-05 12:26 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Luciana Fujii 2010-12-27 13:49:29 UTC
After adding configurable properties to GNOME Video Effects, the format will be a little more complicated to parse and it would be nice if GNOME Video Effects provided a library to be used by applications.

I started to develop this library in http://gitorious.org/libgve/libgve/commits/parser. It's only a start, but critiques, ideas and patches are welcome.
Comment 1 Youness Alaoui 2010-12-30 05:54:21 UTC
Haven't really had any time to look at it, just had a very quick glance at the .h file, a few small comments :
- the private struct should not be in the .h if it's meant to be private :)
- the pipeline description should probably be a property (GObject property) instead of a get_ method.
- Why is there a set_property method ? Is the object supposed to create and handle a GstElement or just parse the .effect and give the data back easily ?
- why new_from_file() why not just new()? if there is no way to create the object without specifying a file, then no need for that _from_file suffix.

Maybe have a look at what I wrote for the GstFilters, it's not a parser, it's a filter that also does parse it, the filter code should be small though so it shouldn't bother you too much :
http://git.collabora.co.uk/?p=user/kakaroto/gst-plugins-base.git;a=blob;f=gst-libs/gst/filters/gst-gnome-effect-filter.c;h=c9e4c182eb2ed82120eb099b82d49d0f678f5b9d;hb=refs/heads/move-filters

I'll have some free time in a couple of days, and I can then help out with the design of the lib.

Good job so far.
Comment 2 Luciana Fujii 2011-01-04 21:13:26 UTC
Thanks for your comments. I think I have fixed the issues you pointed.

There is a set_property method because the GVEVideoEffect creates a GstElement (a bin) from the description when the method get_effect is called. We were thinking (me and Thiago Souza Santos) of making GVEVideoEffect inherit from GstBin, but right now the Bin is only created if the method is called, in case someone wants only information but not the GstElement.

I don't really like the get_effect method, but I'm not really sure how this could be improved. Would always creating the bin be a problem?

Another issue is how to give the data back easily. I'm storing the properties as GParamSpecs in a hash table. Maybe the applications will want the GParamSpecs? I don't know. I'm happy with any help in designing the lib!
Comment 3 Youness Alaoui 2011-01-07 23:12:50 UTC
(In reply to comment #2)
> Thanks for your comments. I think I have fixed the issues you pointed.
> 
> There is a set_property method because the GVEVideoEffect creates a GstElement
> (a bin) from the description when the method get_effect is called. We were
> thinking (me and Thiago Souza Santos) of making GVEVideoEffect inherit from
> GstBin, but right now the Bin is only created if the method is called, in case
> someone wants only information but not the GstElement.

Ok, in my case, I'd probably want to get just the information, but having the lib build the element for you is also good and should be there.
Maybe separate the API into 'data gathering' and into 'effect manipulation'.

> 
> I don't really like the get_effect method, but I'm not really sure how this
> could be improved. Would always creating the bin be a problem?
How about a 'build_pipeline' method that returns the GstElement, and doesn't store it or anything, then every function to manipulate the pipeline would require the element as argument, for example :
gve_configure_property (gve, pipeline, property_key, value);

> 
> Another issue is how to give the data back easily. I'm storing the properties
> as GParamSpecs in a hash table. Maybe the applications will want the
> GParamSpecs? I don't know. I'm happy with any help in designing the lib!
Maybe just use a GstStructure with a dump of the data from the .effect file ? 

What do you think ?
Comment 4 Youness Alaoui 2011-03-03 22:48:36 UTC
Hi!

Finally got some time to look at this again.
I have a few comments on the parser... 
Sorry, I'm picky when reviewing code, so don't get depressed, it doesn't mean it's not good, it's just an extensive review :)

- Is the class supposed to be subclassable? if it's not meant to, then the class/instance structures should stay in a private header.
- 'gve_video_effect_*' seems redundant, should be 'gnome_video_effect_*' or simle 'gve_*', no?
- The Categories of the effect should also be available as a property
- The filename should probably also be available as a property
- You should add a comment on the private structure's GHashTable to tell what the association is, what kind of objects it contains and whether or not it holds a reference to them.
/* gchar * (g_strdup)-> GParamSpec * (reffed) */
- You should use g_hash_table_new_full and give it g_free as the key destructor and g_param_spec_unref for the value destructor

- gve_video_effect_parse should return a boolean, and the gve_video_effect_new should unref the object and return NULL if it couldn't parse the file.
- in gve_video_effect_parse, you don't need to have a 'locale' variable if you just use it to pass NULL to g_key_file_get_locale_string. On the other hand, it would probably be useful for the object to have a 'locale' property as CONSTRUCT_ONLY with NULL as default and use that, so a user can specify the locale he wants. I would expect that value to then be set to the current locale so the user can also know which locale was used if he decided to keep it as the default.
- in gve_video_effect_parse, you check for the has_prefix on the group name, but you don't verify that the group name actually contains a name (id) after that prefix, otherwise someone could create a group [Property::] and when you do your g_strdup, you'll end up with an id==NULL.
- in gve_video_effect_parse, you forgot to call g_key_file_free (key_file);

- in create_param, you check the Type, but if the Type is unknown, you never create pspec, then you use it.. you should do a if (pspec) { g_param_spec_set_qdata ... }
- in create_param, you don't free config.property
- in create_param, you only do a g_warning on some missing properties, but some of them should be fatal, such as a missing Name, Property or Type. Also, you shouldn't do a g_warning on a missing Comment because it should be optional I think and some people like to run their apps with a G_DEBUG=fatal_warnings.
- in create_double_param, you check for the length of the 'range' list, but then you still use range[0] and range[1] even if length != 2. You should just return NULL if it's not right.
- I know the code is not finished, but I might as well put this here.. if the type is missing, then assume 'proxy' and then take use the Property value to figure out the type of property. Same with a missing Default/Range.
- The retrieved properties should be made available to the user, since they need to show the choices in a UI and know what they can modify.
- in create_param, you should this 'param_internal' structure which i think is useless, you can easily give the name and comment as variables to the create_X_param functions. I suppose it was there as the type inside the hash table then you had the idea of using GParamSpec ?

- gve_video_effect_create_bin just creates the bin and replaces priv->bin, it should have something like :
if (priv->bin) return;
- in create_bin, you should add an ffmpegcolorspace in front of the pipeline, I noticed in my tests that it's pretty much necessary for anyone who wants to chain multiple effects together since most effects only accept a specific format.
This is however debatable, do we want the library to build you something that 'just works' or do we want it to only build whatever the .effect tells it to, and the user of the lib is responsible in putting the right ffmpegcolorspace where they need to go?
- in create_bin, instead of doing a g_hash_table_get_keys and g_list_first, g_hash_table_lookup, g_list_next, etc.. you should just do a g_hash_table_iter_init and while (g_hash_table_iter_next (&iter, &key, &value))
- in create_bin, instead of all those if/else on the type of GParamSpec, use g_param_value_set_default to set the GValue directly to the right type/value.

- in gve_video_effect_set_property, you do a g_hash_table_lookup then use pspec, but you don't validate that it's != NULL
- in gve_video_effect_set_property, you g_warning if the element can't be found in the bin, but you still use the variable even if it's NULL.
- in  gve_video_effect_set_property, you don't need to if/else and call g_object_set, simply use g_object_set_property which takes a GValue.

- gve_video_effect_get_effect should verify that priv->bin is NULL before calling create_bin (although with the above-mentioned fix, create_bin should be a no-op if priv->bin exists, it's still better to double check and make the code robust everywhere).
- gve_video_effect_get_effect should gst_object_ref(priv->bin) before returning it.


Ok, that's about it for my review of your code. Now I'll raise a few more comments on the design itself. First, I believe the library should provide you with all the data it can parse from in the .effect file, so there should be properties (or method calls if it's simpler) in order to query all of that information. Like I said above, for example, the Categories should be available as a property and those configurations (effect properties) should also be available somehow, either through a property (sounds hard) or through a method call in order to get a copy of that hash table.
It's a good idea to use a GParamSpec, I didn't think of that.
I think you should use the 'constructed' method of the GObject and do the parsing there, use a priv->error to tell the gve_new whether or not the file parsing was successful, and return that GError to the user (have a 'new' function that takes a GError as argument makes it clear that it could fail to create the object).
You should also provide a class function to list all the effect files.. it could even try to parse each .effect file and only return a list of files that are actually valid/parseable.

Again, see my implementation of the GnomeVideoEffectFilter in my GstFilters branch, ignore the 'revert' method and consider the 'apply' method as a 'create_bin'. You can take code from there, there's the Categories parsing and transformation into a property, and there's the .effect file parsing in the constructed.
http://git.collabora.co.uk/?p=user/kakaroto/gst-plugins-base.git;a=blob;f=gst-libs/gst/filters/gst-gnome-effect-filter.c;h=c9e4c182eb2ed82120eb099b82d49d0f678f5b9d;hb=refs/heads/move-filters

I hope this is useful!

p.s: of course, I don't hold any Truth, so you can always disagree on anything I said :)

Youness
Comment 5 GNOME Infrastructure Team 2021-07-05 12:26:27 UTC
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/gnome-video-effects/-/issues/

Thank you for your understanding and your help.