1276061 - Kill SurfaceFilter::WriteRows(), because it's memory unsafe and we almost never need it

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

In most cases SurfaceFilter::WriteRows() is just used to copy from a buffer into a row (i.e. memcpy) or clear a row (i.e. memset). Let's just implement new API functions that do those things in a memory-safe manner and kill WriteRows(). The downscaler still needs something WriteRows()-esque, so we'll keep a method around with Unsafe in the name that is not exposed to callers outside of the SurfacePipe code. (i.e. the SurfacePipe class does not expose it) All but one usage of WriteRows() can be replaced by the new memory-safe methods.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add SurfaceFilter::WriteBuffer() and SurfaceFilter::WriteEmptyRow().

OK, here come some large patches, but they should be pretty self explanatory.

I've identified (as mentioned in comment 0), that the only thing WriteRows() is
really used for is memcpy() and memset(). We can replace the memcpy() case with
a safer method, WriteRows(), which is bounds checked. Likewise for the memset()
case - that's only used to clear a row, so it can just be replaced with
WriteEmptyRow().

For DownscalingFilter we do need a WriteRows()-like method, because we can't do
anything about the API that the Skia downscaling code uses, and we don't want to
introduce any additional copies. However, we can expose that API to internal
consumers (i.e. SurfaceFilters) without exposing it to external consumers (i.e.
the decoders, which only interact with a SurfacePipe). That's what we do. The
new method is named WriteUnsafeComputedRow(), which hopefully has a scary-enough
name to make people think twice before using it.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Remove SurfaceFilter::WriteRows().

This patch removes every use of WriteRows(), both in the actual code and in the
tests. The changes should be pretty self-explanatory. In the main code, every
use of WriteRows() gets replaced by one of the new methods. In the test code,
mostly the WriteRows() variants of tests are just deleted. There were a few
tests that didn't have both WritePixels() and WriteRows() versions; those are
converted to only use WritePixels(). Also, some of the code in Common.cpp had
some template-based abstraction over the WritePixels() and WriteRows() variants
of some functions; the abstractions are removed in this patch since they're no
longer necessary.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add ImageLib GTest utility functions for checking paletted image output.

Before writing part 4, which adds the new tests for these methods, it'll be
helpful to introduce some helper functions that make it easier to check the
output of writing to paletted surfaces. All we're doing is implementing paletted
versions of the functions that check that the entire image, rows of the image,
or a rect of the image contains a certain color.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add GTests for WriteBuffer() and WriteEmptyRow().

This patch introduces a whole bunch of new tests for WriteBuffer() and
WriteEmptyRow() (and WriteUnsafeComputedRow() isn't left out, either, though by
its nature we can't verify much about it). There are both unit tests of the
methods' behavior, in TestSurfaceSink, and tests that they're exposed correctly
via the SurfacePipe API, in TestSurfacePipeIntegration. Hopefully the test code
is pretty straightforward.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8757603 [details] [diff] [review]
(Part 1) - Add SurfaceFilter::WriteBuffer() and SurfaceFilter::WriteEmptyRow().

Review of attachment 8757603 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/SurfacePipe.h
@@ +319,5 @@
> +   * @aStartColumn or after the pixels copied from the buffer are cleared.
> +   *
> +   * Bounds checking failures produce warnings in debug builds because although
> +   * the bounds checking maintains safety, this kind of failure could indicate a
> +   * bug in the calling code.

Why not assert instead? Warnings are easy to overlook.

@@ +365,5 @@
> +
> +    // Clear the area before |aStartColumn|.
> +    const size_t prefixLength = std::min<size_t>(mInputSize.width, aStartColumn);
> +    if (MOZ_UNLIKELY(prefixLength != aStartColumn)) {
> +      NS_WARNING("Provided starting column is out-of-bounds in WriteBuffer");

Assert?

@@ +375,5 @@
> +    // Write |aLength| pixels from |aSource| into the row, with bounds checking.
> +    const size_t bufferLength =
> +      std::min<size_t>(mInputSize.width - prefixLength, aLength);
> +    if (MOZ_UNLIKELY(bufferLength != aLength)) {
> +      NS_WARNING("Provided buffer length is out-of-bounds in WriteBuffer");

Assert?

@@ +416,5 @@
> +   * directly write to the row. This is unsafe because SurfaceFilter can't
> +   * provide any bounds checking; that's up to the lambda itself. For this
> +   * reason, the other Write*() methods should be preferred whenever it's
> +   * possible to use them; WriteUnsafeComputedRow() should be used only when
> +   * it's absolutely necessary to avoid extra copies or other performance

Can this function be made private?

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8757605 [details] [diff] [review]
(Part 2) - Remove SurfaceFilter::WriteRows().

Review of attachment 8757605 [details] [diff] [review]:
-----------------------------------------------------------------

Lovely.

::: image/test/gtest/TestDeinterlacingFilter.cpp
@@ +326,1 @@
>            break;

You can remove the break. Ditto for all the subsequent cases in this switch.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8757606 [details] [diff] [review]
(Part 3) - Add ImageLib GTest utility functions for checking paletted image output.

Review of attachment 8757606 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/test/gtest/Common.cpp
@@ +174,5 @@
> +  RawAccessFrameRef currentFrame = aDecoder->GetCurrentFrameRef();
> +  uint8_t* imageData;
> +  uint32_t imageLength;
> +  currentFrame->GetImageData(&imageData, &imageLength);
> +  ASSERT_TRUE_OR_RETURN(imageData != nullptr, false);

Nit: I'd use |imageData| or |!!imageData| here.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8757607 [details] [diff] [review]
(Part 4) - Add GTests for WriteBuffer() and WriteEmptyRow().

Review of attachment 8757607 [details] [diff] [review]:
-----------------------------------------------------------------

I admit that my review of this patch is not the most thorough review I have ever done.

::: image/test/gtest/TestSurfacePipeIntegration.cpp
@@ +165,5 @@
> +  // green pixels. We'll use this for the WriteBuffer() tests.
> +  uint32_t buffer[100] = { 0 };
> +  for (int i = 0; i < 100; ++i) {
> +    buffer[i] = BGRAColor::Green().AsPixel();
> +  }

Here and elsewhere: it looks odd to explicitly zero the array and then immediately overwrite with something else. Also, would memset() be better than a loop? (I see that in a couple of the other places not all of the array elements have the same value. But it seems a shame to make the common case verbose in order to make it consistent with the uncommon case...)

::: image/test/gtest/TestSurfaceSink.cpp
@@ +79,5 @@
> +
> +  // Check that the generated image is correct.
> +  aCheckFunc();
> +}
> +template <typename WriteFunc> void

Nit: blank line between functions.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Thanks for the reviews!

(In reply to Nicholas Nethercote [:njn] from comment #5)
> Why not assert instead? Warnings are easy to overlook.

I originally did do that, but then I can't write tests for that case, because the assert crashes the test harness! I decided thorough testing is more important than a debug-only assert, so that's the direction I went.

> Can this function be made private?

It's private w.r.t. code outside the SurfacePipe code itself since it's not exposed on the SurfacePipe class, but within the SurfacePipe code, unfortunately not. That's because one SurfaceFilter has to be able to call WriteUnsafeComputedRow() on the next SurfaceFilter in the chain.

> Here and elsewhere: it looks odd to explicitly zero the array and then
> immediately overwrite with something else. Also, would memset() be better
> than a loop? (I see that in a couple of the other places not all of the
> array elements have the same value. But it seems a shame to make the common
> case verbose in order to make it consistent with the uncommon case...)

Yep, I agree. I'll make that change. (And the other changes requested above.)

Comment 10

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> Here and elsewhere: it looks odd to explicitly zero the array and then
> immediately overwrite with something else. Also, would memset() be better
> than a loop? (I see that in a couple of the other places not all of the
> array elements have the same value. But it seems a shame to make the common
> case verbose in order to make it consistent with the uncommon case...)

I forgot until I went to do this that I tried memset() before, but memset() can only write a 1 byte pattern. memset_pattern4() exists for a 4 byte pattern, like we need here, but I'm not sure of its portability.

I'll fix the initialization thing, though.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Remove SurfaceFilter::WriteRows().

Addressed review comments except where mentioned above. Same for the subsequent patches.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add ImageLib GTest utility functions for checking paletted image output.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add GTests for WriteBuffer() and WriteEmptyRow().

Comment 14

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48e220d82d59a695d2b89db1aa70e47e14b7a412
Bug 1276061 (Part 1) - Add SurfaceFilter::WriteBuffer() and SurfaceFilter::WriteEmptyRow(). r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/306db1d372da6fb57432b737517b1bbacdb53f91
Bug 1276061 (Part 2) - Remove SurfaceFilter::WriteRows(). r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/07551e3c756fc43cccb928d9c4b652f1be9a91b9
Bug 1276061 (Part 3) - Add ImageLib GTest utility functions for checking paletted image output. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bb4ddc6e947d51850081ddac5faafb48f1daeef
Bug 1276061 (Part 4) - Add GTests for WriteBuffer() and WriteEmptyRow(). r=njn

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48e220d82d59
https://hg.mozilla.org/mozilla-central/rev/306db1d372da
https://hg.mozilla.org/mozilla-central/rev/07551e3c756f
https://hg.mozilla.org/mozilla-central/rev/1bb4ddc6e947