Closed Bug 1359527 Opened 3 years ago Closed 3 years ago

NYTimes SVG can take 30ms to render

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

(Depends on 2 open bugs)

Details

Attachments

(5 files, 9 obsolete files)

329.82 KB, image/svg+xml
Details
4.97 KB, patch
Details | Diff | Splinter Review
31.91 KB, patch
mchang
: review+
Details | Diff | Splinter Review
2.51 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
7.54 KB, patch
mchang
: review+
Details | Diff | Splinter Review
Still a WIP dump before I forget this week on PTO. I tried making the callers of GetMaskForMaskedFrame assume what was returned was an RGBA surface, then use push/pop layer to convert it to an alpha mask using luminance. However, I had lots of flags being pushed around like whether to convert it to an alpha mask, which SVG type or create a new flag about how to create an alpha mask. It wasn't I' very pretty. Also, sometimes the mask wasn't directy used in push/pop groups, so I had to feed this information down to nsSVGPath as it sometimes had an extra mask. The code got quite ugly.

I wanted to try to do something like DrawTarget::SnapshotAlpha, that would give you an alpha surface based on the SVG type. This worked great for everything except D2D. I've been fighting D2D to draw the luminance effect to a temporary surface, since D2D can't draw an effect from the texture it's reading from. However, this doesn't seem to be working which is where I'm at now.
Comment on attachment 8863172 [details] [diff] [review]
WIP - Create an alpha snapshot option for SVG Masks

Just wondering if you could take a look at drawTargetD2D1::SnapshotAlpha here. What I'd like to do is create a temporary bitmap with the current contents after a luminance d2d effect. This doesn't seem to be working and the resulting surface always seems zeroed out. Does this not work in general or am I missing something here? Thanks!
Attachment #8863172 - Flags: feedback?(bas)
Attachment #8863172 - Flags: feedback?(bas)
Attached patch DrawTarget::SnapshotAlpha (obsolete) — Splinter Review
Any idea why DrawTargetD2D1::SnapshotAlpha doesn't seem to do anything?
Attachment #8863172 - Attachment is obsolete: true
Attachment #8866987 - Flags: feedback?(jmuizelaar)
Comment on attachment 8866987 [details] [diff] [review]
DrawTarget::SnapshotAlpha

It seems reasonable to me but Bas probably has better intuition.
Attachment #8866987 - Flags: feedback?(jmuizelaar) → feedback?(bas)
Comment on attachment 8866987 [details] [diff] [review]
DrawTarget::SnapshotAlpha

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

::: gfx/2d/DrawTargetD2D1.cpp
@@ +123,5 @@
> +  }
> +
> +  // Create the luminance effect
> +  RefPtr<ID2D1Effect> luminanceEffect;
> +  hr = mDC->CreateEffect(CLSID_D2D1LuminanceToAlpha, getter_AddRefs(luminanceEffect));

Note you'll want to cache this effect, effect creation is relatively expensive.

@@ +141,5 @@
> +  // Draw the output of the luminance to the tmp bitmap 
> +  Rect fullRect(0, 0, mSize.width, mSize.height);
> +  mDC->SetTransform(D2D1::IdentityMatrix());
> +  mDC->SetTarget(tmpBitmap);
> +  mDC->DrawImage(luminanceImage);

This is probably your problem, I'm guessing GetOutput isn't implemented well for this effect. You can implement the same thing more efficiently by using:

https://msdn.microsoft.com/en-us/library/windows/desktop/jj841149(v=vs.85).aspx

Instead of getting the output image and then drawing that. Let me know how that goes, otherwise I'll have some other ideas.
Attachment #8866987 - Flags: feedback?(bas)
Drawing the luminance effect directly doesn't seem to work exactly as needed as well :/.
Attachment #8868265 - Flags: feedback?(bas)
(In reply to Mason Chang [:mchang] from comment #7)
> Created attachment 8868265 [details] [diff] [review]
> Directly draw the luminance effect
> 
> Drawing the luminance effect directly doesn't seem to work exactly as needed
> as well :/.

Define 'doesn't seem to work exactly as needed'? :) That's relatively little information to go on.
Comment on attachment 8868265 [details] [diff] [review]
Directly draw the luminance effect

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

::: gfx/2d/DrawTargetD2D1.cpp
@@ +146,5 @@
> +  mDC->DrawImage(luminanceEffect,
> +                 nullptr,
> +                 nullptr,
> +                 D2D1_INTERPOLATION_MODE_LINEAR,
> +                 D2D1_COMPOSITE_MODE_SOURCE_OVER);

At least you're not clearing the bitmap so you'll want to use COPY, but I doubt that's your issue.
Attachment #8868265 - Flags: feedback?(bas)
See Also: → 1362246
Attached patch Working SnapshotLuminance patch (obsolete) — Splinter Review
Seems to work other than one stylo maybe reftest failure - https://treeherder.mozilla.org/#/jobs?repo=try&revision=94e7847589c32ce6572ef284ee90b7d7d9427aeb
Attachment #8866987 - Attachment is obsolete: true
Attachment #8868265 - Attachment is obsolete: true
Comment on attachment 8869468 [details] [diff] [review]
Working SnapshotLuminance patch

Looks like the failure was an intermittent.
Attachment #8869468 - Flags: review?(jmuizelaar)
Comment on attachment 8869468 [details] [diff] [review]
Working SnapshotLuminance patch

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

Bas should review this.
Attachment #8869468 - Flags: review?(jmuizelaar) → review?(bas)
Comment on attachment 8869468 [details] [diff] [review]
Working SnapshotLuminance patch

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

::: gfx/2d/2D.h
@@ +896,5 @@
>    virtual already_AddRefed<SourceSurface> Snapshot() = 0;
> +
> +  // Snapshots the contents and returns an alpha mask
> +  // based on the RGB values.
> +  virtual already_AddRefed<SourceSurface> SnapshotLuminance(uint8_t aMaskParams, float aOpacity);

Since this is mutating the DrawTarget calling it Snapshot is probably not the best. Maybe IntoLuminanceSource()?

::: gfx/2d/DrawTarget.cpp
@@ +288,5 @@
> +    default:
> +    {
> +      ComputeAlphaMask(map.mData, map.mStride,
> +                       destMap.mData, destMap.mStride,
> +                       size, aOpacity);

Shouldn't the users of this just be drawing to an A8 surface?
Comment on attachment 8869468 [details] [diff] [review]
Working SnapshotLuminance patch

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

::: gfx/2d/DrawTarget.cpp
@@ +288,5 @@
> +    default:
> +    {
> +      ComputeAlphaMask(map.mData, map.mStride,
> +                       destMap.mData, destMap.mStride,
> +                       size, aOpacity);

What do you mean? We create an A8 surface above and draw to it.
Comment on attachment 8869468 [details] [diff] [review]
Working SnapshotLuminance patch

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

::: gfx/2d/DrawTarget.cpp
@@ +288,5 @@
> +    default:
> +    {
> +      ComputeAlphaMask(map.mData, map.mStride,
> +                       destMap.mData, destMap.mStride,
> +                       size, aOpacity);

If the producer of the mask knows that they are going to need an alpha mask they should just create an A8 DrawTarget. Also the code in ComputeAlphaMask is terribly slow. Even though it's just being moved I object to existence in the first place :) I'll see if I can write a patch that removes it.
Comment on attachment 8869468 [details] [diff] [review]
Working SnapshotLuminance patch

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

Also, have you measured a performance improvement with this patch?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> Comment on attachment 8869468 [details] [diff] [review]
> Working SnapshotLuminance patch
> 
> Review of attachment 8869468 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also, have you measured a performance improvement with this patch?

I did and it is all over the place. Sometimes it's much faster, sometimes its slower. I can't reproduce the slowness on NYT on my Windows machine since probably the page changed. My giant Snapchat mask has huge variability, sometimes 2ms to 60 ms. The old CPU version goes from 30-40ms, so this might be a case as with box shadows where certain sizes aren't worth making a temporary bitmap.
I tried to wrap the output of the Luminance Image with a SourceSurfaceD2D1. But according to the docs [1], the output sets the RGB channels to 0 and only sets the alpha channel. If we directly use the output of the luminance effect, it sets the bitmap's RGB channels to 0.

This worked with push/pop group because we should first Snapshot the full bitmap, which creates a copy in SourceSurface[2]. This copy would then become the input to the luminance effect, and the output of the luminance effect with the copy would become the mask. It was never directly touching the mBitmap of DrawTargetD2D.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/hh706363(v=vs.85).aspx
[2] http://searchfox.org/mozilla-central/source/gfx/2d/SourceSurfaceD2D1.cpp#97
As I understand it, effects don't mutate their inputs. Also, I think the copy in SourceSurface only happens if original DrawTarget changes.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #19)
> As I understand it, effects don't mutate their inputs. Also, I think the
> copy in SourceSurface only happens if original DrawTarget changes.

I found the bug here if we do not create a temporary surface and instead only wrap the output of the luminance effect in SnapshotLuminance. When we call DrawTargetD2D1::PushLayer, we create a bitmap brush and use that as the mask [1]. However, the query interface actually fails, so the bitmap brush becomes empty. It looks like if you're given an actual ID2D1Image, you aren't allowed to query interface [2] for an ID2D1Bitmap. From the test cases I've seen, usually when we call PushLayer, we're given a DataSourceSurface, which creates an ID2D1Bitmap and thus we can queryInterface for it successfully. I think all calls at [3] where the result is an actual ID2D1Effect will actually fail here. From the MSDN docs [4], we can only use an ID2D1ImageBrush on Windows 8 and later, which means we'd have to use a temporary surface on Windows 7 to draw the image to a bitmap, then create the bitmap brush.

A workaround could be to check what the actual mask surface is, and create a bitmap brush if we have a data surface. If we have an ID2D1Image, we can create an image brush on windows 8 and higher and create a temporary surface on windows 7 and lower.

I wanted to see if this failed anywhere else in master before though - try with a MOZ_CRASH if query_interface fails - https://treeherder.mozilla.org/#/jobs?repo=try&revision=904483673de29e61bff21d9171bae8c96be5cb6a


[1] http://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetD2D1.cpp#841
[2] https://stackoverflow.com/questions/31118397/direct2d-convert-id2d1image-to-id2d1bitmap/31165846
[3] http://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetD2D1.cpp#832
[4] https://msdn.microsoft.com/en-us/library/windows/desktop/dd756651(v=vs.85).aspx
I think we should be able to just switch to using an image brush unconditionally. I believe the Windows 8 functionality is also available in Win7 with platform update which is required for D2D these days.
Attached patch Add DrawTarget::IntoLuminance (obsolete) — Splinter Review
Attachment #8866549 - Attachment is obsolete: true
Attachment #8869468 - Attachment is obsolete: true
Attachment #8869468 - Flags: review?(bas)
Attachment #8870511 - Flags: review?(jmuizelaar)
Attachment #8870511 - Flags: review?(bas)
Comment on attachment 8870511 [details] [diff] [review]
Add DrawTarget::IntoLuminance

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

::: gfx/2d/2D.h
@@ +896,5 @@
>    virtual already_AddRefed<SourceSurface> Snapshot() = 0;
> +
> +  // Snapshots the contents and returns an alpha mask
> +  // based on the RGB values.
> +  virtual already_AddRefed<SourceSurface> IntoLuminanceSource(uint8_t aMaskParams, float aOpacity);

MaskParams should be an enum

::: gfx/2d/DrawTargetD2D1.cpp
@@ +17,5 @@
>  #include "Tools.h"
>  
> +#include "nsStyleConsts.h"
> +#include "nsAppRunner.h"
> +#include "nsDebug.h"

We can't include these headers in Moz2D

@@ +125,5 @@
> +  }
> +
> +  // Make sure we flush everything.
> +  // This will call BeginDraw for us
> +  Flush();

Is this needed?

::: gfx/2d/SourceSurfaceD2D1.cpp
@@ +6,5 @@
>  #include "SourceSurfaceD2D1.h"
>  #include "DrawTargetD2D1.h"
>  #include "Tools.h"
> +#include "nsDebug.h"
> +#include "nsAppRunner.h"

We can't include these headers in Moz2D
Attachment #8870511 - Flags: review?(jmuizelaar) → review-
Attached patch Add DrawTarget::IntoLuminance (obsolete) — Splinter Review
Updated with fixes from comment 24.
Attachment #8870511 - Attachment is obsolete: true
Attachment #8870511 - Flags: review?(bas)
Attachment #8870541 - Flags: review?(jmuizelaar)
Attachment #8870541 - Flags: review?(bas)
Attached patch Add DrawTarget::IntoLuminance (obsolete) — Splinter Review
Attachment #8870541 - Attachment is obsolete: true
Attachment #8870541 - Flags: review?(jmuizelaar)
Attachment #8870541 - Flags: review?(bas)
Attachment #8870542 - Flags: review?(jmuizelaar)
Attachment #8870542 - Flags: review?(bas)
Attachment #8870542 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8870542 [details] [diff] [review]
Add DrawTarget::IntoLuminance

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

Bas reviewed this over my shoulder. He said it looked fine.
Attachment #8870542 - Flags: review?(bas) → review+
Attached patch Add DrawTarget::IntoLuminance (obsolete) — Splinter Review
I landed bug 1367147 which conflicts with this. Here's a rebased version.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9281b1e567329b55f3a677780e7d1d9ad4c94098
Attachment #8870542 - Attachment is obsolete: true
Attachment #8872009 - Flags: review+
Fails to build on Android:
 /home/worker/workspace/build/src/gfx/2d/DrawTarget.cpp:79:50: error: 'ComputesRGBLuminanceMask_NEON' was not declared in this scope
Attachment #8872009 - Attachment is patch: true
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e12fa325e112
Add DrawTarget::IntoLuminance. r=jrmuizel,Bas
So I can reproduce bad rendering locally on tests like layout/reftests/w3c-css/submitted/masking/mask-composite-1c.html. Can you also reproduce this Mason? Was masking working for you before?
Flags: needinfo?(mchang)
I tried adding a Flush() and that didn't help.
This is happening because of [1]. If we have an ID2D1Image that isn't backed by an actual ID2D1Bitmap, you can't query for the bitmap from the image. Jeff and I talked about this over irc. We'll use push/pop layer instead. I got this working yesterday, but I'm hitting try failures now. 

[1] http://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetD2D1.cpp#355
Flags: needinfo?(mchang)
Carrying r+. Needed two fixes:

1) When pushing a layer, don't transform the rect.
2) A data source surface might be partially uploaded via an ID2D1Bitmap. In these cases, the sizes of the bitmap and the mask surface will be different.
Attachment #8872009 - Attachment is obsolete: true
Attachment #8874881 - Flags: review+
Attachment #8874882 - Flags: review?(jmuizelaar) → review+
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f09499d7310c
Part 1 Add DrawTarget::IntoLuminance. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0fe335fbe5
Part 2 Add D2D fuzzing for GPU Luminance. r=jrmuizel
Backed out for Android bustage while processing gfx/2d/moz.build:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6cae88b7d279dd14d8f52b71380b98a5fbb5786
https://hg.mozilla.org/integration/mozilla-inbound/rev/216d4d5cbb0c924b71616e7d721d99845962fb4f

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=eb0fe335fbe5256fd2278f7c846699ffbbaecf83&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=104929019&repo=mozilla-inbound

task 2017-06-06T16:18:55.044362Z] 16:18:55     INFO -  Reticulating splines...
[task 2017-06-06T16:18:56.276350Z] 16:18:56     INFO -  Traceback (most recent call last):
[task 2017-06-06T16:18:56.279298Z] 16:18:56     INFO -    File "/home/worker/workspace/build/src/configure.py", line 124, in <module>
[task 2017-06-06T16:18:56.279338Z] 16:18:56     INFO -      sys.exit(main(sys.argv))
[task 2017-06-06T16:18:56.279369Z] 16:18:56     INFO -    File "/home/worker/workspace/build/src/configure.py", line 34, in main
[task 2017-06-06T16:18:56.279395Z] 16:18:56     INFO -      return config_status(config)
[task 2017-06-06T16:18:56.279432Z] 16:18:56     INFO -    File "/home/worker/workspace/build/src/configure.py", line 119, in config_status
[task 2017-06-06T16:18:56.279461Z] 16:18:56     INFO -      return config_status(args=[], **encode(sanitized_config, encoding))
[task 2017-06-06T16:18:56.279519Z] 16:18:56     INFO -    File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/config_status.py", line 147, in config_status
[task 2017-06-06T16:18:56.279551Z] 16:18:56     INFO -      definitions = list(definitions)
[task 2017-06-06T16:18:56.279585Z] 16:18:56     INFO -    File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/frontend/emitter.py", line 183, in emit
[task 2017-06-06T16:18:56.279604Z] 16:18:56     INFO -      objs = list(emitfn(out))
[task 2017-06-06T16:18:56.279635Z] 16:18:56     INFO -    File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/frontend/emitter.py", line 1026, in emit_from_context
[task 2017-06-06T16:18:56.279660Z] 16:18:56     INFO -      for obj in self._handle_linkables(context, passthru, generated_files):
[task 2017-06-06T16:18:56.279690Z] 16:18:56     INFO -    File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/frontend/emitter.py", line 802, in _handle_linkables
[task 2017-06-06T16:18:56.279709Z] 16:18:56     INFO -      'exist: \'%s\'' % (symbol, full_path), context)
[task 2017-06-06T16:18:56.279728Z] 16:18:56     INFO -  mozbuild.frontend.reader.SandboxValidationError:
[task 2017-06-06T16:18:56.279743Z] 16:18:56     INFO -  ==============================
[task 2017-06-06T16:18:56.279763Z] 16:18:56     INFO -  ERROR PROCESSING MOZBUILD FILE
[task 2017-06-06T16:18:56.279779Z] 16:18:56     INFO -  ==============================
[task 2017-06-06T16:18:56.279806Z] 16:18:56     INFO -  The error occurred while processing the following file or one of the files it includes:
[task 2017-06-06T16:18:56.279827Z] 16:18:56     INFO -      /home/worker/workspace/build/src/gfx/2d/moz.build
[task 2017-06-06T16:18:56.279852Z] 16:18:56     INFO -  The error occurred when validating the result of the execution. The reported error is:
[task 2017-06-06T16:18:56.279884Z] 16:18:56     INFO -      File listed in SOURCES does not exist: '/home/worker/workspace/build/src/gfx/2d/LuminanceNEON.cpp'
Flags: needinfo?(mchang)
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e14c50095f36
Part 1 - Add DrawTarget::IntoLuminance r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb451a4d22c
Part 2 - Add D2D fuzzing for IntoLuminance r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/e14c50095f36
https://hg.mozilla.org/mozilla-central/rev/7fb451a4d22c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Why did you drop the NEON code?
Flags: needinfo?(mchang)
Also this commented line made it in:

+already_AddRefed<SourceSurface>
+DrawTargetD2D1::IntoLuminanceSource(LuminanceType aLuminanceType, float aOpacity)
+{
+  //return DrawTarget::IntoLuminanceSource(aLuminanceType, aOpacity);
+  if (aLuminanceType != LuminanceType::LUMINANCE) {
+    return DrawTarget::IntoLuminanceSource(aLuminanceType, aOpacity);
+  }
This hunk is also wrong:

+  if (StyleSVG()->mColorInterpolation ==
+    NS_STYLE_COLOR_INTERPOLATION_LINEARRGB) {
+    maskType = NS_STYLE_COLOR_INTERPOLATION_LINEARRGB;
+  }
+
   RefPtr<SourceSurface> surface;
   if (maskType == NS_STYLE_MASK_TYPE_LUMINANCE) {
-    RefPtr<SourceSurface> maskSnapshot = maskDT->Snapshot();
+    RefPtr<SourceSurface> maskSnapshot =
+      maskDT->IntoLuminanceSource(GetLuminanceType(maskType),
+                                  aParams.opacity);
     if (!maskSnapshot) {
       return nullptr;
     }
-

The change to NS_STYLE_COLOR_INTERPOLATION_LINEARRGB needs to be inside the 'if (maskType == NS_STYLE_MASK_TYPE_LUMINANCE)'
otherwise we use an ALPHA mask for NS_STYLE_COLOR_INTERPOLATION_LINEARRGB
Attached patch Fix up mismergesSplinter Review
Attachment #8875910 - Flags: review?(mchang)
Comment on attachment 8875910 [details] [diff] [review]
Fix up mismerges

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

Thanks, sorry about that.
Attachment #8875910 - Flags: review?(mchang) → review+
Flags: needinfo?(mchang)
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da58840d7f3d
Fixup DrawTarget::IntoLuminance mismerge. r=mchang
Duplicate of this bug: 1362246
Depends on: 1372577
Depends on: 1375452
Blocks: 1381911
Depends on: 1384929
No longer blocks: 1381911
Depends on: 1381911
Depends on: 1425385
Depends on: 1427778
Depends on: 1429594
Depends on: 1448667
Depends on: 1522422
No longer depends on: 1522422
Regressions: 1522422
You need to log in before you can comment on or make changes to this bug.