From e3689f2b8cb1c718476d4e656b6c096b1a8cd489 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 1 Nov 2023 12:46:42 -0700 Subject: [PATCH 1/3] Add some important comments to some of the FilteContents methods --- .../entity/contents/filters/filter_contents.h | 43 ++++++++++++++----- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/impeller/entity/contents/filters/filter_contents.h b/impeller/entity/contents/filters/filter_contents.h index f06c17c19364c..0b93c6bc83a73 100644 --- a/impeller/entity/contents/filters/filter_contents.h +++ b/impeller/entity/contents/filters/filter_contents.h @@ -132,17 +132,23 @@ class FilterContents : public Contents { // |Contents| const FilterContents* AsFilter() const override; - /// @brief Determines the coverage of source pixels that will be needed - /// to apply this filter under the given transform and produce - /// results anywhere within the indicated coverage limit. + /// @brief Determines the coverage of source pixels that will be needed + /// to produce results for the indicated output limit. This is a + /// reverse bounds calculation computing a source coverage from + /// an intended output coverage. /// - /// This is useful for subpass rendering scenarios where a filter - /// will be applied to the output of the subpass and we need to - /// determine how large of a render target to allocate in order - /// to collect all pixels that might affect the supplied output - /// coverage limit. While we might clip the rendering of the subpass, - /// we want to avoid clipping out any pixels that contribute to - /// the output limit via the filtering operation. + /// The method computes a result such that if the filter is applied + /// to a set of pixels filling the computed source coverage, it + /// should produce an output that covers the entire indicated + /// |output_limit|. + /// + /// This is useful for subpass rendering scenarios where a filter + /// will be applied to the output of the subpass and we need to + /// determine how large of a render target to allocate in order + /// to collect all pixels that might affect the supplied output + /// coverage limit. While we might clip the rendering of the subpass, + /// we want to avoid clipping out any pixels that contribute to + /// the output limit via the filtering operation. std::optional GetSourceCoverage(const Matrix& effect_transform, const Rect& output_limit) const; @@ -178,11 +184,28 @@ class FilterContents : public Contents { virtual void SetRenderingMode(Entity::RenderingMode rendering_mode); private: + /// @brief Internal utility method for |GetLocalCoverage|. + /// + /// |GetLocalCoverage| will intersect the filter's coverage as + /// computed by this method with the oject's coverage hint. + /// This method should compute the coverage of the associated + /// inputs and then apply any modifications to that coverage + /// representing the actions of the filtering operation, ignoring + /// the coverage hint. virtual std::optional GetFilterCoverage( const FilterInput::Vector& inputs, const Entity& entity, const Matrix& effect_transform) const; + /// @brief Internal utility method for |GetSourceCoverage|. + /// + /// The public method will use this method to compute the inverse + /// coverage for the indicated |output_limit| and then go on to + /// consult the inputs to further expand/refine the required + /// source coverage. + /// This method is only responsible for computing the inverse + /// coverage for the filtering operation based on the indicated + /// |output_coverage|, ignoring the inputs. virtual std::optional GetFilterSourceCoverage( const Matrix& effect_transform, const Rect& output_limit) const = 0; From 70be55635b076cd534373c5c0fc68e8a6268bbdd Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 1 Nov 2023 13:13:31 -0700 Subject: [PATCH 2/3] clarify transform in GetSourceCoverage method --- impeller/entity/contents/filters/filter_contents.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/impeller/entity/contents/filters/filter_contents.h b/impeller/entity/contents/filters/filter_contents.h index 0b93c6bc83a73..af5d03c5bc252 100644 --- a/impeller/entity/contents/filters/filter_contents.h +++ b/impeller/entity/contents/filters/filter_contents.h @@ -133,9 +133,10 @@ class FilterContents : public Contents { const FilterContents* AsFilter() const override; /// @brief Determines the coverage of source pixels that will be needed - /// to produce results for the indicated output limit. This is a - /// reverse bounds calculation computing a source coverage from - /// an intended output coverage. + /// to produce results for the indicated |output_limit| under the + /// indicated |effect_transform|. This is essentially a reverse of + /// the |GetCoverage| method computing a source coverage from + /// an intended |output_limit| coverage. /// /// The method computes a result such that if the filter is applied /// to a set of pixels filling the computed source coverage, it From 7b136ff8c2ae50acc91720cb76c62762257f9a24 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 1 Nov 2023 14:33:53 -0700 Subject: [PATCH 3/3] updates from feedback --- .../entity/contents/filters/filter_contents.h | 53 +++++++++++-------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/impeller/entity/contents/filters/filter_contents.h b/impeller/entity/contents/filters/filter_contents.h index af5d03c5bc252..544080a6c5489 100644 --- a/impeller/entity/contents/filters/filter_contents.h +++ b/impeller/entity/contents/filters/filter_contents.h @@ -133,23 +133,35 @@ class FilterContents : public Contents { const FilterContents* AsFilter() const override; /// @brief Determines the coverage of source pixels that will be needed - /// to produce results for the indicated |output_limit| under the - /// indicated |effect_transform|. This is essentially a reverse of + /// to produce results for the specified |output_limit| under the + /// specified |effect_transform|. This is essentially a reverse of /// the |GetCoverage| method computing a source coverage from /// an intended |output_limit| coverage. /// + /// Both the |output_limit| and the return value are in the + /// transformed coordinate space, and so do not need to be + /// transformed or inverse transformed by the |effect_transform| + /// but individual parameters on the filter might be in the + /// untransformed space and should be transformed by the + /// |effect_transform| before applying them to the coverages. + /// /// The method computes a result such that if the filter is applied /// to a set of pixels filling the computed source coverage, it - /// should produce an output that covers the entire indicated + /// should produce an output that covers the entire specified /// |output_limit|. /// /// This is useful for subpass rendering scenarios where a filter /// will be applied to the output of the subpass and we need to /// determine how large of a render target to allocate in order /// to collect all pixels that might affect the supplied output - /// coverage limit. While we might clip the rendering of the subpass, - /// we want to avoid clipping out any pixels that contribute to - /// the output limit via the filtering operation. + /// coverage limit. While we might end up clipping the rendering + /// of the subpass to its destination, we want to avoid clipping + /// out any pixels that contribute to the output limit via the + /// filtering operation. + /// + /// @return The coverage bounds in the transformed space of any source pixel + /// that may be needed to produce output for the indicated filter + /// that covers the indicated |output_limit|. std::optional GetSourceCoverage(const Matrix& effect_transform, const Rect& output_limit) const; @@ -185,28 +197,18 @@ class FilterContents : public Contents { virtual void SetRenderingMode(Entity::RenderingMode rendering_mode); private: - /// @brief Internal utility method for |GetLocalCoverage|. - /// - /// |GetLocalCoverage| will intersect the filter's coverage as - /// computed by this method with the oject's coverage hint. - /// This method should compute the coverage of the associated - /// inputs and then apply any modifications to that coverage - /// representing the actions of the filtering operation, ignoring - /// the coverage hint. + /// @brief Internal utility method for |GetLocalCoverage| that computes + /// the output coverage of this filter across the specified inputs, + /// ignoring the coverage hint. virtual std::optional GetFilterCoverage( const FilterInput::Vector& inputs, const Entity& entity, const Matrix& effect_transform) const; - /// @brief Internal utility method for |GetSourceCoverage|. - /// - /// The public method will use this method to compute the inverse - /// coverage for the indicated |output_limit| and then go on to - /// consult the inputs to further expand/refine the required - /// source coverage. - /// This method is only responsible for computing the inverse - /// coverage for the filtering operation based on the indicated - /// |output_coverage|, ignoring the inputs. + /// @brief Internal utility method for |GetSourceCoverage| that computes + /// the inverse effect of this transform on the specified output + /// coverage, ignoring the inputs which will be accommodated by + /// the caller. virtual std::optional GetFilterSourceCoverage( const Matrix& effect_transform, const Rect& output_limit) const = 0; @@ -220,6 +222,11 @@ class FilterContents : public Contents { const Rect& coverage, const std::optional& coverage_hint) const = 0; + /// @brief Internal utility method to compute the coverage of this + /// filter across its internally specified inputs and subject + /// to the coverage hint. + /// + /// Uses |GetFilterCoverage|. std::optional GetLocalCoverage(const Entity& local_entity) const; FilterInput::Vector inputs_;