Refactor and improve Delay

src/animation.rs

@@ -1,4 +1,5 @@
 use std::cmp::Ordering;
+use std::num::NonZeroU32;
 use std::time::Duration;
 
 use crate::error::ImageResult;
@@ -70,7 +71,7 @@ impl Frame {
     #[must_use]
     pub fn new(buffer: RgbaImage) -> Frame {
         Frame {
-            delay: Delay::from_ratio(Ratio { numer: 0, denom: 1 }),
+            delay: Delay::ZERO,
             left: 0,
             top: 0,
             buffer,
@@ -125,22 +126,51 @@ impl Frame {
 }
 
 impl Delay {
+    /// A delay of zero. In other words, no delay.
+    pub const ZERO: Self = Delay {
+        ratio: Ratio::from_whole(0),
+    };
+
+    /// Creates a new `Delay` from the specified number of whole milliseconds.
+    #[must_use]
+    pub fn from_millis(millis: u32) -> Self {
+        Self {
+            ratio: Ratio::from_whole(millis),
+        }
+    }
+
+    /// Returns the total number of whole milliseconds contained by this `Delay`.
+    #[must_use]
+    pub fn as_millis(self) -> u32 {
+        self.ratio.numer / self.ratio.denom
+    }
+
     /// Create a delay from a ratio of milliseconds.
     ///
+    /// # Panics
+    ///
+    /// Panics if the denominator is zero.
+    ///
     /// # Examples
     ///
     /// ```
-    /// use image::Delay;
-    /// let delay_10ms = Delay::from_numer_denom_ms(10, 1);
+    /// # use image::Delay;
+    /// let delay_10ms = Delay::from_millis(10);
+    /// assert_eq!(delay_10ms, Delay::from_numer_denom_ms(10, 1));
+    /// assert_eq!(delay_10ms, Delay::from_numer_denom_ms(100, 10));
+    /// assert_eq!(delay_10ms, Delay::from_numer_denom_ms(30, 3));
     /// ```
     #[must_use]
     pub fn from_numer_denom_ms(numerator: u32, denominator: u32) -> Self {
+        let Some(denominator) = NonZeroU32::new(denominator) else {
+            panic!("Denominator of a delay must be non-zero");
+        };
         Delay {
             ratio: Ratio::new(numerator, denominator),
         }
     }
 
-    /// Convert from a duration, clamped between 0 and an implemented defined maximum.
+    /// Convert from a duration, clamped between 0 and an implementation-defined maximum.
     ///
     /// The maximum is *at least* `i32::MAX` milliseconds. It should be noted that the accuracy of
     /// the result may be relative and very large delays have a coarse resolution.
@@ -149,10 +179,12 @@ impl Delay {
     ///
     /// ```
     /// use std::time::Duration;
-    /// use image::Delay;
+    /// # use image::Delay;
     ///
-    /// let duration = Duration::from_millis(20);
-    /// let delay = Delay::from_saturating_duration(duration);
+    /// assert_eq!(
+    ///     Delay::from_millis(20),
+    ///     Delay::from_saturating_duration(Duration::from_millis(20))
+    /// );
     /// ```
     #[must_use]
     pub fn from_saturating_duration(duration: Duration) -> Self {
@@ -186,15 +218,7 @@ impl Delay {
     /// `from_numer_denom_ms` constructor.
     #[must_use]
     pub fn numer_denom_ms(self) -> (u32, u32) {
-        (self.ratio.numer, self.ratio.denom)
-    }
-
-    pub(crate) fn from_ratio(ratio: Ratio) -> Self {
-        Delay { ratio }
-    }
-
-    pub(crate) fn into_ratio(self) -> Ratio {
-        self.ratio
+        (self.ratio.numer, self.ratio.denom.get())
     }
 
     /// Given some fraction, compute an approximation with denominator bounded.
@@ -218,12 +242,7 @@ impl Delay {
 
         // Computes the nominator of the absolute difference between two such fractions.
         fn abs_diff_nom((an, ad): (u64, u64), (bn, bd): (u64, u64)) -> u64 {
-            let c0 = an * bd;
-            let c1 = ad * bn;
-
-            let d0 = c0.max(c1);
-            let d1 = c0.min(c1);
-            d0 - d1
+            (an * bd).abs_diff(bn * ad)
         }
 
         let exact = (u64::from(nom), u64::from(denom));
@@ -292,11 +311,9 @@ impl Delay {
 
 impl From<Delay> for Duration {
     fn from(delay: Delay) -> Self {
-        let ratio = delay.into_ratio();
-        let ms = ratio.to_integer();
-        let rest = ratio.numer % ratio.denom;
-        let nanos = (u64::from(rest) * 1_000_000) / u64::from(ratio.denom);
-        Duration::from_millis(ms.into()) + Duration::from_nanos(nanos)
+        Duration::from_nanos(
+            (delay.ratio.numer as u64 * 1_000_000) / delay.ratio.denom.get() as u64,
+        )
     }
 }
 
@@ -308,37 +325,34 @@ const fn gcd(mut a: u32, mut b: u32) -> u32 {
     a
 }
 
-#[derive(Copy, Clone, Debug)]
-pub(crate) struct Ratio {
+/// A fully reduced fraction.
+///
+/// `numer` and `denom` are guaranteed to be coprime. This ensures that each fraction has a unique
+/// representation. Therefore, equality is trivial.
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+struct Ratio {
     numer: u32,
-    denom: u32,
+    denom: NonZeroU32,
 }
 
 impl Ratio {
     #[inline]
-    pub(crate) fn new(numerator: u32, denominator: u32) -> Self {
-        assert_ne!(denominator, 0);
+    fn new(numerator: u32, denominator: NonZeroU32) -> Self {
+        let denominator = denominator.get();
         let divisor = gcd(numerator, denominator);
         Self {
             numer: numerator / divisor,
-            denom: denominator / divisor,
+            denom: NonZeroU32::new(denominator / divisor).unwrap(),
         }
     }
-
-    #[inline]
-    pub(crate) fn to_integer(self) -> u32 {
-        self.numer / self.denom
-    }
-}
-
-impl PartialEq for Ratio {
-    fn eq(&self, other: &Self) -> bool {
-        self.cmp(other) == Ordering::Equal
+    const fn from_whole(numerator: u32) -> Self {
+        Self {
+            numer: numerator,
+            denom: NonZeroU32::MIN, // == 1
+        }
     }
 }
 
-impl Eq for Ratio {}
-
 impl PartialOrd for Ratio {
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
         Some(self.cmp(other))
@@ -355,9 +369,9 @@ impl Ord for Ratio {
         // a * d <cmp> c * b
 
         let a: u32 = self.numer;
-        let b: u32 = self.denom;
+        let b: u32 = self.denom.get();
         let c: u32 = other.numer;
-        let d: u32 = other.denom;
+        let d: u32 = other.denom.get();
 
         // We cast the types from `u32` to `u64` in order
         // to not overflow the multiplications.
@@ -368,6 +382,8 @@ impl Ord for Ratio {
 
 #[cfg(test)]
 mod tests {
+    use std::num::NonZeroU32;
+
     use super::{Delay, Duration, Ratio};
 
     #[test]
@@ -406,7 +422,10 @@ mod tests {
             Duration::from_millis(0xFFFF_FFFF / 1000) + Duration::from_micros(0xFFFF_FFFF % 1000);
         let delay = Delay::from_saturating_duration(fine);
         // Funnily, 0xFFFF_FFFF is divisble by 5, thus we compare with a `Ratio`.
-        assert_eq!(delay.into_ratio(), Ratio::new(0xFFFF_FFFF, 1000));
+        assert_eq!(
+            delay.ratio,
+            Ratio::new(0xFFFF_FFFF, NonZeroU32::new(1000).unwrap())
+        );
     }
 
     #[test]
@@ -426,6 +445,6 @@ mod tests {
         assert_eq!(duration.as_millis(), 0);
         // Not precisely the original but should be smaller than 0.
         let delay = Delay::from_saturating_duration(duration);
-        assert_eq!(delay.into_ratio().to_integer(), 0);
+        assert_eq!(delay.as_millis(), 0);
     }
 }

src/codecs/gif.rs

@@ -35,7 +35,7 @@ use std::num::NonZeroU32;
 use gif::ColorOutput;
 use gif::{DisposalMethod, Frame};
 
-use crate::animation::{self, Ratio};
+use crate::animation;
 use crate::color::{ColorType, Rgba};
 use crate::error::{
     DecodingError, EncodingError, ImageError, ImageResult, LimitError, LimitErrorKind,
@@ -440,7 +440,7 @@ impl FrameInfo {
             height: u32::from(frame.height),
             disposal_method: frame.dispose,
             // frame.delay is in units of 10ms so frame.delay*10 is in ms
-            delay: animation::Delay::from_ratio(Ratio::new(u32::from(frame.delay) * 10, 1)),
+            delay: animation::Delay::from_millis(u32::from(frame.delay) * 10),
         }
     }
 }
@@ -577,7 +577,7 @@ impl<W: Write> GifEncoder<W> {
         img_frame: animation::Frame,
     ) -> ImageResult<Frame<'static>> {
         // get the delay before converting img_frame
-        let frame_delay = img_frame.delay().into_ratio().to_integer();
+        let frame_delay = img_frame.delay().as_millis();
         // convert img_frame into RgbaImage
         let mut rbga_frame = img_frame.into_buffer();
         let (width, height) = self.gif_dimensions(rbga_frame.width(), rbga_frame.height())?;

src/codecs/png.rs

@@ -10,7 +10,7 @@ use std::io::{BufRead, Seek, Write};
 
 use png::{BlendOp, DeflateCompression, DisposeOp};
 
-use crate::animation::{Delay, Ratio};
+use crate::animation::Delay;
 use crate::color::{ColorType, ExtendedColorType};
 use crate::error::{
     DecodingError, ImageError, ImageResult, LimitError, LimitErrorKind, ParameterError,
@@ -242,7 +242,7 @@ fn attributes_from_info(info: &png::Info<'_>) -> DecodedImageAttributes {
             d => u32::from(d),
         };
 
-        Delay::from_ratio(Ratio::new(num, denom))
+        Delay::from_numer_denom_ms(num, denom)
     });
 
     DecodedImageAttributes {

src/codecs/webp/decoder.rs

@@ -89,7 +89,7 @@ impl<R: BufRead + Seek> ImageDecoder for WebPDecoder<R> {
                 .inner
                 .read_frame(buf)
                 .map_err(ImageError::from_webp_decode)?;
-            Some(Delay::from_numer_denom_ms(delay, 1))
+            Some(Delay::from_millis(delay))
         } else {
             self.inner
                 .read_image(buf)

src/io/image_reader_type.rs

@@ -833,8 +833,6 @@ impl<'stream> ImageReader<'stream> {
                 SequenceControl::None => return None,
             }
 
-            let no_delay = Delay::from_saturating_duration(Default::default());
-
             let mut frame = DynamicImage::default();
             let frame_decoded = match self.decode_to_dynimage(&mut frame) {
                 Ok(frame) => frame,
@@ -844,7 +842,7 @@ impl<'stream> ImageReader<'stream> {
 
             let x = frame_decoded.attributes().x;
             let y = frame_decoded.attributes().y;
-            let delay = frame_decoded.attributes().delay.unwrap_or(no_delay);
+            let delay = frame_decoded.attributes().delay.unwrap_or(Delay::ZERO);
             let frame = frame.into_rgba8();
 
             let frame = Frame::from_parts(frame, x, y, delay);