fix duration handling and conversion #82
Conversation
…he sum While a time signal can have multiple splice descriptors, they all start at the same time and run in parallel. Adding them to a sum is incorrect. Return the first duration found instead.
also extend the test to catch this issue and add a new test for the other way around to test against usual framerates
allow the user to get the raw duration value
|
|
| } | ||
| } | ||
|
|
||
| func TestDurationToTicks(t *testing.T) { |
There was a problem hiding this comment.
Could you re-factor this test to be table-driven like other tests in this package?
There was a problem hiding this comment.
I don't see how that would be possible. It's a loop over various framerates and for each framerates the code tests for durations from 1 frame to as many frames as 90 seconds equals.. I don't see how that could be done in a table format, unless I pregenerate a table for all test cases, which would consist of almost 20000 entries. The existing test I expanded is not a table either, for the same reason I guess.
There was a problem hiding this comment.
Ahh yeah, good point! I'll get this merged and released shortly. Thanks again.
| func TicksToDuration(ticks uint64) time.Duration { | ||
| s := float64(ticks) / float64(TicksPerSecond) | ||
| return time.Duration(int64(s * float64(time.Second))) | ||
| return time.Duration(math.Round((float64(ticks) / TicksPerNanosecond))) |
There was a problem hiding this comment.
spurious parenthesis?
| return time.Duration(math.Round((float64(ticks) / TicksPerNanosecond))) | |
| return time.Duration(math.Round(float64(ticks) / TicksPerNanosecond)) |
| @@ -144,24 +144,29 @@ func (sis *SpliceInfoSection) Decode(b []byte) (err error) { | |||
| return nil | |||
| } | |||
|
|
|||
| // Duration attempts to return the duration of the signal. | |||
| func (sis *SpliceInfoSection) Duration() time.Duration { | |||
| // DurationTicks attempts to return the duration of the signal in ticks. | |||
There was a problem hiding this comment.
maybe add the same caveat from the PR description here (and Duration below), so that the problem is a bit more obvious on usage?
There was a problem hiding this comment.
I'll apply these improvements to a separate PR before cutting a release. I think its a good idea to capture @julijane s clarification on these functions in the documentation.
|
🙈 oops, couple of milliseconds too late
|
|
I plan to make another PR regarding durations anyways, I will see that I address your points then. |
This PR fixes two issues and adds a feature. It consists of three commits for more clarity:
Commit 1: For time signals the current code loops through all segmentation descriptors and adds up all duration values. This is completely incorrect. While there might be multiple segmentation described, they all start at the same time and run in parallel, so the duration values do not add up. There is no single correct solution. My change just returns the first actual duration found instead. Other possible choices would be to either return the smallest or the biggest value. But users dealing with signals with conflicting values should probably not use this method in the first place.
Commit 2: The helper functions DurationToTicks and TicksToDuration introduce rounding errors. For example for a duration value 12322800 the duration should be exactly 136920 milliseconds (as the value can be cleanly divided by 90). But the current code results in a value of 13691.999999 milliseconds instead. This happens due to calculating the duration as seconds first and then converting it back to a time.Duration value which has nanosecond base. The changed code introduces a new const TicksPerNanosecond and calculates directly on the nanosecond base, reducing the number of operations which can introduce inaccuracies. Also use math.Round() instead of math.Ceil() at DurationToTicks because that seems more sensible. The existing test for TicksToDurations has been extended to catch the rounding error described before. Also a new test has been added to test for the other way around, testing durations which represent full frames for usual framerates.
Commit 3: Add a new method DurationTicks() to allow the use to get the raw value instead of a time.Duration which can be helpful for debugging broken markers.