1120294 - Use A8 format surface to store svg mask

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Ethan Lin[:ethlin]]

Use a A8 format surface to store svg mask. It could benefit the performance of apply mask and we should only set the alpha channel when calculating mask.

Comment 1

[Ethan Lin[:ethlin]]

Attachment: bug_1120294.patch

Create a A8 format surface to store the svg mask. We still need the original ARGB buffer, but the new A8 mask could reduce the time of mask applying. I also change the memset action of each luminance calculation function, so we only need to do the 1/4 memset now.

Comment 2

[Robert Longson [:longsonr]]

Comment on attachment 8547353 [details] [diff] [review]
bug_1120294.patch

> 
> static void
>-ComputesRGBLuminanceMask(uint8_t *aData,
>-                         int32_t aStride,
>+ComputesRGBLuminanceMask(uint8_t *aSrcData,

can we have aSrcData be a const pointer in every case and not modify the source pixels? 
Same comment applies to every method.

>+                         int32_t aSrcStride,
>+                         uint8_t *aDstData,
>+                         int32_t aDstStride,
>                          const IntSize &aSize,
>                          float aOpacity)
> {
> 
>+  // Create a aplha channel mask for output

s/a aplha/an alpha/

Comment 3

[Ethan Lin[:ethlin]]

Attachment: v1 - use a8 surface and modify neon code for svg

Thanks for the suggestions. I tried to use const srcData, but I couldn't send const pointer to ComputeLinearRGBLuminanceMask because there is some alpha calculations. For the other two functions, I still use const pointer. I also enhance the neon part that we can use neon to do the alpha checking.

Comment 4

[Ethan Lin[:ethlin]]

Attachment: v1 - use a8 surface and modify neon code for svg

Previous patch is wrong. Update a new patch.

Comment 5

[Robert Longson [:longsonr]]

Comment on attachment 8547495 [details] [diff] [review]
v1 - use a8 surface and modify neon code for svg

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

Some comments apply in many places. Overall this is in pretty good shape though. I only give it a - as I want to see the new version.

::: layout/svg/nsSVGMaskFrame.cpp
@@ +135,1 @@
>          }

Copy the 4 srcPixels into a uint32_t then change that.

@@ +145,1 @@
>                          blueFactor) >> 8) * (a / 255.0f));

And work with the copy here.

@@ +276,5 @@
>    RefPtr<DataSourceSurface> maskSurface = maskSnapshot->GetDataSurface();
>    DataSourceSurface::MappedSurface map;
>    if (!maskSurface->Map(DataSourceSurface::MapType::READ_WRITE, &map)) {
>      return nullptr;
>    }

We really want this to be MapType::READ

@@ +281,5 @@
>  
> +  // Create a alpha channel mask for output
> +  RefPtr<DrawTarget> dstMaskDT =
> +    Factory::CreateDrawTarget(BackendType::CAIRO, maskSurfaceSize,
> +                              SurfaceFormat::A8);

dest is clearer than dst

::: layout/svg/nsSVGMaskFrameNEON.cpp
@@ +31,5 @@
>    uint8x8_t redVec = vdup_n_u8(redFactor);
>    uint8x8_t greenVec = vdup_n_u8(greenFactor);
>    uint8x8_t blueVec = vdup_n_u8(blueFactor);
> +  uint8x8_t zeroVec = vdup_n_u8(0);
> +  uint8x8_t oneVec = vdup_n_u8(1);

I'd rather we used Vector than Vec. Abbreviations are really not useful as a coding style.

@@ +47,5 @@
> +      gray = vmul_u8(gray, vand_u8(alphaVec, oneVec));
> +
> +      // Put the result to the 8 pixels
> +      vst1_u8(dstPixel, gray);
> +      srcPixel += 8 * 4;

* sizeof srcPixel[0] is clearer than the magic 4

@@ +53,3 @@
>      }
>  
> +    // Calculate the reset pixels of the line by cpu

reset? Shouldn't this be Calculate the rest of the pixels...

@@ +56,5 @@
>      for (int32_t x = 0; x < remainderWidth; x++) {
> +      if (srcPixel[GFX_ARGB32_OFFSET_A] > 0) {
> +        dstPixel[0] = (redFactor * srcPixel[GFX_ARGB32_OFFSET_R]+
> +                       greenFactor * srcPixel[GFX_ARGB32_OFFSET_G] +
> +                       blueFactor * srcPixel[GFX_ARGB32_OFFSET_B]) >> 8;

I'd prefer *dstPixel to dstPixel[0] but I don't really mind if you want to leave it as is.

@@ +60,5 @@
> +                       blueFactor * srcPixel[GFX_ARGB32_OFFSET_B]) >> 8;
> +      } else {
> +        dstPixel[0] = 0;
> +      }
> +      srcPixel += 4;

sizeof would be clearer

::: layout/svg/nsSVGMaskFrameNEON.h
@@ +15,2 @@
>                                int32_t aStride,
> +                              uint8_t *aDstData,

Make these aSrcData and aSrcStride or better yet change everything to aSourceData, aSourceStride, aDestData and aDestStride.

Comment 6

[Robert Longson [:longsonr]]

I've reviewed the wrong patch then. Can you apply the review comments anyway and I'll rereview once you've done that.

Comment 7

[Ethan Lin[:ethlin]]

Attachment: v1 - use a8 surface and modify neon code for svg

Thanks for the comments. I applied most of the comments, but I cannot replace the magic number 4 with sizeof(sourcePixel[0]) because the type of sourcePixel is uint8_t. Most places are using the the magic number now. This may be another issue for reducing the magic number.

Comment 8

[Robert Longson [:longsonr]]

Comment on attachment 8547601 [details] [diff] [review]
v1 - use a8 surface and modify neon code for svg

>       if (a) {
>+        uint8_t tempPixel[4];
>+        memcpy(tempPixel, sourcePixel, 4);

move the memcpy into an else case of the if below.

>         if (a != 255) {
>-          pixel[GFX_ARGB32_OFFSET_B] =
>-            (255 * pixel[GFX_ARGB32_OFFSET_B]) / a;
>-          pixel[GFX_ARGB32_OFFSET_G] =
>-            (255 * pixel[GFX_ARGB32_OFFSET_G]) / a;
>-          pixel[GFX_ARGB32_OFFSET_R] =
>-            (255 * pixel[GFX_ARGB32_OFFSET_R]) / a;
>+          tempPixel[GFX_ARGB32_OFFSET_B] =
>+            (255 * tempPixel[GFX_ARGB32_OFFSET_B]) / a;
>+          tempPixel[GFX_ARGB32_OFFSET_G] =
>+            (255 * tempPixel[GFX_ARGB32_OFFSET_G]) / a;
>+          tempPixel[GFX_ARGB32_OFFSET_R] =
>+            (255 * tempPixel[GFX_ARGB32_OFFSET_R]) / a;

and use sourcePixel as the source here, thereby avoiding the memcpy in the if case.

>         }
>

Comment 9

[Ethan Lin[:ethlin]]

Attachment: v2 - Reduce the chance of memcpy

This patch apply the comment that checking the alpha to reduce the chance of memory copy.

Comment 10

[Robert Longson [:longsonr]]

Comment on attachment 8547881 [details] [diff] [review]
v2 - Reduce the chance of memcpy

>+          uint8_t tempPixel[4];
>+          memcpy(tempPixel, sourcePixel, 4);

You don't need the memcpy

>           tempPixel[GFX_ARGB32_OFFSET_B] =
>             (255 * tempPixel[GFX_ARGB32_OFFSET_B]) / a;
>           tempPixel[GFX_ARGB32_OFFSET_G] =
>             (255 * tempPixel[GFX_ARGB32_OFFSET_G]) / a;
>           tempPixel[GFX_ARGB32_OFFSET_R] =
>             (255 * tempPixel[GFX_ARGB32_OFFSET_R]) / a;

If you change tempPixel to sourcePixel after each = here.

>+          /* sRGB -> linearRGB -> intensity */
>+          *destPixel =
>+            static_cast<uint8_t>
>+                       (((gsRGBToLinearRGBMap[tempPixel[GFX_ARGB32_OFFSET_R]] *
>+                          redFactor +
>+                          gsRGBToLinearRGBMap[tempPixel[GFX_ARGB32_OFFSET_G]] *
>+                          greenFactor +
>+                          gsRGBToLinearRGBMap[tempPixel[GFX_ARGB32_OFFSET_B]] *
>+                          blueFactor) >> 8) * (a / 255.0f));
>+        }

Comment 11

[Ethan Lin[:ethlin]]

Attachment: v2 - Reduce the chance of memcpy

I misunderstand the meaning. I think this patch should be correct.

Comment 12

[Ethan Lin[:ethlin]]

Please land the attachment 8547601 [details] [diff] [review] and attachment 8547961 [details] [diff] [review] to mozilla-central.

Try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ea685725bcf

Comment 13

[Robert Longson [:longsonr]]

The folks who do checkin-needed do like a checkin comments with r= on them. In this case r=longsonr

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da63d41b57e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/fba277ed63b8

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/da63d41b57e9
https://hg.mozilla.org/mozilla-central/rev/fba277ed63b8