Add some important comments to some of the FilteContents methods

[flar]

Fixes flutter/flutter#137703

Also clarified and fixed indentation on docstring comment for the public method FilterContents::GetSourceCoverage

[gaaclarke]

So, it's the inverse of GetCoverage?

[flar]

Correct. Is that confusing from what I wrote? Or are you suggesting a simplification?

[flar]

Oops, I lost the mention of the transform. Minor update coming...

[flar]

How about:

  /// @brief  Determines the coverage of source pixels that will be needed
  ///         to produce results for the indicated output coverage under the
  ///         indicated effect transform. This is essentially a reverse of
  ///         the |GetCoverage| method computing a source coverage from
  ///         an intended output coverage.

(I'm also renaming the output_limit parameter to output_coverage to match that text)

[flar]

Ugh, renaming the parameter and propagating it to all of the downstream methods would touch a bunch of files unnecessarily, so I'll change the comment to match instead.

[gaaclarke]

It was just a genuine question. That doesn't match my understanding after having looked at DirectionalGaussianBlurFilterContents.

[flar]

Perhaps you are confused by the non-intuitive fact that blur filters expand in both directions. Just as a given edge pixel can be the input for a fringe around the source and so it expands when you apply the filter, each pixel takes input from a range of pixels around it so you need more source pixels than output pixels in order to compute the values correctly.

So, the gaussian filter is computing a reverse bounds by expanding it...

[gaaclarke]

As per our discussion in the chat. If you want to format these differently. I'm fine with that. Here is an example of implicit brief/details that doesn't use extra whitespace.

/// This is brief.
/// This is a details, lots and lots of details.
/// Here's even more details.
void Foobar();

[gaaclarke]

Suggested change

	/// to produce results for the indicated output limit. This is a
	/// to produce results for the indicated `output_limit`. This is a

[flar]

I used the |output_limit| notation in my latest push, which is what I've used elsewhere for parameters. Is that correct?

[gaaclarke]

This never defines what effect_transform is. Looking at the implementation of some GetFilterSourceCoverage i know that only the basis is used. That should be mentioned.

For DirectionalGaussianBlurFilterContents::GetFilterSourceCoverage shouldn't the result of GetFilterSourceCoverage be smaller than output_limit since the source will not have the blur radius? It is actually returning a larger result.

The problem I ran into was this:

TEST(DirectionalGaussianBlurFilterContents, FilterSourceCoverage) {
  Scalar sigma_radius_1 = CalculateSigmaForBlurRadius(1.0);
  auto contents = std::make_unique<DirectionalGaussianBlurFilterContents>();
  contents->SetSigma(Sigma{sigma_radius_1});
  contents->SetDirection({1.0, 0.0});
  std::optional<Rect> coverage = contents->GetFilterSourceCoverage(
      /*effect_transform=*/Matrix::MakeScale({2.0, 2.0, 1.0}),
      /*output_limit=*/Rect::MakeLTRB(100, 100, 200, 200));
  ASSERT_EQ(coverage, ???);
}

Should the output_limit be Rect::MakeLTRB(100 - 2, 100, 200 + 2, 200) or Rect::MakeLTRB(50 - 2, 50, 100 + 2, 100) or Rect::MakeLTRB(200 - 2, 200, 400 + 2, 400)? It's not clear how the effect_transform is related to the operation by reading the documentation.

[flar]

This never defines what effect_transform is. Looking at the implementation of some GetFilterSourceCoverage i know that only the basis is used. That should be mentioned.

Use of Basis is specific to a filter computing its best guess, not to the general operation. All this method specifies is that the filters should do a reverse computation, not how they should do it.

For the record, I don't think the use of basis on those filters is correct and I should file an issue about it (done). I used basis because the existing methods in the class already used basis and I wanted to be consistent with the current state of their implementations.

For DirectionalGaussianBlurFilterContents::GetFilterSourceCoverage shouldn't the result of GetFilterSourceCoverage be smaller than output_limit since the source will not have the blur radius? It is actually returning a larger result.

No, it has to be larger. Input pixels are taken from a radius and so we need more pixels from the source than the size of the dest.

The problem I ran into was this:

TEST(DirectionalGaussianBlurFilterContents, FilterSourceCoverage) {
  Scalar sigma_radius_1 = CalculateSigmaForBlurRadius(1.0);
  auto contents = std::make_unique<DirectionalGaussianBlurFilterContents>();
  contents->SetSigma(Sigma{sigma_radius_1});
  contents->SetDirection({1.0, 0.0});
  std::optional<Rect> coverage = contents->GetFilterSourceCoverage(
      /*effect_transform=*/Matrix::MakeScale({2.0, 2.0, 1.0}),
      /*output_limit=*/Rect::MakeLTRB(100, 100, 200, 200));
  ASSERT_EQ(coverage, ???);
}

Should the output_limit be Rect::MakeLTRB(100 - 2, 100, 200 + 2, 200) or Rect::MakeLTRB(50 - 2, 50, 100 + 2, 100) or Rect::MakeLTRB(200 - 2, 200, 400 + 2, 400)? It's not clear how the effect_transform is related to the operation by reading the documentation.

I mentioned that it was relative to the transform in the most recent cut, but I'll be more specific. The supplied "output_limit" and the result should both be in the transformed space. The sigma is in the untransformed space, but the input and output coverages are both in the transformed space. So, you need to adjust the sigma to its correct value in the transformed space and then all 3 are in the same space.

So, here you would need to scale the radius by 2, but then apply it directly to the 100,100 values and return them relative to 100,100 - so expand by 2 in both left and right. Your first answer looks right to me, other than assuming that sigma is the value you should expand to. There is a calculation in the code that maps sigma to the "fringe" of the gaussian blur. That "fringe" calculation will be in the untransformed space, so compute sigma -> fringe, and then map fringe through the transform.

But since both the input and the output rectangles are in the transformed space of the effect_transform, neither of those gets run through, or inversed through, the transform.

[gaaclarke]

This one is mostly good, but make the brief define what the function does briefly. Also define effect_transform. You'll probably just want to link to a definition somewhere else in this file.

What is the coordinate space of the output_limit?

[flar]

|GetSourceCoverage| is a link to the full definition...

[flar]

As per our discussion in the chat. If you want to format these differently. I'm fine with that. Here is an example of implicit brief/details that doesn't use extra whitespace.

Consistency within files and modules is always a win, though. I was more questioning if we are sure we want to finalize on this style before we start going back and really filling out a lot of comments as we are likely to do in the near future. The chat showed widespread support for doing the style already being used in this file.

[gaaclarke]

This is much more helpful. I still suspect they can be improved but I'm definitely not well informed enough to suggest anything. Thanks!

[auto-submit]

auto label is removed for flutter/engine/47567, due to - The status or check suite Linux linux_android_aot_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.