Gstpbutilsinstallplugins

coubi · October 10, 2010, 7:06pm

Hi.

I will try to port gstpbutilsinstallplugins [1] to Ruby/Gst
(Gst::InstallPlugins module)

I see that enum constants are (usually?) defined twice by G_DEF_CLASS
and G_DEF_CONSTANTS.
i.e. : GST_TYPE_MESSAGE_TYPE in rgst-message.c (constantants are
defined under Gst::Message::Type and under Gst::Message)
Should I do the same ?

The gst_install_plugins_async function take a gchar** as the first
argument (NULL-terminated array of strings).
I was thinking of define Gst::InstallPlugins.async(details,
context=nil){block} with details a ruby array.
Is there a common way (macro) to convert a ruby array to
If not should I define a macro ? In rbgst-install-plugins.c ? In
rbgst-install-plugins.h ? In a glib file ?

[1]API reference

–
Vincent C.

coubi · October 11, 2010, 1:24am

I cannot found

2010/10/10 Vincent C. [email protected]:

Hi.

The gst_install_plugins_async function take a gchar** as the first
argument (NULL-terminated array of strings).
I was thinking of define Gst::InstallPlugins.async(details,
context=nil){block} with details a ruby array.
Is there a common way (macro) to convert a ruby array to
If not should I define a macro ? In rbgst-install-plugins.c ? In
rbgst-install-plugins.h ? In a glib file ?

For now I have implemented the conversion directly in the c function.

–
Vincent C.

coubi · October 11, 2010, 1:35am

Sorry about the previous mail. I hit the wrong key!

I cannot found a example of G_DEF_CLASS2.
Are they no implementations of it ?
I want to wrap GstInstallPluginsContext.

gst_install_plugins_context_free [1] must be called when GC must be
collect the object.
[1]API reference

I do not think that
rb_cGstInstallPluginsContext=G_DEF_CLASS2(GST_TYPE_INSTALL_PLUGINS_CONTEXT,
“Context”,
mGstInstallPlugins, 0,
gst_install_plugins_context_free);

is the correct way. It should also free the DATA.

I do not know how to initalize Gst::InstallPlugins::Context new
objects using gobjet.
How work G_INITIALIZE ?

I think I miss a lot of background on gobject.

2010/10/11 Vincent C. [email protected]:

If not should I define a macro ? In rbgst-install-plugins.c ? In
rbgst-install-plugins.h ? In a glib file ?

For now I have implemented the conversion directly in the c function.

–
Vincent C.

–
Vincent C.

coubi · October 12, 2010, 3:21am

Hi,

In [email protected]
“Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins” on Mon, 11 Oct
2010 01:34:55 +0200,
Vincent C. [email protected] wrote:

I cannot found a example of G_DEF_CLASS2.
Are they no implementations of it ?
I want to wrap GstInstallPluginsContext.

Please use G_DEF_CLASS_WITH_GC_FUNC instead of
G_DEF_CLASS2. G_DEF_CLASS2 is deprecated. You can find some
examples by grepping G_DEF_CLASS_WITH_GC_FUNC at glib2/ or
gtk2/.

Thanks,

kou

coubi · October 12, 2010, 5:04pm

2010/10/12 Kouhei S. [email protected]:

Thanks for your replies.

Please use G_DEF_CLASS_WITH_GC_FUNC instead of
G_DEF_CLASS2. G_DEF_CLASS2 is deprecated. You can find some
examples by grepping G_DEF_CLASS_WITH_GC_FUNC at glib2/ or
gtk2/.

It explains why a grep on G_DEF_CLASS2 failled.

–
Vincent C.

coubi · October 12, 2010, 3:18am

Hi,

In [email protected]
“[ruby-gnome2-devel-en] gstpbutilsinstallplugins” on Sun, 10 Oct 2010
19:06:12 +0200,
Vincent C. [email protected] wrote:

I see that enum constants are (usually?) defined twice by G_DEF_CLASS
and G_DEF_CONSTANTS.
i.e. : GST_TYPE_MESSAGE_TYPE in rgst-message.c (constantants are
defined under Gst::Message::Type and under Gst::Message)
Should I do the same ?

G_DEF_CONSTANTS is for convenience. If enum constants are
used many times, we will provide shortcuts by
G_DEF_CONSTANTS. Otherwise we don’t provide shotcuts.

The gst_install_plugins_async function take a gchar** as the first
argument (NULL-terminated array of strings).
I was thinking of define Gst::InstallPlugins.async(details,
context=nil){block} with details a ruby array.
Is there a common way (macro) to convert a ruby array to
If not should I define a macro ? In rbgst-install-plugins.c ? In
rbgst-install-plugins.h ? In a glib file ?

Thera is no macro for it. We should define it rbgutil.c and
export it by rbgtuil.h.

Thanks,

kou

coubi · October 15, 2010, 10:39pm

Hi.

Here is a patch for a review. I hope my code is not to ugly.

It implementes all gstpbutilsinstallplugins functions.
As indicated in comments, the Gst::InstallPlugins::Context#set_id
triggers a segfault. It may have something to do with boxed objects. I
need some help with this point.
To reproduce the segfault :

require ‘gst’
c=Gst::InstallPlugins::Context.new
c.set_xid(42)

Segfault happens while exicting ruby.

I have also implemented some functions of gstpbutilsmissingplugins
needed to use Gst::InstallPlugins module.
In future should I provide 2 patches instead of one?

coubi · October 17, 2010, 5:44pm

2010/10/17 Kouhei S. [email protected]:

Don’t declare variables at the middle of block.
e.g.:

length=RARRAY(details)->len;

str=RARRAY(details)->ptr;

gchar *carray[length+1]; ← HERE

I declare this variable here because length is not set before.

I will take account of your other comments and send new patches to this
list.

–
Vincent C.

coubi · October 21, 2010, 2:10pm

Hi,

In [email protected]
“Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins” on Sun, 17 Oct
2010 17:44:32 +0200,
Vincent C. [email protected] wrote:

2010/10/17 Kouhei S. [email protected]:

Don’t declare variables at the middle of block.
e.g.:

length=RARRAY(details)->len;

str=RARRAY(details)->ptr;

gchar *carray[length+1]; ← HERE

I declare this variable here because length is not set before.

You can use ALLOCA_N() for dynamic memory allocation.

Thanks,

kou

coubi · October 21, 2010, 5:19pm

2010/10/21 Kouhei S. [email protected]:

str=RARRAY(details)->ptr;

Ok. I will take a look at this macro. Thanks for the info.

–
Vincent C.

coubi · October 17, 2010, 3:14pm

Hi,

nIn [email protected]
“Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins” on Fri, 15 Oct
2010 20:25:29 +0200,
Vincent C. [email protected] wrote:

c.set_xid(42)

Segfault happens while exicting ruby.

I have also implemented some functions of gstpbutilsmissingplugins
needed to use Gst::InstallPlugins module.
In future should I provide 2 patches instead of one?

Thanks for your patch.

I have some comments to your patch but I’ll show just a few
comments:

Please use PKGConfig for detecting gstpbutils.
Don’t declare variables at the middle of block.
e.g.:
- length=RARRAY(details)->len;
- str=RARRAY(details)->ptr;
- gchar *carray[length+1]; ← HERE
Don’t use C++ style comment “//”. Use “/* */” instead.
Please add a space around “=”:
context=NULL;
→
context = NULL;
Use RARRAY_LEN() and RARRAY_PTR() instead of
RARRAY()->len and RARRAY()->ptr.
Don’t define XXX2() function XXX2() name is too bad name.
Use G_DEF_SETTERS(); for define XXX= method for set_XXX.
…

We prefer to small patches with test cases instead of a
large patch. Could you split your patch with small path and
add tests for it? test/test_*.rb will help you.

Thanks,

kou

coubi · November 12, 2010, 6:10pm

2010/10/23 Vincent C. [email protected]:

Don’t define XXX2() function XXX2() name is too bad name.
add tests for it? test/test_*.rb will help you.

–
Vincent C.

Hi team.

I write this mail just to remind you this thread.
If you do not have time to help me implement gst-missing-plugins I will
wait.

–
Vincent C.

coubi · October 23, 2010, 8:38pm

Hi.

I joined 2 pathes this time.
gst-missing-plugins is to be applyed before gst-install-plugins.
gst-missing-plugins handles missing plugin Gst::MessageElement .
gst-install-plugins implements gstpbutilsinstallplugins.

Thanks for your past and future reviews.

2010/10/17 Kouhei S. [email protected]:

Don’t define XXX2() function XXX2() name is too bad name.

Use G_DEF_SETTERS(); for define XXX= method for set_XXX.

…

G_DEF_SETTERS needs a class as a argument. I do not see how it can
define set_xid and xid= for Gst::InstallPlugins::Context class!
I define context_set_xid and context_set_xid2 functions for these
setters. How should I rename it if we keep these functions?

We prefer to small patches with test cases instead of a
large patch. Could you split your patch with small path and
add tests for it? test/test_*.rb will help you.

How can I run tests ? make test does not work.
I had a test for Gst::InstallPlugins::Context#new method and
Gst::MessageElement#missing_plugin?
There is currently no ruby ways to create a missing plugin
Gst::MessageElement so I cannot test other methods.
I do not think I should test Gst::InstallPlugins functions (). Am I
wrong ?

Should I try to define RARRAY2CSTRARRAY(VALUE, ??) macro or is it a
too specific to be reused ? (cf : comment in the patch)

coubi · November 13, 2010, 5:53am

Hi,

Sorry for my late response…

We need more reviewers…

In [email protected]
“Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins” on Sat, 23 Oct
2010 20:38:36 +0200,
Vincent C. [email protected] wrote:

I joined 2 pathes this time.
gst-missing-plugins is to be applyed before gst-install-plugins.
gst-missing-plugins handles missing plugin Gst::MessageElement .
gst-install-plugins implements gstpbutilsinstallplugins.

OK. Can we process gst-missing-plugins before
gst-install-plugins? I want to process gst-install-plugins
after gst-missing-plugins is done.

2010/10/17 Kouhei S. [email protected]:

Don’t define XXX2() function XXX2() name is too bad name.

Use G_DEF_SETTERS(); for define XXX= method for set_XXX.

…

G_DEF_SETTERS needs a class as a argument. I do not see how it can
define set_xid and xid= for Gst::InstallPlugins::Context class!
I define context_set_xid and context_set_xid2 functions for these
setters. How should I rename it if we keep these functions?

What about this?

static VALUE
context_set_xid2(VALUE self, VALUE xid)
{
```
GstInstallPluginsContext *context;
```

context = (GstInstallPluginsContext *)RVAL2GOBJ(self);

gst_install_plugins_context_set_xid(context, NUM2INT(xid));/*

segfault on ruby exit */

```
return xid;
```
}
rb_define_method(rb_cGstInstallPluginsContext, “xid=”,
```
                context_set_xid2, 1);
```

G_DEF_SETTERS(rb_cGstInstallPluginsContext);

We prefer to small patches with test cases instead of a
large patch. Could you split your patch with small path and
add tests for it? test/test_*.rb will help you.

How can I run tests ? make test does not work.

Please try “ruby test/run-test.rb”.

I had a test for Gst::InstallPlugins::Context#new method and
Gst::MessageElement#missing_plugin?
There is currently no ruby ways to create a missing plugin
Gst::MessageElement so I cannot test other methods.
I do not think I should test Gst::InstallPlugins functions (). Am I wrong ?

Can we use existing Gst::MessageElement? But talking about
Gst::MessageElement is the next step.

I think that we need more tests for added
methods. e.g. Gst::InstallPlugins::return#get_name. It will
not work well.

Should I try to define RARRAY2CSTRARRAY(VALUE, ??) macro or is it a
too specific to be reused ? (cf : comment in the patch)

Please define it in the same file for now.

Other comments:

use CSTR2RVAL() instead of rb_str_new2().

static VALUE
return_get_name(VALUE self)
{

return rb_str_new2(gst_install_plugins_return_get_name(

return CSTR2RVAL(gst_install_plugins_return_get_name(
(GstInstallPluginsReturn)RVAL2GOBJ(self)));
}

use RVAL2GENUM() instead of RVAL2GOBJ() for GEnum.

static VALUE
return_get_name(VALUE self)
{
return rb_str_new2(gst_install_plugins_return_get_name(

       (GstInstallPluginsReturn)RVAL2GOBJ(self)));

       RVAL2GENUM(self, GST_TYPE_INSTALL_PLUGINS_RETURN))));

}

don’t add “get_” prefix for no argument getter.
See:
http://ruby-gnome2.sourceforge.jp/hiki.cgi?Naming+and+Conversion+Rules

rb_define_method(rb_cGstInstallPluginsReturn, “get_name”,
return_get_name,

rb_define_method(rb_cGstInstallPluginsReturn, “name”,
return_get_name,
0);

don’t use deep class hierarchy. (nest level shuld be less
than 3.)

See:
http://ruby-gnome2.sourceforge.jp/hiki.cgi?Naming+and+Conversion+Rules

GstInstallPluginsReturn → Gst::InstallPluginsReturn
GstInstallPluginsContext → Gst::InstallPluginsContext
please clean style. (please confirm again.)
We need clean style for maintaining it.

indent.

for example:

static VALUE
async(int argc, VALUE * argv, VALUE self)
{
…
if (!NIL_P(rcontext)){

           if (!RVAL2CBOOL(rb_obj_is_kind_of(rcontext,

rb_cGstInstallPluginsContext)))

       if (!RVAL2CBOOL(rb_obj_is_kind_of(rcontext,

rb_cGstInstallPluginsContext)))
…
}

space.

for example:

static VALUE
async(int argc, VALUE * argv, VALUE self)
{
…

```
   if (!NIL_P(rcontext)){
```

   if (!NIL_P(rcontext)) {
          if (!RVAL2CBOOL(rb_obj_is_kind_of(rcontext,

rb_cGstInstallPluginsContext)))
…
}

static VALUE
context_initialize(VALUE self)
{

```
  GstInstallPluginsContext * context;
```

  GstInstallPluginsContext *context;
  ...

}

Thanks,

kou

coubi · November 14, 2010, 5:02pm

2010/11/13 Kouhei S. [email protected]:

Hi,

Sorry for my late response…

We need more reviewers…

I totally understand that you have a life beside ruby-gnome2.

gst-install-plugins? I want to process gst-install-plugins
after gst-missing-plugins is done.

I agree that gst-missing-plugin should be processed before
gst-install-plugins but all your remarks are based on
gst-install-plugins.

What about this?
rb_define_method(rb_cGstInstallPluginsContext, “xid=”,
      context_set_xid2, 1);
G_DEF_SETTERS(rb_cGstInstallPluginsContext);

My bad. I should have take a look at the code to see how G_DEF_SETTERS
is used.

I had a test for Gst::InstallPlugins::Context#new method and
Gst::MessageElement#missing_plugin?
There is currently no ruby ways to create a missing plugin
Gst::MessageElement so I cannot test other methods.
I do not think I should test Gst::InstallPlugins functions (). Am I wrong ?

Can we use existing Gst::MessageElement? But talking about
Gst::MessageElement is the next step.

I think we should talk about it first and then review gst-intall-plugin.
Some Gst::MessageElement are missing_plugins some are not.
The existing Gst::MessageElement#new methods does not permit to create
a missing_plugins object.

I think that we need more tests for added
methods. e.g. Gst::InstallPlugins::return#get_name. It will
not work well.

I add a test for this method.

Should I try to define RARRAY2CSTRARRAY(VALUE, ??) macro or is it a
too specific to be reused ? (cf : comment in the patch)

Please define it in the same file for now.

I am not sure how ALLOC_N works and how it frees the memory. I do not
see how to write this macro (RARRAY2CSTRARRAY).
The comment in diff file show how I try to return a char** (same as
ALLOC_N) but I failed to see how a local pointer (carray) can be
returned.
I try to pass a pointer as a second argument but an uninitialized
pointer passed to a function does not seem a very good way.
Using ALLOC_N in the async function and then calling RARRAY2CSTRARRAY
will take away the advantages of writting a macro.
The way of wrtting this macro eludes me Sorry but my C knowledge is very
thin.

Other comments:

Other comments have been taken care of.
Hope that I have eradicated all style errors.

coubi · November 17, 2010, 4:22pm

2010/11/16 Kouhei S. [email protected]:

Thanks!

You are welcome.

gst-install-plugins? I want to process gst-install-plugins
after gst-missing-plugins is done.

I agree that gst-missing-plugin should be processed before
gst-install-plugins but all your remarks are based on
gst-install-plugins.

Ahh!!!
Sorry… It’s my mistake…
I’ll re-review your patch again later…

Thanks for your time.

ruby-gnome2-devel-en mailing list
[email protected]
ruby-gnome2-devel-en List Signup and Options

–
Vincent C.

coubi · November 19, 2010, 3:14pm

Hi,

In [email protected]
“Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins” on Sun, 14 Nov
2010 17:02:08 +0100,
Vincent C. [email protected] wrote:

gst-install-plugins but all your remarks are based on
gst-install-plugins.

I reviewed gst-missing-plugin.
I’ve changed your patch (many) and commit it.
Today, I don’t have a power to comment your patch… Sorry…
Could you check commited change in trunk?

gst-install-plugins review will be done later. (I can’t
review it in this year at least… Sorry…)

I hope that someone helps us…

Thanks,

kou

coubi · November 23, 2010, 4:41pm

I reviewed gst-missing-plugin.
I’ve changed your patch (many) and commit it.
Today, I don’t have a power to comment your patch… Sorry…
Could you check commited change in trunk?

I am updating my local repository. I will modify my test script, test
the changes and write back a mail.

gst-install-plugins review will be done later. (I can’t
review it in this year at least… Sorry…)

Sorry to hear this but I understand it.

I hope that someone helps us…

Me too as I need gst-install-plugin for the(/a ?) next release of my
audio player.

Thanks,

You are welcome.

–
kou

–
Vincent C.

coubi · November 24, 2010, 4:37pm

Since I update my repo I have a segfautl in the Gst.Init function.

#code
require ‘gst.so’
Gst.Init
#output
/usr/local/lib/site_ruby/1.8/i686-linux/gst.so:
/usr/local/lib/site_ruby/1.8/i686-linux/gst.so: undefined symbol:
rbgobj_id_children - /usr/local/lib/site_ruby/1.8/i686-linux/gst.so
(LoadError)

–
Vincent C.

coubi · November 16, 2010, 11:27pm

Hi,

In [email protected]
“Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins” on Sun, 14 Nov
2010 17:02:08 +0100,
Vincent C. [email protected] wrote:

Sorry for my late response…

We need more reviewers…

I totally understand that you have a life beside ruby-gnome2.

Thanks!

gst-install-plugins? I want to process gst-install-plugins
after gst-missing-plugins is done.

I agree that gst-missing-plugin should be processed before
gst-install-plugins but all your remarks are based on
gst-install-plugins.

Ahh!!!
Sorry… It’s my mistake…
I’ll re-review your patch again later…

Thanks,

kou