NYTimes SVG can take 30ms to render

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

(Depends on: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments, 9 obsolete attachments)

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
(Assignee)

Description

7 months ago
Created attachment 8861544 [details]
Test Case

Profile - https://perfht.ml/2qbuQqZ
(Assignee)

Comment 1

7 months ago
Created attachment 8863172 [details] [diff] [review]
WIP - Create an alpha snapshot option for SVG Masks

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.
(Assignee)

Comment 2

7 months ago
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)
(Assignee)

Updated

7 months ago
Attachment #8863172 - Flags: feedback?(bas)
(Assignee)

Comment 3

7 months ago
Created attachment 8866549 [details] [diff] [review]
Ugly WIP of passing SVG push/pop layer
(Assignee)

Comment 4

7 months ago
Created attachment 8866987 [details] [diff] [review]
DrawTarget::SnapshotAlpha

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)
(Assignee)

Comment 7

6 months ago
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 :/.
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)
(Assignee)

Updated

6 months ago
See Also: → bug 1362246
(Assignee)

Comment 10

6 months ago
Created attachment 8869468 [details] [diff] [review]
Working SnapshotLuminance patch

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
(Assignee)

Comment 11

6 months ago
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?
(Assignee)

Comment 14

6 months ago
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?
(Assignee)

Comment 17

6 months ago
(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.
(Assignee)

Comment 18

6 months ago
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.
Created attachment 8869631 [details] [diff] [review]
A sketch of getting rid of the badness for alpha masks
(Assignee)

Comment 21

6 months ago
(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.
(Assignee)

Comment 23

6 months ago
Created attachment 8870511 [details] [diff] [review]
Add DrawTarget::IntoLuminance
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-
(Assignee)

Comment 25

6 months ago
Created attachment 8870541 [details] [diff] [review]
Add DrawTarget::IntoLuminance

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)
(Assignee)

Comment 26

6 months ago
Created attachment 8870542 [details] [diff] [review]
Add DrawTarget::IntoLuminance
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+
Created attachment 8872009 [details] [diff] [review]
Add DrawTarget::IntoLuminance

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+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64b324f907fac990d592c7bb19b79a54cddc049c
With the build error hopefully fixed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64b324f907fac990d592c7bb19b79a54cddc049c
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
This might fix it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56257bdebe7e2a6695de1172f1c06b40ffd2be77
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed0f1ff9e2adae23b0ecfbe8f463297840882c77
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79d3c41b60cd9938f69f2ed5638fd34fcf30d3bc
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2d6fe69e0022e9018a9f7d0ae5ee39190e7fdd2
Attachment #8872009 - Attachment is patch: true

Comment 36

6 months ago
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e12fa325e112
Add DrawTarget::IntoLuminance. r=jrmuizel,Bas
And then there's Windows 8: https://treeherder.mozilla.org/logviewer.html#?job_id=102713138&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/75b68c6105e1
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.
(Assignee)

Comment 40

6 months ago
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)
(Assignee)

Comment 41

6 months ago
Created attachment 8874881 [details] [diff] [review]
Add DrawTarget::IntoLuminance

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+
(Assignee)

Comment 42

6 months ago
Created attachment 8874882 [details] [diff] [review]
Reftest fuzzing

Successful tries:

D2D - https://treeherder.mozilla.org/#/jobs?repo=try&revision=34f27067adc4dafea6b7a1a0b4d14f36131c4ce1

All - https://treeherder.mozilla.org/#/jobs?repo=try&revision=07689ab9b6c7f8f67db2adfb2c8651d6ff77a639
Attachment #8874882 - Flags: review?(jmuizelaar)
Attachment #8874882 - Flags: review?(jmuizelaar) → review+

Comment 43

6 months ago
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)
(Assignee)

Comment 45

6 months ago
Fixed with android builds - https://treeherder.mozilla.org/#/jobs?repo=try&revision=098bb169608a2c17be282cfb2fada6fe3ff7bef5
Flags: needinfo?(mchang)

Comment 46

6 months ago
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

Comment 47

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e14c50095f36
https://hg.mozilla.org/mozilla-central/rev/7fb451a4d22c
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
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
Created attachment 8875910 [details] [diff] [review]
Fix up mismerges
Attachment #8875910 - Flags: review?(mchang)
(Assignee)

Comment 52

6 months ago
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+
(Assignee)

Updated

6 months ago
Flags: needinfo?(mchang)

Comment 53

6 months ago
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da58840d7f3d
Fixup DrawTarget::IntoLuminance mismerge. r=mchang

Comment 54

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/da58840d7f3d
Duplicate of this bug: 1362246
Depends on: 1372577

Updated

5 months ago
Depends on: 1375452

Updated

4 months ago
Blocks: 1381911
Depends on: 1384929
No longer blocks: 1381911
Depends on: 1381911
You need to log in before you can comment on or make changes to this bug.