GNOME Bugzilla – Bug 641204
dvbsrc: Faster tuning logic: Use poll instead of artificial usleep
Last modified: 2014-05-30 17:46:16 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.
Created attachment 179848 [details] [review] Tuning patch
This is an improvement over https://bugzilla.gnome.org/show_bug.cgi?id=619679
Created attachment 179925 [details] [review] Further improvements: better error handling (removed all g_warning & co)
Created attachment 179926 [details] [review] Further improvements: The message emit logic is based on system time not on number of reads.
Created attachment 179929 [details] [review] Further improvements: No C++ comments thanks.
Created attachment 179933 [details] [review] Further improvements: removed FIXME
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
*** Bug 619679 has been marked as a duplicate of this bug. ***
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 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
There is an effort to push dvbsrc into -good: see: https://bugzilla.gnome.org/show_bug.cgi?id=581011
Fabrizio, any chance of getting these patches rebased to current GIT ?
Reopening as I can't see any open non developer question.
Tobias, the comment just above *is* a question and hasn't been answered.
Created attachment 275518 [details] [review] better error reporting
Created attachment 275519 [details] [review] Drop C style comments
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.
Created attachment 275520 [details] [review] better error reporting
(In reply to comment #16) > Created an attachment (id=275519) [details] [review] > Drop C style comments C++ ..
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.
Review of attachment 275519 [details] [review]: Ok, this is style fixes.
Review of attachment 275520 [details] [review]: Looks good to me.
Reaynaldo, still on that ?
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.
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.
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
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.
Created attachment 277473 [details] [review] Tuning: s/sleep()/poll + configurable timeouts + extras New version coming from Edward's preliminary review.
Created attachment 277512 [details] [review] proxy new signals/prop on dvbbasebin As requested.
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
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.
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