Repository: pdeljanov/Symphonia (language: rust)

Issue:
time::Duration
We're using Symphonia in librespot as you know, and I was looking to moving most if not all time related things to Durations for consistency and ease of use. Anyway I noticed a lack of Duration support in your time units and calculations. And I was wondering if that was on purpose or not and if you'd be open to something like this?:

```rust
use std::{fmt, time::Duration};

...

impl From<Duration> for Time {
    fn from(duration: Duration) -> Self {
        let seconds = duration.as_secs_f64();
        Time::new(seconds.trunc() as u64, seconds.fract())
    }
}

impl TimeBase {
...
    pub fn calc_time_as_duration(&self, ts: TimeStamp) -> Duration {
        let time = self.calc_time(ts);
        Duration::from_secs_f64(time.seconds as f64 + time.frac)
    }
...
}
```

This would allow us to pass a Duration almost directly when seeking and get a duration for our packet time. If it's something that you're not interested in that's fine to, as it's pretty trivial to convert between Time and Duration ourselves.   


Discussion / hints:
This may be a case of "naming things is hard".

A `Time` value is a length of time expressed in seconds from some arbitrary epoch (usually the start of the media!).

Likewise, a `TimeStamp` is a length of time expressed in time base units, also from an arbitrary epoch. Given a `TimeBase`, you can convert between `Time` and `TimeBase` easily. However, internally, we like to use `TimeStamp`s for the precision.

A `Duration`, like `TimeStamp`, is also expressed in timebase units. For audio streams, the timebase is usually `1 / sample_rate`, which means a `Duration` in the context of audio is simply a length of time expressed in number of audio samples. The key thing here is that a `Duration` represents a length of time, whereas a `Time` or `TimeStamp` represents a point in time. Which is why `Duration` is mainly just used for `AudioBuffer` stuff and basically nothing else.

In practice, there really is no reason `Duration` couldn't be deleted and `TimeStamp` used instead (they're both `u64` and would store a length of time in samples), other than it sounding weird to initialize an `AudioBuffer` with a `TimeStamp` vs. a `Duration`.

Therefore, it wouldn't exactly make sense to be able to convert a `Duration` to a `Time` or `TimeStamp` because they're trying to represent different things. However, it would make sense to be able to add or subtract a `Duration` to a `TimeStamp`. And in practice is this already possible since both `TimeStamp` and `Duration` are type aliases for `u64`.

That being said, open to suggestions and thoughts!
`Duration` can and often does serve the exact same purpose you mentioned for `Time`. Time stamps are also a thing in the rust standard lib. I was wondering also why you basically re-implemented existing things?  
In my case a `Duration` ofc would represent a point in time namely since the beginning of the track. 
> `Duration` can and often does serve the exact same purpose you mentioned for `Time`. 

Yes, since `Time` needs to be in reference to some epoch, it can therefore also be seen as a `Duration` of time from some epoch. However, it's typical for time libraries use separate types to represent a point in time (`std::time::Instant` in Rust, `std::chrono::time_point` in C++) vs. a length of time (`std::time::Duration` in Rust, `std::chrono::duration` in C++). Therefore, I decided to define them separately too.

> Time stamps are also a thing in the rust standard lib. I was wondering also why you basically re-implemented existing things?

Rust's `std::time` seems inadequate. `Instant` is an opaque type that I can't instantiate (only the `std` library can create it). `Duration` is more useable, but it can only represent a duration in fractional seconds, whereas durations in Symphonia commonly need to be in timebase units (ticks). It is better to use ticks directly rather than going through seconds which can introduce error due to the use of floating point.

If `std::time` was more generic and customizable like C++'s `std::chrono`, then I likely would've used it instead.

---

With that said, for convenience, I'd accept adding a `Time::into_duration` and  `impl From<std::time::Duration> for Time`.

I'm not particularly supportive of adding `TimeBase::calc_time_as_duration`, but `Time::into_duration` should be an acceptable substitute (i.e., `tb.calc_time(ts).into_duration()`).
That sounds fair and would do everything I'd need it to do. Thanks for the calcification. I'll put in a PR. I'm not really familiar with writing tests. Would you need any tests for that? 
> That sounds fair and would do everything I'd need it to do. Thanks for the calcification. I'll put in a PR. I'm not really familiar with writing tests. Would you need any tests for that?

Thanks. It's all pretty straightforward so I wouldn't gate the PR on tests. However, if you would like to write a couple tests to make sure the conversion from/to a duration is correct, you can check out the `verify_timebase` test in `units.rs` for an example of how we implement tests.
Would something like this do?
```rust
    #[test]
    fn verify_duration_to_time() {
        // Verify accuracy of Duration -> Time
        let dur1 = Duration::from_secs_f64(38.578125);
        let time1 = Time::from(dur1);

        assert_eq!(time1.seconds, 38);
        assert_eq!(time1.frac, 0.578125);

        // Verify accuracy of Time -> Duration
        let time2 = Time::new(38, 0.578125);
        let dur2 = time2.into_duration();

        let seconds = dur2.as_secs_f64();

        assert_eq!(seconds.trunc(), 38.0);
        assert_eq!(seconds.fract(), 0.578125);
    }
```
Yep, something like that is fine! The only thing I'd probably do differently is split it up into two functions since it's testing two distinct pieces of code.
Ok.

Fix patch:
diff --git a/symphonia-core/src/units.rs b/symphonia-core/src/units.rs
index 77b07a98..12fc4a7e 100644
--- a/symphonia-core/src/units.rs
+++ b/symphonia-core/src/units.rs
@@ -118,6 +118,18 @@ impl From<f64> for Time {
     }
 }
 
+impl From<std::time::Duration> for Time {
+    fn from(duration: std::time::Duration) -> Self {
+        Time::new(duration.as_secs(), f64::from(duration.subsec_nanos()) / 1_000_000_000.0)
+    }
+}
+
+impl From<Time> for std::time::Duration {
+    fn from(time: Time) -> Self {
+        std::time::Duration::new(time.seconds, (1_000_000_000.0 * time.frac) as u32)
+    }
+}
+
 /// A `TimeBase` is the conversion factor between time, expressed in seconds, and a `TimeStamp` or
 /// `Duration`.
 ///
@@ -231,6 +243,7 @@ impl fmt::Display for TimeBase {
 #[cfg(test)]
 mod tests {
     use super::{Time, TimeBase};
+    use std::time::Duration;
 
     #[test]
     fn verify_timebase() {
@@ -260,4 +273,26 @@ mod tests {
         );
         assert_eq!(tb1.calc_timestamp(Time::new(57_646_075_230_342_348, 0.796875)), u64::MAX);
     }
+
+    #[test]
+    fn verify_duration_to_time() {
+        // Verify accuracy of Duration -> Time
+        let dur1 = Duration::from_secs_f64(38.578125);
+        let time1 = Time::from(dur1);
+
+        assert_eq!(time1.seconds, 38);
+        assert_eq!(time1.frac, 0.578125);
+    }
+
+    #[test]
+    fn verify_time_to_duration() {
+        // Verify accuracy of Time -> Duration
+        let time1 = Time::new(38, 0.578125);
+        let dur1 = Duration::from(time1);
+
+        let seconds = dur1.as_secs_f64();
+
+        assert_eq!(seconds.trunc(), 38.0);
+        assert_eq!(seconds.fract(), 0.578125);
+    }
 }


Test patch: