Interesting. Looks like we are passing NULL to gst_color_balance_list_channels().

Here is the code:


  color = gst_bin_get_by_interface (GST_BIN (src), GST_TYPE_COLOR_BALANCE);

  if (color == NULL)
    goto out;

  balance = GST_COLOR_BALANCE (color);

  channels = gst_color_balance_list_channels (balance);


So, color is not NULL but balance is, which means that color doesn't actually implement GstColorBalance which sounds like a bug in gst_bin_get_by_interface().

According to the Ubuntu bug this is with gstreamer0.10-plugins-base 0.10.35-1

> So, color is not NULL but balance is, which means that color doesn't actually
> implement GstColorBalance which sounds like a bug in
> gst_bin_get_by_interface().

If this was the case, there should be a Critical/Warning in the XSession log I think, but there isn't, is there? (Not that I have a better explanation right now).

It's also worth noting that GstColorBalance is one of those interfaces that's wrapped by the horrible GstImplementsInterface interface, which attempted to provide interfaces on a per-instance basis, so to say.

gst_bin_get_by_interface() will just use the GType checks to check for the interface implementation (which will succeed for all instances that advertise the GstColorBalance interface), while the GST_COLOR_BALANCE() cast macro will fail for instances that don't support the color balance interface at the moment for whatever reason (usually either because some device isn't open yet, i.e. it's not in a state >=READY, or because the particular device opened doesn't support this functionality).

(You can see why we've gotten rid of this in 0.11)

So in short, I think you will have to add something like this:

  /* colorbalance is wrapped by GstImplementsInterface, we
   * need to check if it is actually supported for this instance
   * in its current state before trying to use it */
  if (!GST_IS_COLOR_BALANCE (color))
      goto out;

  balance = GST_COLOR_BALANCE (color);


(Don't think gst_bin_get_by_interface() can or should check against the GstImplementsInterface stuff)

Thanks for the explanation, let's do that then.

Created attachment 196096 [details] [review]
video-src: factor out dup_color_balance()

Created attachment 196097 [details] [review]
dup_color_balance: check that element currently implements GstColorBalance

Just to be sure, I don't know exactly what's happening here, I was just speculating. It's the only explanation I can think of though. The additional check would avoid a crash like in the stack trace, but possibly you want to do something else here (e.g. set state to READY if it's a video source and it's still in NULL state or so). Depends on the context of course, which I don't know.

I suspect that's a race: user tries to change colorspace while the element isn't READY yet. Anyway, if those commits look good to you I'll merge them as they can't hurt and are good from a safe programming pov.

Looks fine to me, and should be safe in any case. I've also added some guards to the colorbalance functions in GStreamer now.

Attachment 196096 [details] pushed as 890efc4 - video-src: factor out dup_color_balance()
Attachment 196097 [details] pushed as c785ff7 - dup_color_balance: check that element currently implements GstColorBalance