From 63e36ed43febc9c22a42666b1c817b7d2d63c846 Mon Sep 17 00:00:00 2001
From: Ingvar Stepanyan <rreverser@google.com>
Date: Tue, 14 Apr 2020 22:09:52 +0100
Subject: [PATCH] Simplify comparer (#208)

* Skip baselines earlier in the evaluator

We know that we'll throw them away anyway, so there is no point in even sending them to Sender and comparing them with others once we used them for the best candidate size.

* Simplify comparison logic
---
 src/colors.rs   |  2 +-
 src/evaluate.rs | 80 +++++++++++++++++--------------------------------
 2 files changed, 28 insertions(+), 54 deletions(-)

diff --git a/src/colors.rs b/src/colors.rs
index 615ee33f..f5fe57ec 100644
--- a/src/colors.rs
+++ b/src/colors.rs
@@ -56,7 +56,7 @@ impl ColorType {
     }
 }
 
-#[derive(Debug, PartialEq, PartialOrd, Clone, Copy)]
+#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy)]
 /// The number of bits to be used per channel per pixel
 pub enum BitDepth {
     /// One bit per channel per pixel
diff --git a/src/evaluate.rs b/src/evaluate.rs
index cc083b7f..ba58a8f0 100644
--- a/src/evaluate.rs
+++ b/src/evaluate.rs
@@ -24,50 +24,20 @@ use std::thread;
 
 struct Candidate {
     image: PngData,
-    // if false, that's baseline file to throw away
-    is_reduction: bool,
     filter: u8,
     // first wins tie-breaker
     nth: usize,
 }
 
-#[derive(Default)]
-struct Comparator {
-    best_result: Option<Candidate>,
-}
-
-impl Comparator {
-    fn evaluate(&mut self, new: Candidate) {
-        // a tie-breaker is required to make evaluation deterministic
-        let is_best = if let Some(ref old) = self.best_result {
-            // choose smallest compressed, or if compresses the same, smallest uncompressed, or cheaper filter
-            let new = (
-                new.image.idat_data.len(),
-                new.image.raw.data.len(),
-                new.image.raw.ihdr.bit_depth,
-                new.filter,
-                new.nth,
-            );
-            let old = (
-                old.image.idat_data.len(),
-                old.image.raw.data.len(),
-                old.image.raw.ihdr.bit_depth,
-                old.filter,
-                old.nth,
-            );
-            // <= instead of < is important, because best_candidate_size has been set already,
-            // so the current result may be comparing its size with itself
-            new <= old
-        } else {
-            true
-        };
-        if is_best {
-            self.best_result = if new.is_reduction { Some(new) } else { None };
-        }
-    }
-
-    fn get_result(self) -> Option<PngData> {
-        self.best_result.map(|res| res.image)
+impl Candidate {
+    fn cmp_key(&self) -> impl Ord {
+        (
+            self.image.idat_data.len(),
+            self.image.raw.data.len(),
+            self.image.raw.ihdr.bit_depth,
+            self.filter,
+            self.nth,
+        )
     }
 }
 
@@ -81,10 +51,10 @@ pub(crate) struct Evaluator {
     eval_send: SyncSender<Candidate>,
     // the thread helps evaluate images asynchronously
     #[cfg(feature = "parallel")]
-    eval_thread: thread::JoinHandle<Option<PngData>>,
+    eval_thread: thread::JoinHandle<Option<Candidate>>,
     // in non-parallel mode, images are evaluated synchronously
     #[cfg(not(feature = "parallel"))]
-    eval_comparator: std::cell::RefCell<Comparator>,
+    eval_best_candidate: Option<Candidate>,
 }
 
 impl Evaluator {
@@ -98,29 +68,27 @@ impl Evaluator {
             #[cfg(feature = "parallel")]
             eval_send: tx,
             #[cfg(feature = "parallel")]
-            eval_thread: thread::spawn(move || {
-                let mut comparator = Comparator::default();
-                for candidate in rx {
-                    comparator.evaluate(candidate);
-                }
-                comparator.get_result()
-            }),
+            eval_thread: thread::spawn(move || rx.into_iter().min_by_key(Candidate::cmp_key)),
             #[cfg(not(feature = "parallel"))]
-            eval_comparator: Default::default(),
+            eval_best_candidate: None,
         }
     }
 
     /// Wait for all evaluations to finish and return smallest reduction
     /// Or `None` if all reductions were worse than baseline.
     #[cfg(feature = "parallel")]
-    pub fn get_result(self) -> Option<PngData> {
+    fn get_best_candidate(self) -> Option<Candidate> {
         drop(self.eval_send); // disconnect the sender, breaking the loop in the thread
         self.eval_thread.join().expect("eval thread")
     }
 
     #[cfg(not(feature = "parallel"))]
+    fn get_best_candidate(self) -> Option<Candidate> {
+        self.eval_best_candidate
+    }
+
     pub fn get_result(self) -> Option<PngData> {
-        self.eval_comparator.into_inner().get_result()
+        self.get_best_candidate().map(|candidate| candidate.image)
     }
 
     /// Set baseline image. It will be used only to measure minimum compression level required
@@ -162,6 +130,10 @@ impl Evaluator {
                     &deadline,
                 ) {
                     best_candidate_size.set_min(idat_data.len());
+                    // ignore baseline images after this point
+                    if !is_reduction {
+                        return;
+                    }
                     // the rest is shipped to the evavluation/collection thread
                     let new = Candidate {
                         image: PngData {
@@ -169,7 +141,6 @@ impl Evaluator {
                             raw: Arc::clone(&image),
                         },
                         filter,
-                        is_reduction,
                         nth,
                     };
 
@@ -180,7 +151,10 @@ impl Evaluator {
 
                     #[cfg(not(feature = "parallel"))]
                     {
-                        self.eval_comparator.borrow_mut().evaluate(new);
+                        match self.eval_best_candidate {
+                            Some(prev) if prev.cmp_key() < new.cmp_key() => {}
+                            _ => self.eval_best_candidate = Some(new),
+                        }
                     }
                 }
             });