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 641204 - dvbsrc: Faster tuning logic: Use poll instead of artificial usleep
dvbsrc: Faster tuning logic: Use poll instead of artificial usleep
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.3.3
Assigned To: Reynaldo H. Verdejo Pinochet
GStreamer Maintainers
: 619679 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2011-02-02 00:13 UTC by Fabrizio Milo
Modified: 2014-05-30 17:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Tuning patch (12.66 KB, patch)
2011-02-02 00:15 UTC, Fabrizio Milo
needs-work Details | Review
Further improvements: better error handling (removed all g_warning & co) (5.51 KB, patch)
2011-02-02 20:47 UTC, Fabrizio Milo
needs-work Details | Review
Further improvements: The message emit logic is based on system time not on number of reads. (3.38 KB, patch)
2011-02-02 20:48 UTC, Fabrizio Milo
rejected Details | Review
Further improvements: No C++ comments thanks. (2.96 KB, patch)
2011-02-02 21:04 UTC, Fabrizio Milo
needs-work Details | Review
Further improvements: removed FIXME (3.64 KB, patch)
2011-02-02 21:24 UTC, Fabrizio Milo
rejected Details | Review
This patch allows to control after how long the tuning logic should give up tuning. (4.85 KB, patch)
2011-02-02 22:33 UTC, Fabrizio Milo
needs-work Details | Review
better error reporting (2.45 KB, patch)
2014-05-01 06:43 UTC, Reynaldo H. Verdejo Pinochet
none Details | Review
Drop C style comments (2.12 KB, patch)
2014-05-01 06:45 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review
better error reporting (2.79 KB, patch)
2014-05-01 07:14 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review
Tuning: s/sleep()/poll + configurable timeouts + extras (27.81 KB, patch)
2014-05-27 01:49 UTC, Reynaldo H. Verdejo Pinochet
needs-work Details | Review
Tuning: s/sleep()/poll + configurable timeouts + extras (27.83 KB, patch)
2014-05-29 17:11 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review
proxy new signals/prop on dvbbasebin (3.60 KB, patch)
2014-05-30 05:10 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review
fix signal proxying on dvbbasebin (4.08 KB, patch)
2014-05-30 17:06 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review

Description Fabrizio Milo 2011-02-02 00:13:58 UTC
This patch provides a faster tuning logic.

Extracted out the parameter configuration logic into its own function

+static gboolean
+gst_dvbsrc_set_frontend_params (GstDvbSrc * object,
+    struct dvbsrc_tuning_info *info)ogic to set the frontend parameters

Added the poll tuning loop:

+static gboolean
+gst_dvbsrc_tune_frontend (GstDvbSrc * object)



The patch provide also smaller fixes like:
-#if DVB_API_VERSION == 3 && DVB_API_VERSION_MINOR == 3
+#if DVB_API_VERSION >= 3 && DVB_API_VERSION_MINOR >= 3


The timeout could be set configurable trough a parameter.
Comment 1 Fabrizio Milo 2011-02-02 00:15:57 UTC
Created attachment 179848 [details] [review]
Tuning patch
Comment 2 Fabrizio Milo 2011-02-02 00:20:05 UTC
This is an improvement over https://bugzilla.gnome.org/show_bug.cgi?id=619679
Comment 3 Fabrizio Milo 2011-02-02 20:47:13 UTC
Created attachment 179925 [details] [review]
Further improvements: better error handling (removed all g_warning & co)
Comment 4 Fabrizio Milo 2011-02-02 20:48:10 UTC
Created attachment 179926 [details] [review]
Further improvements: The message emit logic is based on system time not on number of reads.
Comment 5 Fabrizio Milo 2011-02-02 21:04:22 UTC
Created attachment 179929 [details] [review]
Further improvements: No C++ comments thanks.
Comment 6 Fabrizio Milo 2011-02-02 21:24:28 UTC
Created attachment 179933 [details] [review]
Further improvements: removed FIXME
Comment 7 Fabrizio Milo 2011-02-02 22:33:04 UTC
Created attachment 179950 [details] [review]
This patch allows to control after how long the tuning logic should give up tuning.

This patch should solve bug 604539
Comment 8 Sebastian Dröge (slomo) 2011-05-26 08:13:26 UTC
*** Bug 619679 has been marked as a duplicate of this bug. ***
Comment 9 Sebastian Dröge (slomo) 2011-05-26 08:15:16 UTC
Comment on attachment 179848 [details] [review]
Tuning patch

Please use GstPoll instead of poll and refresh this patch to latest GIT (which already uses GstPoll elsewhere for other things)

Other than that this change looks fine together with the other patches.
Comment 10 Sebastian Dröge (slomo) 2011-05-26 08:16:03 UTC
Comment on attachment 179925 [details] [review]
Further improvements: better error handling (removed all g_warning & co)

Please refresh this and the other patches to apply cleanly on latest GIT
Comment 11 Fabrizio Milo 2011-06-10 01:30:50 UTC
There is an effort to push dvbsrc into -good:

see: https://bugzilla.gnome.org/show_bug.cgi?id=581011
Comment 12 Edward Hervey 2012-06-18 08:55:47 UTC
Fabrizio, any chance of getting these patches rebased to current GIT ?
Comment 13 Tobias Mueller 2012-10-22 01:06:09 UTC
Reopening as I can't see any open non developer question.
Comment 14 Edward Hervey 2013-05-02 14:26:43 UTC
Tobias, the comment just above *is* a question and hasn't been answered.
Comment 15 Reynaldo H. Verdejo Pinochet 2014-05-01 06:43:41 UTC
Created attachment 275518 [details] [review]
better error reporting
Comment 16 Reynaldo H. Verdejo Pinochet 2014-05-01 06:45:02 UTC
Created attachment 275519 [details] [review]
Drop C style comments
Comment 17 Reynaldo H. Verdejo Pinochet 2014-05-01 06:50:34 UTC
About time to start flushing some of these :)

I'm guessing we will hold on the one changing property names,
the other two while outdated are still relevant, particularly
the timeout-as-a-prop one.
Comment 18 Reynaldo H. Verdejo Pinochet 2014-05-01 07:14:03 UTC
Created attachment 275520 [details] [review]
better error reporting
Comment 19 Reynaldo H. Verdejo Pinochet 2014-05-01 16:50:01 UTC
(In reply to comment #16)
> Created an attachment (id=275519) [details] [review]
> Drop C style comments

C++ ..
Comment 20 jingbo.hou 2014-05-10 03:09:31 UTC
Using GstPoll instead of usleep is very good idea, it makes the tuning time management easier, and it is very useful for tv scan. i hope that this patch will be merged soon.
Comment 21 Nicolas Dufresne (ndufresne) 2014-05-10 16:34:01 UTC
Review of attachment 275519 [details] [review]:

Ok, this is style fixes.
Comment 22 Nicolas Dufresne (ndufresne) 2014-05-10 16:34:51 UTC
Review of attachment 275520 [details] [review]:

Looks good to me.
Comment 23 Nicolas Dufresne (ndufresne) 2014-05-10 16:38:26 UTC
Reaynaldo, still on that ?
Comment 24 Reynaldo H. Verdejo Pinochet 2014-05-10 21:51:36 UTC
Yes. The code has changed quite a bit so pretty much all it can
be keept from tunning one it is the idea. Should be posting a new version soon.
Comment 25 Reynaldo H. Verdejo Pinochet 2014-05-27 01:49:44 UTC
Created attachment 277252 [details] [review]
Tuning: s/sleep()/poll + configurable timeouts + extras

Drops sleep logic in favor of configurable timeouts/polling
plus some extra refactors and improvements.

Most of the "extras" made sense on a single change, some
I might had handled separated (like the new signal bits)
but the thing grow organically as I attempted to match
Fabrizio's patch breakdown scheme, my bad.

Patch should apply cleanly over today's master, and has been
tested to work (with ATSC mostly).

Would love some hints/corrections specially on the polling /
timeout bits. I'm unsure as to whether moving the frontend's
poll vars to the GstDvbSrc struct would be desirable, maybe
you would like me to provide the user with a way to cancel
the main gst_poll_wait() ? I thought about pushing that idea
forward but would like to have your take on it first.
Comment 26 Edward Hervey 2014-05-28 09:34:51 UTC
Review of attachment 277252 [details] [review]:

Looking good (will try it later today)

Can you also make a separate patch to proxy the new properties/signals on dvbbasebin ?

::: 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

@@ +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 ?

@@ +1664,1 @@
+  GST_DEBUG_OBJECT (object, "gst_dvbsrc_tune_fe");

a better message ? "Starting tuning" for ex ?

@@ +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

@@ +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 ?

@@ +1745,3 @@
+    GST_WARNING_OBJECT (object,
+        "Unable to lock on signal at desired frequency");
+    goto fail_with_signal;

forgot to free the poll set
Comment 27 Reynaldo H. Verdejo Pinochet 2014-05-29 17:08:56 UTC
Hi Edward, thanks for taking a look

(In reply to comment #26)
> Review of attachment 277252 [details] [review]:
> 
> 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.
Comment 28 Reynaldo H. Verdejo Pinochet 2014-05-29 17:11:49 UTC
Created attachment 277473 [details] [review]
Tuning: s/sleep()/poll + configurable timeouts + extras

New version coming from Edward's preliminary review.
Comment 29 Reynaldo H. Verdejo Pinochet 2014-05-30 05:10:35 UTC
Created attachment 277512 [details] [review]
proxy new signals/prop on dvbbasebin

As requested.
Comment 30 Edward Hervey 2014-05-30 14:46:48 UTC
commit 4880a8ad1408f40ecf861a90625d88dbd6589ded
Author: Reynaldo H. Verdejo Pinochet <r.verdejo@sisa.samsung.com>
Date:   Fri May 30 00:49:49 2014 -0400

    dvbbasebin: proxy new props/signals from dvbsrc
    
    Proxy tuning start/done/fail signals and tuning-timeout
    property.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=641204

commit e68a6d48cbadb084b2071f49288997c7f581521e
Author: Reynaldo H. Verdejo Pinochet <r.verdejo@sisa.samsung.com>
Date:   Thu May 1 18:25:05 2014 -0400

    dvbsrc: smarten up tuning logic
    
    * Drop remaining sleep() logic in favor of polling
    * Use best guess delivery system if none is set
    * Make tuning/locking timeout configurable
    * Add signals for tuning start, done and fail
    * Drop gst_dvbsrc_frontend_status(). It was used only
      for signal LOCK checking. This is now part of the
      tuning/locking loop
    * Break up frontend configuration and tuning
      on separate functions
    
    Plus:
    
    * Add some more useful DEBUG/TRACE messages
    * Move over misplaced DVB API message
    * Fix wrong comment for default DVB buffer size (http://linuxtv.org/downloads/v4l-dvb-apis/dmx_fcalls.html#DMX_SET_BUFFER_SIZE)
    
    This patch builds up on previous work done by
    Fabrizio (Misto) Milo <mistobaan@gmail.com>
    
    https://bugzilla.gnome.org/show_bug.cgi?id=641204
Comment 31 Reynaldo H. Verdejo Pinochet 2014-05-30 17:06:16 UTC
Created attachment 277576 [details] [review]
fix signal proxying on dvbbasebin

The signal proxying on "277512: proxy new signals/prop on dvbbasebin"
was wrong. This patch sorts out all issues.
Comment 32 Reynaldo H. Verdejo Pinochet 2014-05-30 17:45:54 UTC
commit e4dac3dbb270bd7aa6f21008d5f45aaeceaaea7c
Author: Reynaldo H. Verdejo Pinochet <r.verdejo@sisa.samsung.com>
Date:   Fri May 30 11:00:06 2014 -0400

    dvbbasebin: fix dvbsrc signal proxying
    
    https://bugzilla.gnome.org/show_bug.cgi?id=641204