From: Elio B. <ebl...@us...> - 2014-08-22 23:32:28
Attachments:
dvdauthor-dvdunauthor_fixes-1.patch
|
This patch contains two fixes for dvdunauthor. The first one is a check on return value from the times() function. The second one is a widened range for computation of remaining time, in order to prevent dvdunauthor from printing negative numbers as remaining time. Elio |
From: Lawrence D'O. <ld...@ge...> - 2014-08-24 01:23:37
|
On Sat, 23 Aug 2014 01:19:03 +0200, Elio Blanca wrote: > This patch contains two fixes for dvdunauthor. > The first one is a check on return value from the times() function. > The second one is a widened range for computation of remaining time, > in order to prevent dvdunauthor from printing negative numbers as > remaining time. > > diff -ur dvdauthor-master/src/dvdunauthor.c > dvdauthor-master-1/src/dvdunauthor.c --- > dvdauthor-master/src/dvdunauthor.c 2014-07-20 > 03:42:06.000000000 +0200 +++ > dvdauthor-master-1/src/dvdunauthor.c 2014-08-23 > 01:08:32.380673829 +0200 @@ -1158,9 +1158,9 @@ if (rl > BIGBLOCKSECT) > rl = BIGBLOCKSECT; now = times(&unused_tms); > - if (now-start > 3 * clkpsec && numsect > 0) > + if (now != (clock_t)-1 && now-start > 3 * clkpsec && > numsect > 0) { > - const int rmn = (totalsect - numsect) * (now - > start) / (numsect * clkpsec); > + const int rmn = (int)(((long long)totalsect - > numsect) * ((long long)now - start) / (numsect * clkpsec)); /* > estimate of time remaining */ fprintf > ( According to the times(2) man page <http://linux.die.net/man/2/times>, the only possible error it can return is EFAULT, which means the address of the buffer passed is invalid, which is a program bug. As for the likelihood of overflowing the 32-bit integer calculation, on my system sysconf(_SC_CLK_TCK) returns 100. I believe on some systems it may return 1000, but I’m not aware of any where the number may be higher. Even at 1000 ticks per second, signed 32-bit arithmetic isn’t going to overflow for 49 days, which would seem to be ample time to decode a few gigabytes of DVD-Video. |
From: Elio B. <ebl...@us...> - 2014-08-25 19:27:18
|
Some clarification is needed. Il 24/08/2014 03:23, Lawrence D'Oliveiro ha scritto: > According to the times(2) man page <http://linux.die.net/man/2/times>, > the only possible error it can return is EFAULT, which means the > address of the buffer passed is invalid, which is a program bug. Checking errno at first is not my preferred way to go. According to the times man page both here http://linux.die.net/man/2/times and here https://www.gnu.org/software/libc/manual/html_node/Processor-Time.html it is said that *On error, (clock_t) -1 is returned* and *times returns (clock_t)(-1) to indicate failure* Checking errno is another story, but the very first step would be checking the return value, which may be -1 if something goes wrong. Dvdunauthor should skip any calculations when the `now' value isn't reliable. Il 24/08/2014 03:23, Lawrence D'Oliveiro ha scritto: > As for the likelihood of overflowing the 32-bit integer calculation, on > my system sysconf(_SC_CLK_TCK) returns 100. I believe on some systems > it may return 1000, but I’m not aware of any where the number may be > higher. Even at 1000 ticks per second, signed 32-bit arithmetic isn’t > going to overflow for 49 days, which would seem to be ample time to > decode a few gigabytes of DVD-Video. As an example, look at this short dvdunauthor log (enriched with helpful fprintf): [cut] totalsect=2284995 numsect=80384 now=1720990742 start=1720989770 (totalsect - numsect) * (now - start) = 2142881892 STAT: [0] VOB 1, Cell 1 (4%, 4:26 remain) totalsect=2284995 numsect=80896 now=1720990744 start=1720989770 (totalsect - numsect) * (now - start) = 2146792426 STAT: [0] VOB 1, Cell 1 (4%, 4:25 remain) totalsect=2284995 numsect=81408 now=1720990745 start=1720989770 (totalsect - numsect) * (now - start) = -2146469971 STAT: [0] VOB 1, Cell 1 (4%, -4:-23 remain) totalsect=2284995 numsect=81920 now=1720990747 start=1720989770 (totalsect - numsect) * (now - start) = -2142563021 STAT: [0] VOB 1, Cell 1 (4%, -4:-21 remain) totalsect=2284995 numsect=82432 now=1720990748 start=1720989770 (totalsect - numsect) * (now - start) = -2140860682 STAT: [0] VOB 1, Cell 1 (4%, -4:-19 remain) totalsect=2284995 numsect=82944 now=1720990750 start=1720989770 (totalsect - numsect) * (now - start) = -2136957316 STAT: [0] VOB 1, Cell 1 (4%, -4:-17 remain) totalsect=2284995 numsect=83456 now=1720990752 start=1720989770 (totalsect - numsect) * (now - start) = -2133055998 STAT: [0] VOB 1, Cell 1 (4%, -4:-15 remain) totalsect=2284995 numsect=83968 now=1720990753 start=1720989770 (totalsect - numsect) * (now - start) = -2131357755 STAT: [0] VOB 1, Cell 1 (4%, -4:-13 remain) totalsect=2284995 numsect=84480 now=1720990755 start=1720989770 (totalsect - numsect) * (now - start) = -2127460021 STAT: [0] VOB 1, Cell 1 (4%, -4:-11 remain) totalsect=2284995 numsect=84992 now=1720990757 start=1720989770 (totalsect - numsect) * (now - start) = -2123564335 STAT: [0] VOB 1, Cell 1 (4%, -4:-9 remain) totalsect=2284995 numsect=85504 now=1720990760 start=1720989770 (totalsect - numsect) * (now - start) = -2117471206 STAT: [0] VOB 1, Cell 1 (4%, -4:-7 remain) [cut] Of course, the value into `rmn' is small enough to be ok into a signed int but the issue lies in intermediate calculation, overflowing very fast (the multiplication comes from the `rmn' formula). At the beginning of the job, dvdunauthor has (totalsect-numsect) which is a big value, then it slowly decrease. At the same time, (now-start) starts with small values and grows as time goes on. This originates many unpredictable integer overflows and they bring those ugly negative numbers reported as remaining time. The formula is well written, as it assures the smallest approximations, but the range used in intermediate calculations IMHO is not adequate. |
From: Lawrence D'O. <ld...@ge...> - 2014-08-26 09:04:13
|
On Mon, 25 Aug 2014 21:13:01 +0200, Elio Blanca wrote: > Checking errno at first is not my preferred way to go. Who said anything about checking errno? > Of course, the value into `rmn' is small enough to be ok into a > signed int but the issue lies in intermediate calculation, > overflowing very fast (the multiplication comes from the `rmn' > formula). The answer is to use floating point for the calculation. While we’re at it, get rid of the unwieldy use of times(2) and substitute time(2) instead. Whole-second accuracy is all we need. |
From: Elio B. <ebl...@us...> - 2014-09-02 19:31:11
Attachments:
dvdauthor-dvdunauthor_fixes-2.patch
|
Il 26/08/2014 11:04, Lawrence D'Oliveiro ha scritto: > Who said anything about checking errno? Ok, got it. > The answer is to use floating point for the calculation. While we’re at > it, get rid of the unwieldy use of times(2) and substitute time(2) > instead. Whole-second accuracy is all we need. Maybe it's ok the way it is now. |
From: Lawrence D'O. <ld...@ge...> - 2014-09-13 04:36:03
|
On Tue, 02 Sep 2014 21:30:50 +0200, Elio Blanca wrote: > Il 26/08/2014 11:04, Lawrence D'Oliveiro ha scritto: > >> While we’re at it, get rid of the unwieldy use of times(2) and substitute >> time(2) instead. Whole-second accuracy is all we need. > > Maybe it's ok the way it is now. OK, accepted. Thanks. |