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 758703 - v4l2src: gst_v4l2_set_attribute warning messages cause infinite loop with .dot dump
v4l2src: gst_v4l2_set_attribute warning messages cause infinite loop with .do...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal normal
: 1.8.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 765801 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-11-26 12:25 UTC by Dimitrios Katsaros
Modified: 2016-05-01 21:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to break infinite message loop (1.90 KB, patch)
2015-11-26 12:36 UTC, Dimitrios Katsaros
committed Details | Review
Patch to emulate driver behavior to trigger the message loop (757 bytes, patch)
2015-11-26 14:45 UTC, Dimitrios Katsaros
none Details | Review

Description Dimitrios Katsaros 2015-11-26 12:25:00 UTC
I am developing a gstreamer pipeline on a v4l2 device using gstreamer. While trying to dump the .dot files using gst-launch I ran into an infinite message loop. When a dot file is dumped from such a pipeline, it iterates over the properties of the v4l2src, amongst which are the Hue, Saturation, Brightness and Contrast controls. the dot dump tries to query the controls, which in my case are not available. This causes the ioctl to fail, issuing a warning on the bus. the new warning causes a new dot dump which causes a new set of warnings to get posted. 

Trying to query the controls for unavailable ones is also not an option, since according to the v4l2 API for VIDIOC_QUERYCTRL: "Drivers may return EINVAL if a control in this range is not supported". Thus filtering the controls that are not available is also not possible. 

A simple fix presented here is to change the warning handling so it doesn't post on the bus.
Comment 1 Dimitrios Katsaros 2015-11-26 12:36:40 UTC
Created attachment 316311 [details] [review]
Patch to break infinite message loop
Comment 2 Nicolas Dufresne (ndufresne) 2015-11-26 13:37:13 UTC
Can you provide steps to reproduce please ?
Comment 3 Dimitrios Katsaros 2015-11-26 13:44:09 UTC
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
Comment 4 Dimitrios Katsaros 2015-11-26 13:48:24 UTC
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.
Comment 5 Nicolas Dufresne (ndufresne) 2015-11-26 13:49:11 UTC
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.
Comment 6 Nicolas Dufresne (ndufresne) 2015-11-26 13:52:01 UTC
(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 ?
Comment 7 Dimitrios Katsaros 2015-11-26 14:43:08 UTC
(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
Comment 8 Dimitrios Katsaros 2015-11-26 14:45:16 UTC
Created attachment 316317 [details] [review]
Patch to emulate driver behavior to trigger the message loop
Comment 9 Nicolas Dufresne (ndufresne) 2015-11-26 15:17:25 UTC
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.
Comment 10 Nicolas Dufresne (ndufresne) 2016-04-16 20:22:56 UTC
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.
Comment 11 Nicolas Dufresne (ndufresne) 2016-04-16 21:00:48 UTC
commited as df9eed0a
Comment 12 Nicolas Dufresne (ndufresne) 2016-05-01 13:12:35 UTC
*** Bug 765801 has been marked as a duplicate of this bug. ***
Comment 13 Nicolas Dufresne (ndufresne) 2016-05-01 21:34:19 UTC
I've just backported to 1.8 stable. Will make it for 1.8.2.