Created attachment 316311 [details] [review]
Patch to break infinite message loop

Can you provide steps to reproduce please ?

You need a device, or at least a device driver, that doesn't provide one of the four base controls: Hue, Saturation, Brightness or Contrast. For the base controls, if one of them is not implemented the behavior should be returning EINVAL. that causes the warning, which causes the dump, the dump causes a new ioctl call, which causes a warning, etc.

If you have the required behavior from the driver then a simple pipeline like:

export GST_DEBUG_DUMP_DOT_DIR=/tmp/
gst-launch-1.0 v4l2src device=/dev/video0 ! autovideosink

should be enough to cause the loop

The reason those four controls cause the problem is because of the gst_v4l2_object_set_property_helper function in v4l2object, at line 578:

    case PROP_BRIGHTNESS:
    case PROP_CONTRAST:
    case PROP_SATURATION:
    case PROP_HUE:
    {
      gint cid = gst_v4l2_object_prop_to_cid (prop_id);

      if (cid != -1) {
        if (GST_V4L2_IS_OPEN (v4l2object)) {
          gst_v4l2_set_attribute (v4l2object, cid, g_value_get_int (value));
        }
      }
      return TRUE;
    }

That calls set attribute that does no sanitization for the call. The alternative could be to implement a check here, but you would need to have a list of the available controls and cross correlate it with what you're calling. But that might be overkill.

Review of attachment 316311 [details] [review]:

In any case I mark this patch as need work for now, as it does not seem to take into account (or in fact try to figure-out) why a warning is currently posted on the bus. It also does not explain how a warning on the bus least to another. Finally, you said the spec allow returning -EINVAL, why do you try to handle it (also, reference to the spec please) ?

Can you provide a backtrace (a part of it) that show where the infinite loop starts, and where it goes through.

(In reply to patcherwork from comment #4)
> That calls set attribute that does no sanitization for the call. The
> alternative could be to implement a check here, but you would need to have a
> list of the available controls and cross correlate it with what you're
> calling. But that might be overkill.

We already enumerate that in the probes, We could do something smarter. I also want to be able to get something out of "extra-controls" getter. But let's focus on this infinit loop, as we need a proper patch for 1.6 I believe. Also, could you provide a patch against vivid or uvc, so we can reproduce you issue ?

(In reply to Nicolas Dufresne (stormer) from comment #5)

I think I didn't explain some stuff so let me try and make things a little more clear :)

> In any case I mark this patch as need work for now, as it does not seem to
> take into account (or in fact try to figure-out) why a warning is currently
> posted on the bus. 

GST_ELEMENT_WARNING posts messages to the bus. from the doc:

Utility function that elements can use in case they encountered a non-fatal data processing problem. The pipeline will post a warning message and the application will be informed.

> It also does not explain how a warning on the bus least
> to another. 

The GST_DEBUG_BIN_TO_DOT_FILE* functions iterate through the components of each element and query all the readable properties for their values. gst-launch has calls to the GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS functions in multiple cases, e.g. state transitions, warnings and errors. Say that the pipeline transitions from NULL to READY, the following steps will happen:

1) GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS is called, iterating over the pipeline elements.

2) v4l2src properties are retrieved. Amongst the properties are Hue, Saturation, Brightness and Contrast. The v4l2device calls gst_v4l2_set_attribute to retrieve the control value.

3) the control is not implemented for the device being targeted so gst_v4l2_set_attribute fails. It calls GST_ELEMENT_WARNING, posing a warning message on the bus. 

4) gst_launch handles the message. It is a warning, which calls GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS to save a dump of the state that caused the warning. we are back at 1).

> Finally, you said the spec allow returning -EINVAL, why do you
> try to handle it (also, reference to the spec please) ?

I am not sure what you mean here since I am not trying to handle it. The doc for VIDIOC_G_CTRL(http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-g-ctrl.html) states: "When the id is invalid drivers return an EINVAL error code."

And if you look at the doc for VIDIOC_QUERYCTRL (http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-queryctrl.html):

"Drivers may return EINVAL if a control in this range is not supported"

So it isn't invalid if a driver chooses to return EINVAL if a control is not provided. 

> Can you provide a backtrace (a part of it) that show where the infinite loop
> starts, and where it goes through.

I will add a patch for vivid that causes the behavior

Created attachment 316317 [details] [review]
Patch to emulate driver behavior to trigger the message loop

Ok, now it make sense. Specifically, gst-launch-1.0 try to dump the pipeline graph on warning. Now, the root problem is that posting warning (or even error) on the bus upon property change shall never happen. We need to find where and/or if those warning message make sense anywhere else (might be artefact of the past). I'd suggest to remove the warning completely from that helper and add a return value. So the caller will decide if / when / how to warn.

Review of attachment 316311 [details] [review]:

Ok, I've looked by myself, get_attribute is called through properties and few interface. Through properties, this is clearly not a good idea, while I'm not sure what need to be done through interface. Probably that the interface should not have been exposed if an required attribute was missing. I'll merge this patch so we get the best for now.

commited as df9eed0a

*** Bug 765801 has been marked as a duplicate of this bug. ***

I've just backported to 1.8 stable. Will make it for 1.8.2.