[Bug 641204] dvbsrc: Faster tuning logic: Use poll instead of artificial usleep

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=641204
  GStreamer | gst-plugins-bad | git

--- Comment #27 from Reynaldo H. Verdejo Pinochet <reynaldo at opendot.cl> 2014-05-29 17:08:56 UTC ---
Hi Edward, thanks for taking a look

(In reply to comment #26)
> Review of attachment 277252 [details]:
> 
> Looking good (will try it later today)
> 
> Can you also make a separate patch to proxy the new properties/signals on
> dvbbasebin ?

Sure. Will attach it to this same bug. Guess you don't
want a new one for it, do you?

> 
> ::: sys/dvb/gstdvbsrc.c
> @@ +206,3 @@
>  #define DEFAULT_STATS_REPORTING_INTERVAL 100
>  #define DEFAULT_TIMEOUT 1000000 /* 1 second */
> +#define DEFAULT_TUNING_TIMEOUT 10 * GST_MSECOND /* 10 seconds */
> 
> s/seconds/milliseconds/ in comment
> 

Fixed

> @@ +1659,3 @@
>    struct dtv_properties props;
>    struct dtv_property dvb_prop[NUM_DTV_PROPS];
> +  GstClockTimeDiff timeout_step = 1 * GST_USECOND;
> 
> 1 microseconds seems a bit hardcore, no ?
>

While it did sound a bit extreme to me at first. I
seem to be getting only 2 retries on average here using
this step, I'd say it's OK.

In addition, Having a short step seemed like a good way
to allow for the time-spent-tuning approximation on the
polling loop (I'm assuming the step timeout is always
reached there). For using a larger step I would have to
do proper time-spent approximation to avoid been that
off. In my opinion though, the current approach is sane
enough and works just fine.

> @@ +1664,1 @@
> +  GST_DEBUG_OBJECT (object, "gst_dvbsrc_tune_fe");
> 
> a better message ? "Starting tuning" for ex ?
> 

Changed, thanks.

> @@ +1699,3 @@
> +  if (!gst_poll_add_fd (poll_set, &fe_fd)) {
> +    GST_WARNING_OBJECT (object, "Could not add frontend fd to poll set");
> +    goto fail;
> 
> forgot to free the poll_set
> 
> @@ +1716,3 @@
> +  if (!gst_dvbsrc_set_fe_params (object, &props)) {
> +    GST_WARNING_OBJECT (object, "Could not set frontend params");
> +    goto fail;
> 
> forgot to free the poll_set
> 

Already freed at fail:, I guess you missed it.

> @@ +1721,3 @@
> +  GST_DEBUG_OBJECT (object, "Setting %d properties", props.num);
> +  if (ioctl (object->fd_frontend, FE_SET_PROPERTY, &props) < 0) {
> +    GST_WARNING_OBJECT (object, "Error tuning channel: %s", g_strerror
> (errno));
> 
> so this is not a fatal issue ?

The element has two paths to _tune() (and hence tune_fe()).
This was the original logic and my changeset leaves that
untouched. On start(), a failed tune is considered fatal,
whereas at set_prop() the code seems to be cool with it. I
think the latter is a more correct approach btw. In my
opinion, it shouldn't be up to _tune_fe() to threat it
as an error but to the calling logic, cause from the DVB
device perspective, it is a recoverable state. You can
change parameters and reissue a SET_PROPERTY as long as
the device is open. What do you think?

> 
> @@ +1745,3 @@
> +    GST_WARNING_OBJECT (object,
> +        "Unable to lock on signal at desired frequency");
> +    goto fail_with_signal;
> 
> forgot to free the poll set

See above ^

Thanks again for your review Edward. Appreciated.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.