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 539108 - gst_ghost_pad_new() does more than call g_object_new()
gst_ghost_pad_new() does more than call g_object_new()
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal blocker
: 0.10.22
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 539063
 
 
Reported: 2008-06-19 10:48 UTC by Murray Cumming
Modified: 2009-01-12 20:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GstGhostPad construction helper functions (11.80 KB, patch)
2008-06-27 02:55 UTC, José Alburquerque
none Details | Review
GstGhostPad construction helper functions (condense duplicate code) (14.19 KB, patch)
2008-06-27 06:07 UTC, José Alburquerque
none Details | Review
Conserves the _new() method functionality (returning NULL where appropriate as before) (14.99 KB, patch)
2008-06-27 21:09 UTC, José Alburquerque
none Details | Review
Test case #1 (669 bytes, text/plain)
2008-10-07 04:20 UTC, José Alburquerque
  Details
Test case #2 (656 bytes, text/plain)
2008-10-07 04:21 UTC, José Alburquerque
  Details
Test case #3 (538 bytes, text/plain)
2008-10-07 04:22 UTC, José Alburquerque
  Details

Description Murray Cumming 2008-06-19 10:48:34 UTC
gst_ghost_pad_new() contains implementation, but *_new() functions should really just call g_object_new(), because they are C convenience functions that language bindings generally don't call. This allows language bindings to use derived GTypes.

The code should probably be in an _init() function, or should be dealt with by the property system.

gst_ghost_pad_new_no_target() also calls private API (gst_ghost_pad_new_full), so it can't even be easily reimplemented.

Here is the current implementation of gst_ghost_pad_new():

GstPad *
gst_ghost_pad_new (const gchar * name, GstPad * target)
{
  GstPad *ret;

  g_return_val_if_fail (GST_IS_PAD (target), NULL);
  g_return_val_if_fail (!gst_pad_is_linked (target), NULL);

  GST_LOG ("name:%s, target:%s:%s", GST_STR_NULL (name),
      GST_DEBUG_PAD_NAME (target));

  if ((ret = gst_ghost_pad_new_no_target (name, GST_PAD_DIRECTION (target))))
    if (!gst_ghost_pad_set_target (GST_GHOST_PAD_CAST (ret), target))
      goto set_target_failed;

  return ret;

  /* ERRORS */
set_target_failed:
  {
    GST_WARNING_OBJECT (ret, "failed to set target %s:%s",
        GST_DEBUG_PAD_NAME (target));
    gst_object_unref (ret);
    return NULL;
  }
}
Comment 1 Sebastian Dröge (slomo) 2008-06-24 05:51:31 UTC
The biggest problem with this is, that GObject doesn't allow object instanciation to fail and doesn't allow property getters/setters to fail, otherwise all the custom code could be easily implemented via GObject properties.

I see no way to fix this bug properly until bug #539886 is adressed in GObject, do you think we can close this (and the other bugs if the same argumentation holds there too) until GObject supports this?
Comment 2 Murray Cumming 2008-06-24 06:52:02 UTC
> The biggest problem with this is, that GObject doesn't allow object >
> instanciation to fail and doesn't allow property getters/setters to fail,

At the least, you can use only public API in your _new() functions, and try to use as little code in them as possible. In the extreme case, that probably means using a public _construct() function. There's a simple example of that here:
http://www.gtkmm.org/docs/gtkmm-2.4/docs/tutorial/html/sec-wrapping-problems.html#wrapping-no-properties

Bug #539886 is unlikely to ever be implemented and I wouldn't want to wait for it. However, it's not unreasonable to expect us (the C++ people) to provide a patch.
Comment 3 Sebastian Dröge (slomo) 2008-06-24 07:13:03 UTC
So essentially what you want is gst_ghost_pad_construct(), which does all the initialization magic that is done in gst_ghost_pad_new() and friends on an already instanciated object? If you want to provide patches that's fine I guess, it should definitely be noted in the docs though that this is just meant for bindings or subclasses and should never ever be called on already used object instances, etc.
Comment 4 Murray Cumming 2008-06-24 07:28:37 UTC
Yes.
Comment 5 José Alburquerque 2008-06-26 05:13:41 UTC
I may be able to take a cut a providing a patch for this a little later in the day.  I'll have to think about this a bit, but I'll see what I can do.
Comment 6 José Alburquerque 2008-06-27 02:55:44 UTC
Created attachment 113497 [details] [review]
GstGhostPad construction helper functions

I've tried this patch and makes thing work better binding things here.  I believe this is along the lines of what this report aims to fix.  Is this okay?  Thanks.
Comment 7 José Alburquerque 2008-06-27 06:07:51 UTC
Created attachment 113507 [details] [review]
GstGhostPad construction helper functions (condense duplicate code)

I just realized that I could have condensed code that was duplicated and achieve similar results.  Is this patch ok?  Thanks.
Comment 8 José Alburquerque 2008-06-27 21:09:57 UTC
Created attachment 113554 [details] [review]
Conserves the _new() method functionality (returning NULL where appropriate as before)

Okay, I think this should be the last patch I submit (I thought it may be important for the *_new() functions to maintain the same functionality and return NULL if an unsuccessful construction of the GstGhostPad is encountered).  Also the GST_LOG (...) calls should not be duplicated.
Comment 9 Sebastian Dröge (slomo) 2008-06-28 11:35:08 UTC
Murray, just curious but how are you handling parameter checking in the C++ bindings? Most of the foo_type_new() functions check the parameters and return NULL or something when something is wrong... if you just call g_object_new() you could create broken object instances as g_object_new() can't fail.
Comment 10 José Alburquerque 2008-06-29 04:45:35 UTC
(In reply to comment #9)
> if you just call g_object_new()
> you could create broken object instances as g_object_new() can't fail.
> 

I know the question is not at me, but I think I should answer what is relevant as to this case, though Murray may want to address other things:  As I think you already know, in C++, objects can inherit from parent classes, etc.  In this case, Gst::GhostPad inherits from Gst::Pad which inherits from Gst::Object (GstObject) which in the gstreamermm C++ bindings inherits from the Glib::Object class for convenience (This is all pretty much as what happens with GObjects in C).

In the C++ bindings, when developers want to create a ghost pad, for example, they would use a create() method (with the appropriate parameters) which instantiates the object (using a C++ new statement) and returns the object "embedded" in a Glib::RefPtr class (which handles automatic object referencing/unreferencing and object destruction when the reference reaches zero).

When the object is instantiated, the constructors of the classes that the object inherits from, starting from the base class (in this case Glib;:Object) and ending with its most derived constructor (in this case Gst::GhostPad), are called.  (Please note that a class, like Gst::GhostPad, can have more than one constructor and they are distinguished by the parameter list they use.)

In these C++ bindings, all GObjects are ultimately created in the Glib::Object constructor with a call to g_object_newv() and kept in the derived object itself (in this case the Gst::GhostPad object).

In this particular case, with the patch I provided, once the GObject is created and stored in the Gst::GhostPad, the appropriate Gst::GhostPad constructor uses one of the gst_ghost_pad_construct*() functions to appropriately "construct" the GObject as valid GStreamer GstGhostPad.  If the construction fails (the gboolean return), the constructor provides a warning to the developer, though the C++ object still exists without a valid GstGhostPad GObject.  The developers can then debug their code because it's probably an error in the way they tried to create the Gst::GhostPad.  I hope this all makes sense.


Comment 11 Murray Cumming 2008-06-29 09:05:25 UTC
(In reply to comment #9)
> Murray, just curious but how are you handling parameter checking in the C++
> bindings? Most of the foo_type_new() functions check the parameters and return
> NULL or something when something is wrong... if you just call g_object_new()
> you could create broken object instances as g_object_new() can't fail.

We generally don't and that's generally not a problem because most _new() functions don't do much checking. And they shouldn't need to, because libraries should expect that people will use just g_object_new(). This should really be avoided - it's far better to always create an instance and have an extra is_valid() function if necessary.

But if we use the new _construct() functions then hopefully we can still be told about failures and return a null Glib::RefPtr from our create() functions.

Comment 12 Murray Cumming 2008-07-26 09:50:57 UTC
It looks like this patch hasn't been reviewed yet. Could someone do that, please?
Comment 13 Marcus Brinkmann 2008-09-04 21:43:48 UTC
Please review this patch, as I would like to use GhostPad wrappers in gstreamermm which are blocked on this patch.  I have to use an ugly work around to make it work for me.
Comment 14 Andy Wingo 2008-10-06 17:30:46 UTC
Sorry for the delay, I'll get to this this week.
Comment 15 Andy Wingo 2008-10-06 17:58:26 UTC
Fixed in HEAD, with a slightly different patch

2008-10-06  Andy Wingo  <wingo@pobox.com>

	* gst/gstghostpad.h:
	* gst/gstghostpad.c (gst_ghost_pad_construct): New function,
	finishes the initialization of ghost pad. Useful for language
	bindings and subclassers of GstGhostPad. Fixes #539108.
	(gst_ghost_pad_new_full): Use the new constructor.
Comment 16 José Alburquerque 2008-10-07 04:17:35 UTC
That was fast.  Thanks. :-)  The following two test cases, however, show a difference in functionality.  In test case #1, using the patch I submitted, gst_ghost_pad_construct() returns false, signaling failed creation, while in test case #2 using the most recent changes you checked in, gst_ghost_pad_construct() returns true, signaling a successful creation.  Test case #3 uses GStreamer API to accomplish the same as test case #1 and test case #2, but failing as in test case #1 (and unlike test case #2).  Below is the output for test case #1, #2 and #3.  Please re-open and investigate.  Thanks.

Test Case #1:
-------------

[11:14][jose@sweety:~/Projects/Programming/C++/Tests/gstreamermm]$ g++ `pkg-config --cflags --libs gstreamer-0.10` test-ghost-pad-01.c 
[11:16][jose@sweety:~/Projects/Programming/C++/Tests/gstreamermm]$ ./a.out Successfully created pad 'pad'; direction = 2.
Could not create ghost pad.

Test Case #2:
-------------

[11:49][jose@sweety:~/Projects/Programming/C++/Tests/gstreamermm]$ g++ `pkg-config --cflags --libs gstreamer-0.10` test-ghost-pad-02.c 
[11:49][jose@sweety:~/Projects/Programming/C++/Tests/gstreamermm]$ ./a.out 
Successfully created pad 'pad'; direction = 2.
Successfully created ghost pad 'gpad'; direction = 2.

Test Case #3:
-------------

[12:10][jose@sweety:~/Projects/Programming/C++/Tests/gstreamermm]$ g++ `pkg-config --cflags --libs gstreamer-0.10` test-ghost-pad-03.c 
[12:11][jose@sweety:~/Projects/Programming/C++/Tests/gstreamermm]$ ./a.out Successfully created pad 'pad'; direction = 2.
Could not create ghost pad.
Comment 17 José Alburquerque 2008-10-07 04:20:08 UTC
Created attachment 120082 [details]
Test case #1

Test case for patch I submitted.
Comment 18 José Alburquerque 2008-10-07 04:21:23 UTC
Created attachment 120083 [details]
Test case #2

Test case for recent CVS changes.
Comment 19 José Alburquerque 2008-10-07 04:22:19 UTC
Created attachment 120084 [details]
Test case #3

Test case using GStreamer API.
Comment 20 Andy Wingo 2008-10-08 19:33:09 UTC
Heya José,

Reopening while this discussion continues. Your test case is really odd; it should work, I think, but the reason it doesn't appears to be that the new pad that you create is incompatible with the "internal" pad (here we get into implementation details).

Here's the GST_DEBUG=4 snippet:

0:00:26.644419975 21326  0x9c653f0 INFO              GST_PADS gstpad.c:1805:gst_pad_link_prepare: trying to link gpad:proxypad0 and '':pad
0:00:26.644438343 21326  0x9c653f0 DEBUG             GST_CAPS gstpad.c:1773:gst_pad_link_check_hierarchy: one of the pads has no parent <'':gpad> and (NULL)
0:00:26.644474102 21326  0x9c653f0 DEBUG             GST_CAPS gstpad.c:2009:gst_pad_get_caps_unlocked:<gpad:proxypad0> get pad caps
0:00:26.644489049 21326  0x9c653f0 DEBUG             GST_CAPS gstpad.c:2013:gst_pad_get_caps_unlocked:<gpad:proxypad0> dispatching to pad getcaps function
0:00:26.644504414 21326  0x9c653f0 DEBUG             GST_PADS gstghostpad.c:255:gst_proxy_pad_do_getcaps:<gpad:proxypad0> pad has no template, returning ANY
0:00:26.644520687 21326  0x9c653f0 DEBUG             GST_CAPS gstpad.c:2026:gst_pad_get_caps_unlocked:<gpad:proxypad0> pad getcaps returned ANY
0:00:26.644538008 21326  0x9c653f0 DEBUG             GST_CAPS gstpad.c:2009:gst_pad_get_caps_unlocked:<'':pad> get pad caps
0:00:26.644551977 21326  0x9c653f0 DEBUG             GST_CAPS gstpad.c:2068:gst_pad_get_caps_unlocked:<'':pad> pad has no caps
0:00:26.644565177 21326  0x9c653f0 DEBUG             GST_CAPS gstpad.c:1685:gst_pad_link_check_compatible_unlocked: src caps ANY
0:00:26.644579634 21326  0x9c653f0 DEBUG             GST_CAPS gstpad.c:1686:gst_pad_link_check_compatible_unlocked: sink caps EMPTY
0:00:26.644623146 21326  0x9c653f0 DEBUG             GST_CAPS gstpad.c:1706:gst_pad_link_check_compatible_unlocked: intersection caps 0x9d29660 EMPTY
0:00:26.644638791 21326  0x9c653f0 DEBUG             GST_CAPS gstpad.c:1724:gst_pad_link_check_compatible_unlocked: intersection is EMPTY
0:00:26.644652969 21326  0x9c653f0 INFO              GST_PADS gstpad.c:1860:gst_pad_link_prepare: caps are incompatible

I am led to believe that your test case would not have worked before the recent changes. Odd that EMPTY is incompatible with ANY though. Does anyone more knowledgeable have any input on this?

Andy
Comment 21 Murray Cumming 2009-01-07 21:10:24 UTC
José, this seems to be waiting for you to do some more investigation, please.
Comment 22 José Alburquerque 2009-01-08 03:48:16 UTC
Sorry, I didn't quite understand that I was my turn to reply

I don't quite understand the internals of GStreamer, but the reason I supplied the test cases was to show that in the case where a ghost pad is created from a target pad, the C API fails to create the ghost pad but the present construct function cannot know to return an unsuccessful value (false), because it does not take the target pad into consideration as gst_ghost_pad_new() does.

What I'm looking for is a way to have C++ constructors, which are parallel to the ghost pad new functions first create the ghost pad with g_object_new() (there are four constructors in the C++ bindings just as there are four ghost pad new functions).

All I'm asking for is that the C++ constructors can somehow use the construct function (or functions, if more than one is needed) to properly construct the ghost pad, and to tell if the construction failed or succeeded, based on whether the corresponding ghost pad new functions would also fail or succeed.  If someone can suggest some combination that I can use with the present construct function to determine if each ghost pad new function would succeed or fail (which then the four C++ constructors can use), I would consider this bug closed.

OTHO, if this is not possible, I would greatly appreciate it if maybe a construct function can be supplied for each ghost pad new function that would correctly succeed or fail if its corresponding new function would succeed or fail like the patch I supplied does (I know it may be hacky but the idea is what I'm trying to convey).
Comment 23 José Alburquerque 2009-01-08 04:31:43 UTC
I already supplied a patch, but I guess it hasn't been acceptable.
Comment 24 José Alburquerque 2009-01-08 19:12:55 UTC
Please, is it possible for the patch I submitted to be reviewed and accepted?  If not and it is necessary to be adapted, could several construct functions be provided, one for each ghost pad new function?  The construct functions could be used after a g_object_new() and would return true/false if the corresponding ghost pad new function would also succeed/fail.  I really appreciate your attention.  Thanks.
Comment 25 Tim-Philipp Müller 2009-01-08 21:28:21 UTC
Marking as blocker for now - since there was new API added here, we should double-check if it's sufficient/correct or not (not sure what the exact issue is right now, will have a look tomorrow).
Comment 26 Jan Schmidt 2009-01-12 13:22:08 UTC
If we're going to change anything for 0.10.22, it needs to happen soon. 3rd (and supposedly final) pre-releases are due tomorrow.

From what I can see, there's not actually anything to do here. AFAICT, the difference between José's tests case #1 and the #2 probably comes down to changes elsewhere in the core related to how pads accept caps.

Each of the ghostpad_new() functions are now some variation on 1) call ghostpad_new_full(), which just does a g_object_new() and gst_ghostpad_construct() and 2) possibly set the ghostpad target. I think that means it should be possible to get identical behaviour from the bindings?


Comment 27 José Alburquerque 2009-01-12 20:58:12 UTC
Okay.  I just didn't take the time to note that gst_ghost_pad_set_target can be used in conjunction with the supplied construct function to test for success in constructors (new functions) with targets.  Sometimes the code is so intricate that it isn't quite obvious.  As Jan says, it is possible to achieve identical behavior in C++ bindings with the supplied construct function together with the set target function.

Thankfully, I consider this bug solved (because it is).  Thanks and apologies to Andy for supplying the fix and to Tim-Philipp and Jan for taking the time to make sure this is resolved.  Closing with everyone's permission.