Merge branch 'filters-refactor' into 'main'

Refactor some of the filters code

See merge request GNOME/librsvg!753

4 files changed, 166 insertions, 115 deletions

diff --git a/src/drawing_ctx.rs b/src/drawing_ctx.rs
index 6eb200b3..3a617e06 100644
--- a/src/drawing_ctx.rs
+++ b/src/drawing_ctx.rs
@@ -970,7 +970,15 @@ impl DrawingCtx {
         current_color: RGBA,
         node_bbox: BoundingBox,
     ) -> Result<SharedImageSurface, RenderingError> {
-        let surface = if let Ok(specs) = filter_list
+        // We try to convert each item in the filter_list to a FilterSpec.
+        //
+        // However, the spec mentions, "If the filter references a non-existent object or
+        // the referenced object is not a filter element, then the whole filter chain is
+        // ignored." - https://www.w3.org/TR/filter-effects/#FilterProperty
+        //
+        // So, run through the filter_list and collect into a Result<Vec<FilterSpec>>.
+        // This will return an Err if any of the conversions failed.
+        let filter_specs = filter_list
             .iter()
             .map(|filter_value| {
                 filter_value.to_filter_spec(
@@ -981,25 +989,37 @@ impl DrawingCtx {
                     node_name,
                 )
             })
-            .collect::<Result<Vec<FilterSpec>, _>>()
-        {
-            specs.iter().try_fold(surface_to_filter, |surface, spec| {
-                filters::render(
-                    spec,
-                    stroke_paint_source.clone(),
-                    fill_paint_source.clone(),
-                    surface,
-                    acquired_nodes,
-                    self,
-                    self.get_transform(),
-                    node_bbox,
-                )
-            })?
-        } else {
-            surface_to_filter
-        };
+            .collect::<Result<Vec<FilterSpec>, _>>();
+
+        match filter_specs {
+            Ok(specs) => {
+                // Start with the surface_to_filter, and apply each filter spec in turn;
+                // the final result is our return value.
+                specs.iter().try_fold(surface_to_filter, |surface, spec| {
+                    filters::render(
+                        spec,
+                        stroke_paint_source.clone(),
+                        fill_paint_source.clone(),
+                        surface,
+                        acquired_nodes,
+                        self,
+                        self.get_transform(),
+                        node_bbox,
+                    )
+                })
+            }
 
-        Ok(surface)
+            Err(e) => {
+                rsvg_log!(
+                    self.session,
+                    "not rendering filter list on node {} because it was in error: {}",
+                    node_name,
+                    e
+                );
+                // just return the original surface without filtering it
+                Ok(surface_to_filter)
+            }
+        }
     }
 
     fn set_gradient(&mut self, gradient: &UserSpaceGradient) -> Result<(), cairo::Error> {
diff --git a/src/filter.rs b/src/filter.rs
index 0e0180a3..963b963c 100644
--- a/src/filter.rs
+++ b/src/filter.rs
@@ -6,13 +6,13 @@ use std::slice::Iter;
 
 use crate::coord_units::CoordUnits;
 use crate::document::{AcquiredNodes, NodeId};
-use crate::drawing_ctx::DrawingCtx;
+use crate::drawing_ctx::{DrawingCtx, ViewParams};
 use crate::element::{Draw, Element, ElementResult, SetAttributes};
 use crate::error::ValueErrorKind;
 use crate::filter_func::FilterFunction;
-use crate::filters::{extract_filter_from_filter_node, FilterResolveError, FilterSpec};
+use crate::filters::{FilterResolveError, FilterSpec, UserSpacePrimitive};
 use crate::length::*;
-use crate::node::NodeBorrow;
+use crate::node::{Node, NodeBorrow};
 use crate::parsers::{Parse, ParseValue};
 use crate::rect::Rect;
 use crate::session::Session;
@@ -114,12 +114,114 @@ impl FilterValue {
             ),
 
             FilterValue::Function(ref func) => {
-                func.to_filter_spec(user_space_params, current_color)
+                Ok(func.to_filter_spec(user_space_params, current_color))
             }
         }
     }
 }
 
+/// Holds the viewport parameters for both objectBoundingBox and userSpaceOnUse units.
+///
+/// When collecting a set of filter primitives (`feFoo`) into a [`FilterSpec`], which is
+/// in user space, we need to convert each primitive's units into user space units.  So,
+/// pre-compute both cases and pass them around.
+///
+/// This struct needs a better name; I didn't want to make it seem specific to filters by
+/// calling `FiltersViewParams` or `FilterCollectionProcessViewParams`.  Maybe the
+/// original [`ViewParams`] should be this struct, with both cases included...
+pub struct ViewParamsGen {
+    object_bounding_box: ViewParams,
+    user_space_on_use: ViewParams,
+}
+
+impl ViewParamsGen {
+    pub fn new(draw_ctx: &DrawingCtx) -> Self {
+        ViewParamsGen {
+            object_bounding_box: draw_ctx.get_view_params_for_units(CoordUnits::ObjectBoundingBox),
+            user_space_on_use: draw_ctx.get_view_params_for_units(CoordUnits::UserSpaceOnUse),
+        }
+    }
+
+    fn get(&self, units: CoordUnits) -> &ViewParams {
+        match units {
+            CoordUnits::ObjectBoundingBox => &self.object_bounding_box,
+            CoordUnits::UserSpaceOnUse => &self.user_space_on_use,
+        }
+    }
+}
+
+fn extract_filter_from_filter_node(
+    filter_node: &Node,
+    acquired_nodes: &mut AcquiredNodes<'_>,
+    session: &Session,
+    filter_view_params: &ViewParamsGen,
+) -> Result<FilterSpec, FilterResolveError> {
+    assert!(is_element_of_type!(filter_node, Filter));
+
+    let filter_element = filter_node.borrow_element();
+
+    let user_space_filter = {
+        let filter_values = filter_element.get_computed_values();
+
+        let filter = borrow_element_as!(filter_node, Filter);
+
+        filter.to_user_space(&NormalizeParams::new(
+            filter_values,
+            filter_view_params.get(filter.get_filter_units()),
+        ))
+    };
+
+    let primitive_view_params = filter_view_params.get(user_space_filter.primitive_units);
+
+    let primitives = filter_node
+        .children()
+        .filter(|c| c.is_element())
+        // Skip nodes in error.
+        .filter(|c| {
+            let in_error = c.borrow_element().is_in_error();
+
+            if in_error {
+                rsvg_log!(
+                    session,
+                    "(ignoring filter primitive {} because it is in error)",
+                    c
+                );
+            }
+
+            !in_error
+        })
+        // Keep only filter primitives (those that implement the Filter trait)
+        .filter(|c| c.borrow_element().as_filter_effect().is_some())
+        .map(|primitive_node| {
+            let elt = primitive_node.borrow_element();
+            let effect = elt.as_filter_effect().unwrap();
+
+            let primitive_name = format!("{}", primitive_node);
+
+            let primitive_values = elt.get_computed_values();
+            let params = NormalizeParams::new(primitive_values, primitive_view_params);
+
+            effect
+                .resolve(acquired_nodes, &primitive_node)
+                .map_err(|e| {
+                    rsvg_log!(
+                        session,
+                        "(filter primitive {} returned an error: {})",
+                        primitive_name,
+                        e
+                    );
+                    e
+                })
+                .map(|primitive| primitive.into_user_space(&params))
+        })
+        .collect::<Result<Vec<UserSpacePrimitive>, FilterResolveError>>()?;
+
+    Ok(FilterSpec {
+        user_space_filter,
+        primitives,
+    })
+}
+
 fn filter_spec_from_filter_node(
     acquired_nodes: &mut AcquiredNodes<'_>,
     draw_ctx: &DrawingCtx,
@@ -128,6 +230,8 @@ fn filter_spec_from_filter_node(
 ) -> Result<FilterSpec, FilterResolveError> {
     let session = draw_ctx.session().clone();
 
+    let filter_view_params = ViewParamsGen::new(draw_ctx);
+
     acquired_nodes
         .acquire(node_id)
         .map_err(|e| {
@@ -155,7 +259,12 @@ fn filter_spec_from_filter_node(
                         );
                         Err(FilterResolveError::ChildNodeInError)
                     } else {
-                        extract_filter_from_filter_node(node, acquired_nodes, draw_ctx)
+                        extract_filter_from_filter_node(
+                            node,
+                            acquired_nodes,
+                            &session,
+                            &filter_view_params,
+                        )
                     }
                 }
 
diff --git a/src/filter_func.rs b/src/filter_func.rs
index e931c8b1..26dcd810 100644
--- a/src/filter_func.rs
+++ b/src/filter_func.rs
@@ -24,7 +24,7 @@ use crate::filters::{
     gaussian_blur::GaussianBlur,
     merge::{Merge, MergeNode},
     offset::Offset,
-    FilterResolveError, FilterSpec, Input, Primitive, PrimitiveParams, ResolvedPrimitive,
+    FilterSpec, Input, Primitive, PrimitiveParams, ResolvedPrimitive,
 };
 use crate::length::*;
 use crate::paint_server::resolve_color;
@@ -638,22 +638,18 @@ impl Parse for FilterFunction {
 impl FilterFunction {
     // If this function starts actually returning an Err, remove this Clippy exception:
     #[allow(clippy::unnecessary_wraps)]
-    pub fn to_filter_spec(
-        &self,
-        params: &NormalizeParams,
-        current_color: RGBA,
-    ) -> Result<FilterSpec, FilterResolveError> {
+    pub fn to_filter_spec(&self, params: &NormalizeParams, current_color: RGBA) -> FilterSpec {
         match self {
-            FilterFunction::Blur(v) => Ok(v.to_filter_spec(params)),
-            FilterFunction::Brightness(v) => Ok(v.to_filter_spec(params)),
-            FilterFunction::Contrast(v) => Ok(v.to_filter_spec(params)),
-            FilterFunction::DropShadow(v) => Ok(v.to_filter_spec(params, current_color)),
-            FilterFunction::Grayscale(v) => Ok(v.to_filter_spec(params)),
-            FilterFunction::HueRotate(v) => Ok(v.to_filter_spec(params)),
-            FilterFunction::Invert(v) => Ok(v.to_filter_spec(params)),
-            FilterFunction::Opacity(v) => Ok(v.to_filter_spec(params)),
-            FilterFunction::Saturate(v) => Ok(v.to_filter_spec(params)),
-            FilterFunction::Sepia(v) => Ok(v.to_filter_spec(params)),
+            FilterFunction::Blur(v) => v.to_filter_spec(params),
+            FilterFunction::Brightness(v) => v.to_filter_spec(params),
+            FilterFunction::Contrast(v) => v.to_filter_spec(params),
+            FilterFunction::DropShadow(v) => v.to_filter_spec(params, current_color),
+            FilterFunction::Grayscale(v) => v.to_filter_spec(params),
+            FilterFunction::HueRotate(v) => v.to_filter_spec(params),
+            FilterFunction::Invert(v) => v.to_filter_spec(params),
+            FilterFunction::Opacity(v) => v.to_filter_spec(params),
+            FilterFunction::Saturate(v) => v.to_filter_spec(params),
+            FilterFunction::Sepia(v) => v.to_filter_spec(params),
         }
     }
 }
diff --git a/src/filters/mod.rs b/src/filters/mod.rs
index 6a67fe06..b7b545b7 100644
--- a/src/filters/mod.rs
+++ b/src/filters/mod.rs
@@ -12,7 +12,7 @@ use crate::element::{Draw, ElementResult, SetAttributes};
 use crate::error::{ElementError, ParseError, RenderingError};
 use crate::filter::UserSpaceFilter;
 use crate::length::*;
-use crate::node::{Node, NodeBorrow};
+use crate::node::Node;
 use crate::paint_server::UserSpacePaintSource;
 use crate::parsers::{CustomIdent, Parse, ParseValue};
 use crate::properties::ColorInterpolationFilters;
@@ -241,80 +241,6 @@ impl Primitive {
     }
 }
 
-pub fn extract_filter_from_filter_node(
-    filter_node: &Node,
-    acquired_nodes: &mut AcquiredNodes<'_>,
-    draw_ctx: &DrawingCtx,
-) -> Result<FilterSpec, FilterResolveError> {
-    let session = draw_ctx.session().clone();
-
-    assert!(is_element_of_type!(filter_node, Filter));
-
-    let filter_element = filter_node.borrow_element();
-
-    let user_space_filter = {
-        let filter_values = filter_element.get_computed_values();
-
-        let filter = borrow_element_as!(filter_node, Filter);
-
-        filter.to_user_space(&NormalizeParams::new(
-            filter_values,
-            &draw_ctx.get_view_params_for_units(filter.get_filter_units()),
-        ))
-    };
-
-    let primitives = filter_node
-        .children()
-        .filter(|c| c.is_element())
-        // Skip nodes in error.
-        .filter(|c| {
-            let in_error = c.borrow_element().is_in_error();
-
-            if in_error {
-                rsvg_log!(
-                    session,
-                    "(ignoring filter primitive {} because it is in error)",
-                    c
-                );
-            }
-
-            !in_error
-        })
-        // Keep only filter primitives (those that implement the Filter trait)
-        .filter(|c| c.borrow_element().as_filter_effect().is_some())
-        .map(|primitive_node| {
-            let elt = primitive_node.borrow_element();
-            let effect = elt.as_filter_effect().unwrap();
-
-            let primitive_name = format!("{}", primitive_node);
-
-            let primitive_values = elt.get_computed_values();
-            let params = NormalizeParams::new(
-                primitive_values,
-                &draw_ctx.get_view_params_for_units(user_space_filter.primitive_units),
-            );
-
-            effect
-                .resolve(acquired_nodes, &primitive_node)
-                .map_err(|e| {
-                    rsvg_log!(
-                        session,
-                        "(filter primitive {} returned an error: {})",
-                        primitive_name,
-                        e
-                    );
-                    e
-                })
-                .map(|primitive| primitive.into_user_space(&params))
-        })
-        .collect::<Result<Vec<UserSpacePrimitive>, FilterResolveError>>()?;
-
-    Ok(FilterSpec {
-        user_space_filter,
-        primitives,
-    })
-}
-
 /// Applies a filter and returns the resulting surface.
 pub fn render(
     filter: &FilterSpec,