GNOME Bugzilla – Bug 762147
adaptivedemux: use GstSystemClock to all real-time calculations
Last modified: 2016-04-28 14:27:01 UTC
A realtime clock is used in many places, such as deciding which fragment to select at start up and deciding how long to sleep before a fragment becomes available. For example dashdemux needs sample the client's estimate of UTC when selecting where to start in a live DASH stream. The problem with dashdemux calculating the client's idea of UTC is that it makes it difficult to create unit tests, because the passage of time is a factor in the test. We need to change dashdemux and adaptivedemux to use the GstSystemClock, so that a unit test can replace the system clock when it needs to be able to control the clock.
Created attachment 321392 [details] [review] 1/3 use GstSystemClock to all real-time calculations
Created attachment 321393 [details] [review] 2/3 use realtime_clock for waiting for a condition
Created attachment 321394 [details] [review] 3/3 tests: use a GstTestClock as the system clock
Review of attachment 321392 [details] [review]: This is a good idea, just a minor comment below. ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +400,3 @@ { GstPadTemplate *pad_template; + GstClockType clock_type = GST_CLOCK_TYPE_REALTIME; Isn't it safer to default to non-realtime here?
(In reply to Thiago Sousa Santos from comment #4) > > Isn't it safer to default to non-realtime here? The problem is that GstTestClock does not have the clock-type property. Defaulting to non-realtime would cause the unit tests to be based upon current time of day, which breaks the ability to make the tests reproducible. I wasn't feeling brave enough to add a property to GstTestClock, but maybe that's an option.
(In reply to A Ashley from comment #5) > (In reply to Thiago Sousa Santos from comment #4) > > > > > Isn't it safer to default to non-realtime here? > > The problem is that GstTestClock does not have the clock-type property. > Defaulting to non-realtime would cause the unit tests to be based upon > current time of day, which breaks the ability to make the tests reproducible. > > I wasn't feeling brave enough to add a property to GstTestClock, but maybe > that's an option. I think that adding the property to GstTestClock would be a safer bet. Can you provide a patch for it so we can merge this all?
Created attachment 326492 [details] [review] adaptivedemux: use GstSystemClock to all real-time calculations Obsoletes patch 1/3 Changes default clock type to GST_CLOCK_TYPE_OTHER
Created attachment 326493 [details] [review] testclock: add clock-type property
Thanks, merged. Only changed the default clock-type to be monotonic as we can't assume all testclock usage out there is returning us realtime. core: commit 57a9919eb16d87b19bb5d91792f3460bd34f4fcc Author: Alex Ashley <bugzilla@ashley-family.net> Date: Thu Apr 21 14:45:39 2016 +0100 testclock: add clock-type property To allow the GstTestClock to be used as a GstSystemClock, it is useful to implement the clock-type property that GstSystemClock provides. This allows GstTestClock to be used as the system clock with code that expects a GstSystemClock. https://bugzilla.gnome.org/show_bug.cgi?id=762147 -bad: commit e86e08b4ac9a939393353729e0ae598ea2ea4fc4 Author: Florin Apostol <florin.apostol@oregan.net> Date: Tue Feb 16 14:44:39 2016 +0000 adaptivedemux: tests: use a GstTestClock as the system clock To allow the adaptivedemux live stream tests to run in non-realtime, use a GstTestClock as the system clock. This allows the unit tests to complete more quickly than if they had to complete in real time. https://bugzilla.gnome.org/show_bug.cgi?id=762147 commit aa58a70d6676f9bc394780a90a39ff47d538fa68 Author: Florin Apostol <florin.apostol@oregan.net> Date: Tue Feb 16 14:44:27 2016 +0000 adaptivedemux: use realtime_clock for waiting for a condition There are several places in adaptivedemux where it waits for time to pass, for example to wait until it should next download a fragment. The problem with this approach is that it means that unit tests are forced to execute in realtime. This commit replaces the use of g_cond_wait_until() with single shot GstClockID that signals the condition variable. Under normal usage, this behaves exactly as before. A unit test can replace the system clock with a GstTestClock, allowing the test to control the timing in adaptivedemux. https://bugzilla.gnome.org/show_bug.cgi?id=762147 commit 74d62b91449b2f21d417bc67b58792217c185b8d Author: Florin Apostol <florin.apostol@oregan.net> Date: Tue Feb 16 14:44:10 2016 +0000 adaptivedemux: use GstSystemClock to all real-time calculations A realtime clock is used in many places, such as deciding which fragment to select at start up and deciding how long to sleep before a fragment becomes available. For example dashdemux needs sample the client's estimate of UTC when selecting where to start in a live DASH stream. The problem with dashdemux calculating the client's idea of UTC is that it makes it difficult to create unit tests, because the passage of time is a factor in the test. This commit changes dashdemux and adaptivedemux to use the GstSystemClock, so that a unit test can replace the system clock when it needs to be able to control the clock. This commit makes no change to the behaviour under normal usage, as GstSystemClock is based upon the system time. https://bugzilla.gnome.org/show_bug.cgi?id=762147