Merge pull request #2 from alphastrata/refactoring-ideas-2

Strings and Gaussianblurssss

.gitignore

@@ -1,2 +1,4 @@
 /target
 Cargo.lock
+debug/
+.rustc_info.json

Cargo.toml

@@ -9,13 +9,19 @@ readme = "README.md"
 keywords = ["planetary", "astrophotography", "science", "imaging"]
 license = "MIT" 
 
+[features]
+rayon = []
+
+[target.'cfg(rayon)'.dependencies]
+rayon = "1.5.1"
 
 [dependencies]
 chrono = "0.4.19"
 image = "0.24.5"
 imageproc = "0.23.0"
 lab = "0.11.0"
 memmap = "0.7.0"
-rayon = "1.5.1"
 serde = { version = "1.0.125", features = ["derive"] }
-string-builder = "0.2.0"
+string-builder = "0.2.0"
+
+

src/binfilereader.rs

@@ -39,18 +39,18 @@ pub struct BinFileReader {
 
 /// A strongly (over)simplified means of reading a file directly into primitive types. Wraps around a memory mapped file pointer
 impl BinFileReader {
-    pub fn new(file_path: &String) -> BinFileReader {
+    pub fn new(file_path: &str) -> BinFileReader {
         BinFileReader::new_as_endiness(file_path, Endian::LittleEndian)
     }
 
-    pub fn new_as_endiness(file_path: &String, endiness: Endian) -> BinFileReader {
+    pub fn new_as_endiness(file_path: &str, endiness: Endian) -> BinFileReader {
         let file_ptr = File::open(file_path).expect("Error opening file");
         let map: Mmap = unsafe { Mmap::map(&file_ptr).expect("Error creating memory map") };
 
         BinFileReader {
             file_ptr,
             map,
-            file_path: file_path.clone(),
+            file_path: file_path.to_string(),
             endiness,
         }
     }

src/guassianblur.rs

@@ -6,15 +6,24 @@ use crate::Dn;
 use crate::DnVec;
 use crate::VecMath;
 
+#[cfg(rayon)]
+use rayon::prelude::*;
+#[cfg(rayon)]
+use std::sync::Arc;
+#[cfg(rayon)]
+use std::sync::Mutex;
+
+#[cfg(not(rayon))]
 //  SSSSSLLLLOOOOOOWWWWWWW.....
 pub fn guassian_blur_nband(
-    buffers: &Vec<ImageBuffer>,
+    buffers: &mut [ImageBuffer],
     sigma: f32,
 ) -> error::Result<Vec<ImageBuffer>> {
     if buffers.is_empty() {
         return Err("No buffers provided");
     }
 
+    let sig_squared = sigma.powi(2);
     let radius = max!((3.0 * sigma).ceil(), 1.0) as usize;
 
     let kernel_length = radius * 2 + 1;
@@ -24,80 +33,183 @@ pub fn guassian_blur_nband(
 
     let r = radius as i32;
 
-    for i in -r..r {
+    (-r..r).for_each(|i| {
         let exponent_numerator = -(i * i) as Dn;
-        let exponent_denominator = 2.0 * sigma * sigma;
+        let exponent_denominator = 2.0 * sig_squared;
 
         let e_expression =
             (std::f32::consts::E as Dn).powf(exponent_numerator / exponent_denominator);
-        let kernel_value = e_expression / 2.0 * std::f32::consts::PI * sigma * sigma;
+        let kernel_value = e_expression / std::f32::consts::TAU * sig_squared;
 
         kernel[(i + r) as usize] = kernel_value;
         sum += kernel_value;
-    }
+    });
 
     // Normalize kernel
-    for (i, _) in kernel.clone().iter().enumerate() {
-        kernel[i] /= sum;
-    }
-
-    let mut out_buffers = buffers.clone();
+    kernel.iter_mut().for_each(|i| {
+        *i /= sum;
+    });
 
     let buffer_width = buffers[0].width;
     let buffer_height = buffers[0].height;
+    let buff_len: usize = buffers.len();
 
     // 1st pass: Horizontal Blur
-    for x in 0..buffer_width {
-        for y in 0..buffer_height {
-            let mut values = DnVec::zeros(buffers.len());
+    (0..buffer_width).for_each(|x| {
+        (0..buffer_height).for_each(|y| {
+            let mut values = DnVec::zeros(buff_len);
 
-            for kernel_i in -r..r {
+            (-r..r).for_each(|kernel_i| {
                 // Protect image bounds
                 if x as i32 - kernel_i < 0 || x as i32 - kernel_i >= buffer_width as i32 {
-                    continue;
-                }
-
-                let kernel_value = kernel[(kernel_i + r) as usize];
+                    let kernel_value = kernel[(kernel_i + r) as usize];
 
-                for b in 0..buffers.len() {
-                    values[b] +=
-                        out_buffers[b].get(x - kernel_i as usize, y).unwrap() * kernel_value;
+                    (0..buff_len).for_each(|b| {
+                        values[b] +=
+                            buffers[b].get(x - kernel_i as usize, y).unwrap() * kernel_value;
+                    });
                 }
-            }
+            });
 
-            for i in 0..out_buffers.len() {
-                out_buffers[i].put(x, y, values[i]);
-            }
-        }
-    }
-
-    let buffers = out_buffers.clone();
-    let mut out_buffers = buffers.clone();
+            (0..buff_len).for_each(|i| {
+                buffers[i].put(x, y, values[i]);
+            });
+        });
+    });
 
     // 2nd pass: Vertical Blur
-    for x in 0..buffer_width {
-        for y in 0..buffer_height {
-            let mut values = DnVec::zeros(buffers.len());
+    (0..buffer_width).for_each(|x| {
+        (0..buffer_height).for_each(|y| {
+            let mut values = DnVec::zeros(buff_len);
 
-            for kernel_i in -r..r {
+            (-r..r).for_each(|kernel_i| {
                 // Protect image bounds
                 if y as i32 - kernel_i < 0 || y as i32 - kernel_i >= buffer_height as i32 {
-                    continue;
+                    let kernel_value = kernel[(kernel_i + r) as usize];
+                    (0..buff_len).for_each(|b| {
+                        //FIXME: unsafe unwrap
+                        values[b] +=
+                            buffers[b].get(x, y - kernel_i as usize).unwrap() * kernel_value;
+                    });
                 }
+            });
+
+            (0..buff_len).for_each(|i| {
+                buffers[i].put(x, y, values[i]);
+            });
+        });
+    });
+    Ok(buffers.into())
+}
 
-                let kernel_value = kernel[(kernel_i + r) as usize];
-                for b in 0..buffers.len() {
-                    values[b] +=
-                        out_buffers[b].get(x, y - kernel_i as usize).unwrap() * kernel_value;
-                }
-            }
-
-            for i in 0..out_buffers.len() {
-                out_buffers[i].put(x, y, values[i]);
-            }
-        }
+#[cfg(rayon)]
+/*
+Hopefully a little faster?, naive optimisations.
+There's no test for this one so, we'll see how we go..
+*/
+pub fn guassian_blur_nband(
+    buffers: &mut [ImageBuffer],
+    sigma: f32,
+) -> error::Result<Vec<ImageBuffer>> {
+    if buffers.is_empty() {
+        return Err("No buffers provided");
     }
-    Ok(out_buffers)
+
+    let sig_squared = sigma.powi(2);
+    let radius = max!((3.0 * sigma).ceil(), 1.0) as usize;
+
+    let kernel_length = radius.powi(2) + 1;
+
+    let mut kernel = DnVec::zeros(kernel_length);
+
+    let r = radius as i32;
+
+    let sum: Dn = (-r..r)
+        .par_iter()
+        .map(|i| {
+            let exponent_numerator = -(i * i) as Dn;
+            let exponent_denominator = sig_squared.powi(2);
+
+            let e_expression =
+                (std::f32::consts::E as Dn).powf(exponent_numerator / exponent_denominator);
+
+            let kernel_value = e_expression / std::f32::consts::TAU * sig_squared;
+
+            kernel[(i + r) as usize] = kernel_value;
+
+            kernel_value
+        })
+        .sum();
+
+    // Normalize kernel
+    kernel.par_iter_mut().for_each(|i| {
+        *i /= sum;
+    });
+
+    // Setup some paralelle iterators, reusable epsilons and locks.
+    let buffer_width = buffers[0].width;
+    let buffer_height = buffers[0].height;
+    let buff_len: usize = buffers.len();
+
+    let width_iter = (0..buffer_width).into_par_iter();
+    let height_iter = (0..buffer_height).into_iter();
+
+    // Smart mutually exclusive smart pointer to allow for us to mutate the buffer across threads.
+    let m_buffers = Arc::new(Mutex::new(buffers.to_vec()));
+
+    // Without a test to run against it's hard to know if this will help, or just thrash heaps of mutex contention.
+    // in theory if the indexes into the buffer are guaranteed to be unique we shouldn't need locks at all..
+    // I will probs refactor this to an mpsc pattern to try that next.
+
+    // 1st pass: Horizontal Blur
+    width_iter.clone().for_each(|x| {
+        let m_c_buffers = m_buffers.clone(); // These are only clones of the pointer
+
+        height_iter.clone().for_each(|y| {
+            let values = (-r..r)
+                .filter(|&kernel_i| {
+                    x as i32 - kernel_i < 0 || x as i32 - kernel_i >= buffer_width as i32
+                })
+                .flat_map(|kernel_i| {
+                    let kernel_value = kernel[(kernel_i + r) as usize];
+                    (0..buff_len).map(move |b| {
+                        buffers[b].get(x - kernel_i as usize, y).unwrap() * kernel_value
+                    })
+                })
+                .fold(DnVec::zeros(buff_len), |acc, v| acc + v);
+
+            (0..buff_len).for_each(|i| {
+                let mut buffers = m_c_buffers.lock().unwrap();
+                buffers[i].put(x, y, values[i]);
+            });
+        });
+    });
+
+    // 2nd pass: Vertical Blur
+    width_iter.for_each(|x| {
+        let m_c_buffers = m_buffers.clone();
+        height_iter.clone().for_each(|y| {
+            //TODO: the logic is basically the same as the above so break out into a helper and pass in the variables.
+            let values = (-r..r)
+                .filter(|&kernel_i| {
+                    y as i32 - kernel_i < 0 || y as i32 - kernel_i >= buffer_height as i32
+                })
+                .flat_map(|kernel_i| {
+                    let kernel_value = kernel[(kernel_i + r) as usize];
+                    (0..buff_len).map(move |b| {
+                        buffers[b].get(y - kernel_i as usize, y).unwrap() * kernel_value
+                    })
+                })
+                .fold(DnVec::zeros(buff_len), |acc, v| acc + v);
+
+            (0..buff_len).for_each(|i| {
+                let mut buffers = m_c_buffers.lock().unwrap(); // Move mutable borrow outside of inner closure
+                buffers[i].put(x, y, values[i]);
+            });
+        });
+    });
+
+    Ok(buffers.into())
 }
 
 pub trait RgbImageBlur {
@@ -107,14 +219,14 @@ pub trait RgbImageBlur {
 impl RgbImageBlur for Image {
     fn guassian_blur(&mut self, sigma: f32) {
         let mut buffers = vec![];
-        for b in 0..self.num_bands() {
+        (0..self.num_bands()).for_each(|b| {
             buffers.push(self.get_band(b).to_owned());
-        }
+        });
 
-        if let Ok(buffers) = guassian_blur_nband(&buffers, sigma) {
-            for (b, _) in buffers.iter().enumerate() {
+        if let Ok(buffers) = guassian_blur_nband(&mut buffers, sigma) {
+            buffers.iter().enumerate().for_each(|(b, _)| {
                 self.set_band(&buffers[b], b);
-            }
+            });
         }
     }
 }

src/image.rs

@@ -143,11 +143,11 @@ impl Image {
     }
 
     pub fn open_str(file_path: &str) -> error::Result<Image> {
-        Image::open(&String::from(file_path))
+        Image::open(file_path)
     }
 
-    pub fn open(file_path: &String) -> error::Result<Image> {
-        if !path::file_exists(file_path.as_str()) {
+    pub fn open(file_path: &str) -> error::Result<Image> {
+        if !path::file_exists(file_path) {
             panic!("File not found: {}", file_path);
         }
 

src/matrix.rs

@@ -101,16 +101,16 @@ impl Matrix {
             let ai2 = a.m[(2 << 2) + row];
             let ai3 = a.m[(3 << 2) + row];
 
-            product[row] = ai0 * b.m[(0 << 2)] + ai1 * b.m[1] + ai2 * b.m[2] + ai3 * b.m[3];
-            product[(1 << 2) + row] = ai0 * b.m[(1 << 2)]
+            product[row] = ai0 * b.m[0 << 2] + ai1 * b.m[1] + ai2 * b.m[2] + ai3 * b.m[3];
+            product[(1 << 2) + row] = ai0 * b.m[1 << 2]
                 + ai1 * b.m[(1 << 2) + 1]
                 + ai2 * b.m[(1 << 2) + 2]
                 + ai3 * b.m[(1 << 2) + 3];
-            product[(2 << 2) + row] = ai0 * b.m[(2 << 2)]
+            product[(2 << 2) + row] = ai0 * b.m[2 << 2]
                 + ai1 * b.m[(2 << 2) + 1]
                 + ai2 * b.m[(2 << 2) + 2]
                 + ai3 * b.m[(2 << 2) + 3];
-            product[(3 << 2) + row] = ai0 * b.m[(3 << 2)]
+            product[(3 << 2) + row] = ai0 * b.m[3 << 2]
                 + ai1 * b.m[(3 << 2) + 1]
                 + ai2 * b.m[(3 << 2) + 2]
                 + ai3 * b.m[(3 << 2) + 3];