Refactor Map and Plugin error handling, improve safety and usability

- Updated Map::new() to return a Result type, handling creation failures.
- Enhanced error enumeration in MapError and PluginError for better clarity.
- Modified key retrieval and string conversion methods to return Result types, improving error handling.
- Adjusted tests to accommodate changes in Map creation and error handling.
- Refactored Plugin function invocation to return Result types, ensuring better error propagation.
- Removed unnecessary panic calls and replaced them with proper error handling.
- Improved iterator methods to handle potential errors gracefully.

Author: animafps

rspipe/src/main.rs

@@ -138,7 +138,7 @@ fn main() {
             }
         }
 
-        let mut vars_map = Map::new();
+        let mut vars_map = Map::new().unwrap();
         for (key, value) in script_args {
             if let Err(e) = vars_map.set(&key, &value) {
                 eprintln!("Failed to set script variable {}: {}", key, e);
@@ -181,11 +181,11 @@ fn main() {
     };
 
     // Determine frame range
-    let start_frame = matches.get_one::<usize>("start").copied().unwrap_or(0);
+    let start_frame = matches.get_one::<i32>("start").copied().unwrap_or(0);
     let end_frame = matches
-        .get_one::<usize>("end")
+        .get_one::<i32>("end")
         .copied()
-        .unwrap_or((video_info.num_frames - 1) as usize);
+        .unwrap_or(video_info.num_frames - 1);
 
     if start_frame > end_frame {
         eprintln!("Start frame cannot be greater than end frame");
@@ -236,15 +236,15 @@ fn main() {
 fn process_frames_concurrent(
     node: &rustsynth::node::Node,
     writer: &mut OutputWriter,
-    start_frame: usize,
-    end_frame: usize,
+    start_frame: i32,
+    end_frame: i32,
     num_requests: usize,
     progress: &mut ProgressTracker,
 ) {
     use std::sync::mpsc;
 
     let total_frames = end_frame - start_frame + 1;
-    let (tx, rx) = mpsc::channel::<(usize, Result<rustsynth::frame::Frame, String>)>();
+    let (tx, rx) = mpsc::channel::<(i32, Result<rustsynth::frame::Frame, String>)>();
     let node_clone = node.clone();
 
     // Track pending requests
@@ -254,7 +254,7 @@ fn process_frames_concurrent(
     let mut next_request = start_frame;
 
     // Request initial batch
-    for _ in 0..num_requests.min(total_frames) {
+    for _ in 0..num_requests.min(total_frames.try_into().unwrap()) {
         *pending_requests.lock().unwrap() += 1;
         let tx_clone = tx.clone();
         let node_clone = node_clone.clone();

rspipe/src/progress.rs

@@ -2,14 +2,14 @@ use std::io::{self, Write};
 use std::time::Instant;
 
 pub struct ProgressTracker {
-    total_frames: usize,
+    total_frames: i32,
     start_time: Instant,
     last_update: Instant,
     verbose: bool,
 }
 
 impl ProgressTracker {
-    pub fn new(total_frames: usize, verbose: bool) -> Self {
+    pub fn new(total_frames: i32, verbose: bool) -> Self {
         let now = Instant::now();
         ProgressTracker {
             total_frames,
@@ -19,7 +19,7 @@ impl ProgressTracker {
         }
     }
 
-    pub fn update(&mut self, completed_frames: usize) {
+    pub fn update(&mut self, completed_frames: i32) {
         let now = Instant::now();
 
         // Only update progress every 100ms to avoid spam

rustsynth/src/core.rs

@@ -5,21 +5,41 @@ use crate::{
     format::VideoFormat,
     frame::{Frame, FrameContext},
     log::{log_handler_callback, LogHandle, LogHandler, MessageType},
-    map::Map,
+    map::{Map, MapError},
+    node::Node,
     plugin::Plugin,
 };
 use bitflags::bitflags;
-use core::fmt;
 use rustsynth_sys as ffi;
+use std::fmt;
 use std::{
-    ffi::{CStr, CString},
+    ffi::{CStr, CString, NulError},
     marker::PhantomData,
     ptr::NonNull,
 };
+use thiserror::Error;
 
 #[cfg(test)]
 mod tests;
 
+/// The error type for `Core` operations.
+#[derive(Error, Debug)]
+pub enum CoreError {
+    #[error("Failed to create map: {0}")]
+    MapCreationFailed(#[from] MapError),
+    #[error("Invalid filter name: {0}")]
+    InvalidFilterName(#[from] NulError),
+    #[error("Failed to create video filter")]
+    VideoFilterCreationFailed,
+    #[error("Failed to create audio filter")]
+    AudioFilterCreationFailed,
+    #[error("{0}")]
+    Custom(String),
+}
+
+/// A specialized `Result` type for `Core` operations.
+pub type CoreResult<T> = Result<T, CoreError>;
+
 bitflags! {
     /// Options when creating a core.
     pub struct CoreCreationFlags: u32 {
@@ -208,13 +228,13 @@ impl<'core> CoreRef<'core> {
     }
 
     /// Create a video filter using the Filter trait
-    pub fn create_video_filter<F>(&self, filter: &F) -> Result<Map<'_>, String>
+    pub fn create_video_filter<F>(&self, filter: &F) -> CoreResult<Map<'_>>
     where
         F: Filter<'core>,
     {
-        let out = Map::new();
+        let out = Map::new()?;
         // Get video info from the filter
-        let video_info = filter.get_video_info()?;
+        let video_info = filter.get_video_info().map_err(CoreError::Custom)?;
         let dependencies = filter.get_dependencies();
 
         // Convert dependencies to FFI format
@@ -226,7 +246,7 @@ impl<'core> CoreRef<'core> {
         let instance_data = Box::into_raw(filter_box) as *mut std::ffi::c_void;
 
         // Create C strings for name
-        let name_cstr = CString::new(F::NAME).map_err(|_| "Invalid filter name")?;
+        let name_cstr = CString::new(F::NAME)?;
 
         unsafe {
             API::get_cached().create_video_filter(
@@ -247,12 +267,12 @@ impl<'core> CoreRef<'core> {
     }
 
     /// Create a video filter using the Filter trait (returns node directly)
-    pub fn create_video_filter2<F>(&self, filter: &F) -> Result<crate::node::Node<'core>, String>
+    pub fn create_video_filter2<F>(&self, filter: &F) -> CoreResult<crate::node::Node<'core>>
     where
         F: Filter<'core>,
     {
         // Get video info from the filter
-        let video_info = filter.get_video_info()?;
+        let video_info = filter.get_video_info().map_err(CoreError::Custom)?;
         let dependencies = filter.get_dependencies();
 
         // Convert dependencies to FFI format
@@ -264,7 +284,7 @@ impl<'core> CoreRef<'core> {
         let instance_data = Box::into_raw(filter_box) as *mut std::ffi::c_void;
 
         // Create C strings for name
-        let name_cstr = CString::new(F::NAME).map_err(|_| "Invalid filter name")?;
+        let name_cstr = CString::new(F::NAME)?;
 
         let node_ptr = unsafe {
             API::get_cached().create_video_filter2(
@@ -281,20 +301,20 @@ impl<'core> CoreRef<'core> {
         };
 
         if node_ptr.is_null() {
-            return Err("Failed to create video filter".to_string());
+            return Err(CoreError::VideoFilterCreationFailed);
         }
 
         Ok(unsafe { crate::node::Node::from_ptr(node_ptr) })
     }
 
     /// Create a audio filter using the Filter trait
-    pub fn create_audio_filter<F>(&self, filter: &F) -> Result<Map<'_>, String>
+    pub fn create_audio_filter<F>(&self, filter: &F) -> CoreResult<Map<'_>>
     where
         F: Filter<'core>,
     {
-        let out = Map::new();
+        let out = Map::new()?;
         // Get audio info from the filter
-        let audio_info = filter.get_audio_info()?;
+        let audio_info = filter.get_audio_info().map_err(CoreError::Custom)?;
         let dependencies = filter.get_dependencies();
 
         // Convert dependencies to FFI format
@@ -306,7 +326,7 @@ impl<'core> CoreRef<'core> {
         let instance_data = Box::into_raw(filter_box) as *mut std::ffi::c_void;
 
         // Create C strings for name
-        let name_cstr = CString::new(F::NAME).map_err(|_| "Invalid filter name")?;
+        let name_cstr = CString::new(F::NAME)?;
 
         unsafe {
             API::get_cached().create_audio_filter(
@@ -327,12 +347,12 @@ impl<'core> CoreRef<'core> {
     }
 
     /// Create an audio filter using the Filter trait (returns node directly)
-    pub fn create_audio_filter2<F>(&self, filter: &F) -> Result<crate::node::Node<'core>, String>
+    pub fn create_audio_filter2<F>(&self, filter: &F) -> CoreResult<Node<'core>>
     where
         F: Filter<'core>,
     {
         // Get audio info from the filter
-        let audio_info = filter.get_audio_info()?;
+        let audio_info = filter.get_audio_info().map_err(CoreError::Custom)?;
         let dependencies = filter.get_dependencies();
 
         // Convert dependencies to FFI format
@@ -344,7 +364,7 @@ impl<'core> CoreRef<'core> {
         let instance_data = Box::into_raw(filter_box) as *mut std::ffi::c_void;
 
         // Create C strings for name
-        let name_cstr = CString::new(F::NAME).map_err(|_| "Invalid filter name")?;
+        let name_cstr = CString::new(F::NAME)?;
 
         let node_ptr = unsafe {
             API::get_cached().create_audio_filter2(
@@ -361,7 +381,7 @@ impl<'core> CoreRef<'core> {
         };
 
         if node_ptr.is_null() {
-            return Err("Failed to create audio filter".to_string());
+            return Err(CoreError::AudioFilterCreationFailed);
         }
 
         Ok(unsafe { crate::node::Node::from_ptr(node_ptr) })

rustsynth/src/lib.rs

@@ -59,7 +59,7 @@ pub fn api_version() -> i32 {
 macro_rules! owned_map {
     ($({$key:literal: $x:expr }),*) => {
         {
-            let mut temp_map = $crate::map::Map::new();
+            let mut temp_map = $crate::map::Map::new().unwrap();
             $(
                 temp_map.set($key, $x).unwrap();
             )*

rustsynth/src/map/errors.rs

@@ -16,8 +16,12 @@ pub enum MapError {
     InvalidKey(#[from] InvalidKeyError),
     #[error("Couldn't convert to a CString")]
     CStringConversion(#[from] NulError),
+    #[error("Failed to create map")]
+    CreationFailed,
     #[error("Unknown error (see Map::error())")]
     Error,
+    #[error("UTF-8 conversion error: {0}")]
+    Utf8Error(#[from] std::string::FromUtf8Error),
 }
 
 impl From<MapError> for String {

rustsynth/src/map/iterators.rs

@@ -29,7 +29,7 @@ impl<'map, 'elem> Iterator for Keys<'map, 'elem> {
             return None;
         }
 
-        let key = self.map.key(self.index);
+        let key = self.map.key(self.index).ok()?;
         self.index += 1;
         Some(key)
     }

rustsynth/src/map/mod.rs

@@ -88,16 +88,14 @@ fn handle_append_prop_error(error: i32) -> MapResult<()> {
 }
 
 impl<'elem> Map<'elem> {
-    pub fn new() -> Self {
+    pub fn new() -> MapResult<Self> {
         let handle = unsafe { API::get_cached().create_map() };
-        if handle.is_null() {
-            panic!("Failed to create VSMap");
-        }
+        let handle = NonNull::new(handle).ok_or(MapError::CreationFailed)?;
 
-        Self {
-            handle: NonNull::new(handle).unwrap(),
+        Ok(Self {
+            handle,
             _elem: PhantomData,
-        }
+        })
     }
 
     pub const fn as_ptr(&self) -> *mut ffi::VSMap {
@@ -150,7 +148,7 @@ impl<'elem> Map<'elem> {
     #[inline]
     pub(crate) fn make_raw_key(key: &str) -> MapResult<CString> {
         Map::is_key_valid(key)?;
-        Ok(CString::new(key).unwrap())
+        Ok(CString::new(key)?)
     }
 
     /// Clears the map.
@@ -163,14 +161,10 @@ impl<'elem> Map<'elem> {
 
     /// Returns the error message contained in the map, if any.
     #[inline]
-    pub fn error(&'_ self) -> Option<Cow<'_, str>> {
+    pub fn error(&'_ self) -> Result<&str, std::str::Utf8Error> {
         let error_message = unsafe { API::get_cached().map_get_error(self) };
-        if error_message.is_null() {
-            return None;
-        }
-
         let error_message = unsafe { CStr::from_ptr(error_message) };
-        Some(error_message.to_string_lossy())
+        error_message.to_str()
     }
 
     /// Adds an error message to a map. The map is cleared first.
@@ -208,8 +202,8 @@ impl<'elem> Map<'elem> {
     /// # Panics
     /// Panics if `index >= self.key_count()`.
     #[inline]
-    pub fn key(&self, index: usize) -> &str {
-        self.key_raw(index).to_str().unwrap()
+    pub fn key(&self, index: usize) -> Result<&str, std::str::Utf8Error> {
+        self.key_raw(index).to_str()
     }
 
     /// Returns an iterator over all keys in a map.
@@ -1088,11 +1082,3 @@ impl<'elem> Map<'elem> {
 pub trait IntoOwnedMap {
     fn into_owned_map<'elem>(self) -> Map<'elem>;
 }
-
-
-impl Default for Map<'_> {
-    #[inline]
-    fn default() -> Self {
-        Self::new()
-    }
-}