924102 - Add support for filter rendering to Moz2D

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Markus Stange [:mstange]]

The plan is to add:
 - a FilterNode interface in Filters.h
 - two new DrawTarget APIs: CreateFilter and DrawFilter

We will use this for SVG and CSS filters.

Comment 1

[Markus Stange [:mstange]]

Attachment: wip

This is just a dump of what I have locally at the moment. It only compiles on Mac (I think) and needs to be cleaned up.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 814110 [details] [diff] [review]
wip

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

::: gfx/2d/2D.h
@@ +11,5 @@
>  #include "Rect.h"
>  #include "Matrix.h"
>  #include "UserData.h"
>  
> +#include "Filters.h"

Can't you forward-declare FilterNode and remove this #include?

Comment 3

[Andreas Gal :gal]

roc, I would like to discuss the approach here. I would like to take the API and implement the software fallback via Skia. This is a lot of code that seems hard to get right and performant and it has been implement many times before, including in Skia.

I understand there are concerns about skia's performance behavior and pixel scraping, but the software path is a fallback only, the GPU path likely has the same problems, and this is a lot of code and effort to prevent a problem that doesn't seem unique to this code.

Can we at least do this incremental? We use skia to land a 100-liner that implements this and we can iterate on an optimized version over time?

Comment 4

[Markus Stange [:mstange]]

At the work week roc, Jeff, Vlad and I agreed on doing it the other way round: Land what I have now, work on the skia implementation directly afterwards, and then upstream everything that our own software implementation does better than Skia into Skia so that we can get rid of our copy.

I'll put my updated patches up for review tomorrow. I spent the last week fixing bugs in the Direct2D backend, that's why this bug still looks dormant. (And there was a public holiday in Germany on Friday.)

(In reply to Andreas Gal :gal from comment #3)
> We use skia to land a 100-liner that
> implements this

It's going to be more than that - there's some stuff we need that Skia does not implement, and we want to move all filter processing code out to Moz2D so that we can have the nice API where we only pass one filter structure for painting the whole filter instead of calling repeatedly per primitive.

Comment 5

[Andreas Gal :gal]

Works for me.

Comment 6

[Markus Stange [:mstange]]

Cool!

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

FWIW, additional points in favour of taking Markus' code first in addition to comment #4:
-- His code has been green on try (at least, it was recently on Mac). We don't know that about the Skia filter code.
-- Making the Skia filter code timing-safe is not easy. For one thing, it uses SkScalar all over the place, which is float the way we build Skia (and the way everyone else builds Skia, says Jeff). So we'd need to undefine SK_SCALAR_IS_FLOAT or do a large search-and-replace job on the Skia filter code. Neither of those are very appealing especially in the short term.
-- Although this is a lot of new code, the filter implementation part rewrites and replaces a lot of our existing code (currently under SVG), and much of the glue code will be needed even if we use Skia. So it's not as big an increase in complexity as it looks.

> the GPU path likely has the same problems, and this is a lot of code and effort to prevent a problem that doesn't
> seem unique to this code.

Timing attacks are limited to places where we touch the pixel data of rendered Web content with non-constant-time operations. We don't compute over individual Web content pixels in very many places in our code.

I don't know how the GPU path is going to work out w.r.t. timing attacks. It may turn out that we face an irreconcilable tradeoff between filter performance and information leakage, and we may decide to resolve that in favour of filter performance. Even if so, I think it's still valuable to have a timing-safe software path that we know we can fall back to if we have to. We never know when something like this will blow up into a big issue. (CSS history sniffing is an example of an information leak we knew about and sat on for years before it suddenly become a big deal and every browser had to fix it (at a small performance and compatibility cost).)

Anyway, thanks for comment #5 and I'm looking forward to seeing patches for review here :-).

Comment 8

[Markus Stange [:mstange]]

Attachment: v1

Looks like this still isn't finished yet, but let's start the review process anyway.

There are some remaining issues with the D2D1 backend so I disabled it with some "#if 0"s for now so that we can land something. They are:
-- Tiled turbulence sometimes has gaps. I thought this was due to the crop filter in between, but the gaps were still there when I removed all crop filters.
-- Component transfer filters do something strange with premultiplication. The docs say that the input can be either premultiplied or unpremultiplied, and that the output is always premultiplied. However, I have no idea how the filter knows whether the input is premultiplied or unpremultiplied, and simply piping the result through an unpremultiply filter unconditionally did not work in some cases. Needs more investigation.

Other issues with this patch:
-- Currently does not compile on linux on Try due to warnings-as-errors because the SetAttribute methods in FilterNodeSoftware.h cause "overloaded-virtual" warnings. I guess we need tons of "using" declarations?
-- It failed to compile on Windows on Try without giving a reason in the error message. Bas, can you help me fix this? https://tbpl.mozilla.org/php/getParsedLog.php?id=30079494&tree=Try&full=1
-- I did not include the DrawTargetSkia and DrawTargetRecording changes in this patch.

Other stuff that needs fixing but can probably happen after the initial landing:
-- Lighting filters need to use fixed point arithmetic throughout. At the moment it's an unfinished mixture.
-- FilterProcessing files need comments about why this setup with the multiple source files was necessary. Basically, some compilers need a special flag when compiling with SSE2 instructions, but that flag can make them produce SSE2 instructions for some scalar code, too, so the flag must not be used for compiling the code that determines whether the user's CPU supports SSE2. Same for NEON.
-- There's more but I don't have a complete list right now.

Comment 9

[Robert Longson [:longsonr]]

> -- Component transfer filters do something strange with premultiplication.
> The docs say that the input can be either premultiplied or unpremultiplied,
> and that the output is always premultiplied. However, I have no idea how the
> filter knows whether the input is premultiplied or unpremultiplied, and
> simply piping the result through an unpremultiply filter unconditionally did
> not work in some cases. Needs more investigation.

We keep track of whether the surface we're working with contains unpremultiplied or premultipled data in the current SVG implementation. Each surface has a colour model see http://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGFilterInstance.cpp#480, http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGFilters.h#110 and http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGFEDisplacementMapElement.h#72

I think we have reftests for this: feDisplacementMap-alpha-01.svg and feDisplacementMap-colour-01-ref.svg from bug 584322 and bug 603584

Comment 10

[Markus Stange [:mstange]]

Thanks Robert, but I was actually referring to the Direct2D filters, e.g. http://msdn.microsoft.com/en-us/library/windows/desktop/hh706362%28v=vs.85%29.aspx
In bug 924103 I'm changing most of the code you referenced (mostly by moving it into FilterSupport).

Comment 11

[Markus Stange [:mstange]]

Attachment: v2

This fixes some bugs in the lighting normal calculation.

Comment 12

[Markus Stange [:mstange]]

Attachment: v3

Comment 13

[Markus Stange [:mstange]]

Attachment: part 1, v1: Add Point3D and Matrix5x4 types

Comment 14

[Markus Stange [:mstange]]

Attachment: part 2, v1: API

Comment 15

[Markus Stange [:mstange]]

Attachment: part 3, v1: Add SIMD.h with scalar + SSE2 implementations

Comment 16

[Markus Stange [:mstange]]

Attachment: part 4, v1: Add filter processing SIMD implementations + a few scalar implementations

Comment 17

[Markus Stange [:mstange]]

Attachment: part 5, v1: Add SVGTurbulenceRenderer

This class implements the algorithm described at https://dvcs.w3.org/hg/FXTF/raw-file/default/filters/index.html#feTurbulenceElement in a faster and hopefully more readable way. I put it into its own class because it's rather elaborate.

Comment 18

[Markus Stange [:mstange]]

Attachment: part 6, v1: Make AlphaBoxBlur accept separate X and Y blur radii

The blur filter will make use of AlphaBoxBlur, and it needs to support different blur radii for x and y.

Comment 19

[Markus Stange [:mstange]]

Attachment: part 7, v1: Add FilterNodeSoftware implementation

Comment 20

[Markus Stange [:mstange]]

Attachment: part 8, v1: Implement DrawTarget filter APIs with FilterNodeSoftware everywhere

Comment 21

[Markus Stange [:mstange]]

This is all for now. I'll attach the DrawTargetSkia and DrawTargetRecording implementations when I've extracted them.

Comment 22

[Bas Schouten (:bas.schouten)]

Comment on attachment 829251 [details] [diff] [review]
part 3, v1: Add SIMD.h with scalar + SSE2 implementations

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

I checked this for structure, not correctness. Looks good to me though. Eventually we'll probably want to split this up a little more somehow, but for now it's fine since it's nice and well-contained chaos.

Comment 23

[Bas Schouten (:bas.schouten)]

Comment on attachment 829251 [details] [diff] [review]
part 3, v1: Add SIMD.h with scalar + SSE2 implementations

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

If derf has some time to glance over this it would be of some value I feel. He knows more about this sort of thing than I do.

Comment 24

[Bas Schouten (:bas.schouten)]

Comment on attachment 829250 [details] [diff] [review]
part 2, v1: API

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

I wrote and designed a large part of this, I probably shouldn't be reviewing this alone. Matt, please sanity check :)

Comment 25

[Bas Schouten (:bas.schouten)]

Comment on attachment 829258 [details] [diff] [review]
part 4, v1: Add filter processing SIMD implementations + a few scalar implementations

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

The general idea looks great! I haven't verified in detail every single algorithm in here, I assume we've used tests to verify its correctness :-). I feel this could use a little more documentations, the couple of functions I looked at in detail it took me quite some time to figure out what they were supposed to do.

Just to be sure please use MOZ_ALWAYS_INLINE for the things where inlining is very important. Inline is ignored fairly often, at least in visual studio with our default build flags on windows.

::: gfx/2d/FilterProcessingSIMD-inl.h
@@ +303,5 @@
> +      i16x8_t s_bbbbgggg1234, s_rrrraaaa1234;
> +      i16x8_t d_bbbbgggg1234, d_rrrraaaa1234;
> +      UnpackAndShuffleComponents(s1234, s_bbbbgggg1234, s_rrrraaaa1234);
> +      UnpackAndShuffleComponents(d1234, d_bbbbgggg1234, d_rrrraaaa1234);
> +      // Shuffle for double high

These sort of comments could be a little more descriptive :).

@@ +607,5 @@
> +}
> +
> +template<typename i32x4_t, typename i16x8_t, uint32_t aCompositeOperator>
> +static inline i16x8_t
> +CompositeTwoPixels(i16x8_t source, i16x8_t sourceAlpha, i16x8_t dest, const i16x8_t& destAlpha)

Did you verify all our compilers inline this nicely and actually remove this switch? Otherwise you'll need to use template specializations without the switch :).

Comment 26

[Bas Schouten (:bas.schouten)]

Comment on attachment 829259 [details] [diff] [review]
part 5, v1: Add SVGTurbulenceRenderer

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

::: gfx/2d/SVGTurbulenceRenderer.cpp
@@ +52,5 @@
> +
> +} // unnamed namespace
> +
> +template<TurbulenceType Type, bool Stitch, typename T>
> +SVGTurbulenceRenderer<Type,Stitch,T>::SVGTurbulenceRenderer(const Size &aBaseFrequency, int32_t aSeed,

I feel this could probably get a lot of perf improvements from some SSE2 love. But I can imagine perf might not be that important here.

::: gfx/2d/SVGTurbulenceRenderer.h
@@ +13,5 @@
> +namespace gfx {
> +
> +// TODO: Add floatx4 functions to SIMD.h and use them instead of this vec4.
> +template<typename T>
> +struct vec4

Our stuctnames are capitalized everywhere else, they probably shouold be here as well.

Comment 27

[Bas Schouten (:bas.schouten)]

Comment on attachment 829260 [details] [diff] [review]
part 6, v1: Make AlphaBoxBlur accept separate X and Y blur radii

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

Well that was easy ...

Comment 28

[Bas Schouten (:bas.schouten)]

Comment on attachment 829261 [details] [diff] [review]
part 7, v1: Add FilterNodeSoftware implementation

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

This patch is going to take me a little bit more time to review, here's some initial comments.

::: gfx/2d/FilterNodeSoftware.cpp
@@ +177,5 @@
> +
> +// Fast approximate division by 255. It has the property that
> +// for all 0 <= n <= 255*255, FAST_DIVIDE_BY_255(n) == n/255.
> +// But it only uses two adds and two shifts instead of an
> +// integer division (which is expensive on many processors).

I saw these classes in other places as well, please ensure we share all of these.

You also probably want to MOZ_ALWAYS_INLINE this.

@@ +253,5 @@
> +  destData += DataOffset(aDest, aDestPoint);
> +
> +  if (BytesPerPixel(aSrc->GetFormat()) == 4) {
> +    for (int32_t y = 0; y < aSrcRect.height; y++) {
> +      for (int32_t x = 0; x < aSrcRect.width; x++) {

memcpy is significantly faster than this in my tests.

@@ +290,5 @@
> +  if (BytesPerPixel(aSurface->GetFormat()) == 4) {
> +    uint32_t sourcePixel = *(uint32_t*)sourcePixelData;
> +    for (int32_t y = 0; y < aFillRect.height; y++) {
> +      for (int32_t x = 0; x < aFillRect.width; x++) {
> +        *((uint32_t*)data + x) = sourcePixel;

Do one row, and memcpy it into the rest :). That'll be a lot faster.

@@ +317,5 @@
> +  data += DataOffset(aSurface, aFillRect.TopLeft());
> +  if (BytesPerPixel(aSurface->GetFormat()) == 4) {
> +    for (int32_t y = 0; y < aFillRect.height; y++) {
> +      for (int32_t x = 0; x < aFillRect.width; x++) {
> +        *((uint32_t*)data + x) = *((uint32_t*)sampleData + x);

memcpy!

Comment 29

[Markus Stange [:mstange]]

Clang turned these loops into memcpy for me, I assumed other compilers would do the same ;)

Comment 30

[Markus Stange [:mstange]]

Attachment: part 3, v2: Add SIMD.h with scalar + SSE2 implementations

This adds some Float32 APIs for SIMD turbulence rendering.

Comment 31

[Markus Stange [:mstange]]

Attachment: part 4, v2: Add SVGTurbulenceRenderer

This uses the float SIMD functions for turbulence rendering for a 2.5x speedup.

I've moved the implementation into an -inl.h header file because it needs to be included in FilterProcessingScalar.cpp and FilterProcessingSSE2.cpp which are compiled with different compiler flags.

Comment 32

[Markus Stange [:mstange]]

Attachment: part 5, v2: Add filter processing SIMD implementations + a few scalar implementations

I've tweaked some algorithms and added SIMD code for the arithmetic combine filter. I've also added some comments.

Comment 33

[Markus Stange [:mstange]]

Attachment: part 7, v2: Add FilterNodeSoftware implementation

I applied the PodCopy / PodZero improvements you suggested.

Comment 34

[Markus Stange [:mstange]]

Attachment: part 9, v1: Add Direct2D 1.1 implementation

Most of this code was written by you. Who should review that part?

There were some problems with the original code, here's how I fixed them:
-- If you had a SourceSurface that was the snapshot of a DrawTargetD2D, and the SourceSurface became detached because the DrawTarget went away, calling FilterNodeD2D::SetInput with that SourceSurface did not work because you couldn't get an ID2D1Image for it. So I made FilterNodeD2D take a reference to the DrawTarget it was created for, so that it can use that DrawTarget to get the right ID2D1Image for those SourceSurfaces. I made GetImageForSurface public on DrawTargetD2D1.
-- The convolve matrix filter had no way to respect the source rect attribute (which I had to add for SVG compatibility). I had to resort to some tricks for that, details are in the comments.
-- Component transfer filters premultiply in a way that doesn't really fit our use case. See the comments in the code for more details.
-- I moved FilterNode creation code into a static method of FilterNodeD2D1 so that it can be shared between DrawTargetD2D and DrawTargetD2D1.

I also added handling for some lighting enums, fixed up turbulence rendering by adding a rect attribute, and added the alpha mode attribute for the color matrix filter.

Comment 35

[Bas Schouten (:bas.schouten)]

Comment on attachment 8334059 [details] [diff] [review]
part 9, v1: Add Direct2D 1.1 implementation

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

Jeff, can you get to this before you go on PTO?

Comment 36

[Bas Schouten (:bas.schouten)]

Comment on attachment 8334047 [details] [diff] [review]
part 7, v2: Add FilterNodeSoftware implementation

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

::: gfx/2d/FilterNodeSoftware.cpp
@@ +289,5 @@
> +      *((uint32_t*)data + x) = sourcePixel;
> +    }
> +  } else if (BytesPerPixel(aSurface->GetFormat()) == 1) {
> +    uint8_t sourcePixel = *sourcePixelData;
> +    for (int32_t x = 0; x < aFillRect.width; x++) {

memset

@@ +344,5 @@
> +  } else if (BytesPerPixel(aSurface->GetFormat()) == 1) {
> +    for (int32_t y = 0; y < aFillRect.height; y++) {
> +      int8_t sampleColor = *sampleData;
> +      for (int32_t x = 0; x < aFillRect.width; x++) {
> +        data[x] = sampleColor;

memset

Comment 37

[Markus Stange [:mstange]]

(In reply to Bas Schouten (:bas.schouten) from comment #36)
> Comment on attachment 8334047 [details] [diff] [review]
> part 7, v2: Add FilterNodeSoftware implementation
> 
> Review of attachment 8334047 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/FilterNodeSoftware.cpp
> @@ +289,5 @@
> > +      *((uint32_t*)data + x) = sourcePixel;
> > +    }
> > +  } else if (BytesPerPixel(aSurface->GetFormat()) == 1) {
> > +    uint8_t sourcePixel = *sourcePixelData;
> > +    for (int32_t x = 0; x < aFillRect.width; x++) {
> 
> memset
> 
> @@ +344,5 @@
> > +  } else if (BytesPerPixel(aSurface->GetFormat()) == 1) {
> > +    for (int32_t y = 0; y < aFillRect.height; y++) {
> > +      int8_t sampleColor = *sampleData;
> > +      for (int32_t x = 0; x < aFillRect.width; x++) {
> > +        data[x] = sampleColor;
> 
> memset

How? I think I'd need something like memset_pattern4 for this.

Comment 38

[Markus Stange [:mstange]]

Attachment: part 7, v3: Add FilterNodeSoftware implementation

This fixes the blur source inflation size calculation to match up with the one in Blur.cpp.

Comment 39

[Markus Stange [:mstange]]

Attachment: part 10, v1: Add transform filter and remove offset filter

A transform filter simplifies the implementation of the <feImage> SVG filter.

Comment 40

[Bas Schouten (:bas.schouten)]

Comment on attachment 8334531 [details] [diff] [review]
part 10, v1: Add transform filter and remove offset filter

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

::: gfx/2d/FilterNodeSoftware.cpp
@@ +1017,5 @@
> +  dt->SetTransform(transform);
> +  dt->DrawSurface(input, r, r, DrawSurfaceOptions(mFilter));
> +
> +  RefPtr<SourceSurface> result = dt->Snapshot();
> +  RefPtr<DataSourceSurface> resultData = result->GetDataSurface();

This is very bad for D2D. (you'll trigger readback) I suspect we'll want to pick one backend that we know is fast with software and do CreateDrawTarget with that backend. Presumably Skia or Cairo. This also prevents the ugliness of passing the type around.

Comment 41

[Markus Stange [:mstange]]

Does the Moz2D standalone repo build system currently require at least Skia or Cairo to be enabled? If not, what should I do about that?

I agree that doing it this way with D2D is bad in the general case, but it's not as bad when the transform filter is the first in the chain. When we use software filters for D2D source surfaces, we need to do readback at some point anyway, and doing it after the transform will be faster because then we won't need to do the transform in software.
Transforms that are simple translations don't hit the readback path because they take the early exit with
>  if (transform.IsIdentity() && srcRect.Size() == aRect.Size()) {
>    return input;
>  }

Comment 42

[Markus Stange [:mstange]]

... and the patches in bug 924103 will only use non-offset transform filters that apply directly to input source surfaces, and not in the middle of a filter chain.

Comment 43

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8334059 [details] [diff] [review]
part 9, v1: Add Direct2D 1.1 implementation

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

::: gfx/2d/FilterNodeD2D1.cpp
@@ +179,5 @@
> +void ConvertValue(uint32_t aType, uint32_t aAttribute, IntSize &aValue)
> +{
> +  switch (aType) {
> +  case FILTER_MORPHOLOGY:
> +    if (aAttribute == ATT_MORPHOLOGY_RADII) {

Can you add a comment about what this is for?

Comment 44

[Markus Stange [:mstange]]

Attachment: part 10, v2: Add transform filter and remove offset filter

This uses BACKEND_CAIRO always. DrawTargetSkia::DrawSurface currently does not support drawing pure DataSourceSurfaces.

Comment 45

[Markus Stange [:mstange]]

Attachment: part 10, v3: Add transform filter and remove offset filter

On the D2D1 transform filter we need to set border mode to "hard", because an <feImage> with an image that's just a single opaque pixel is expected to stay opaque when upscaled - it shouldn't have transparent fringes. This is tested by the reftest anim-feImage-preserveAspectRatio-01.svg.

Comment 46

[u480271]

(In reply to Markus Stange [:mstange] from comment #37)
> (In reply to Bas Schouten (:bas.schouten) from comment #36)
> > Comment on attachment 8334047 [details] [diff] [review]
> > part 7, v2: Add FilterNodeSoftware implementation
> > 
> > Review of attachment 8334047 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/2d/FilterNodeSoftware.cpp
> > @@ +289,5 @@
> > > +      *((uint32_t*)data + x) = sourcePixel;
> > > +    }
> > > +  } else if (BytesPerPixel(aSurface->GetFormat()) == 1) {
> > > +    uint8_t sourcePixel = *sourcePixelData;
> > > +    for (int32_t x = 0; x < aFillRect.width; x++) {
> > 
> > memset
> > 
> > @@ +344,5 @@
> > > +  } else if (BytesPerPixel(aSurface->GetFormat()) == 1) {
> > > +    for (int32_t y = 0; y < aFillRect.height; y++) {
> > > +      int8_t sampleColor = *sampleData;
> > > +      for (int32_t x = 0; x < aFillRect.width; x++) {
> > > +        data[x] = sampleColor;
> > 
> > memset
> 
> How? I think I'd need something like memset_pattern4 for this.

Isn't byte being write to data? memset(data, sampleColor, aFillRect.width), no?

Comment 47

[Markus Stange [:mstange]]

You're right! Somehow I missed that.

Comment 48

[Markus Stange [:mstange]]

Attachment: part 11, v1: Stub out filter APIs on DrawTargetRecording and DrawTargetSkia

I'll deal with these after the initial landing. At the moment RecordedEvent.cpp isn't completely in sync with moz2d (it's missing http://hg.mozilla.org/users/bschouten_mozilla.com/moz2d/rev/5fe087148226 and http://hg.mozilla.org/users/bschouten_mozilla.com/moz2d/rev/ae3b33c3f929 ) and DrawTargetSkia can't handle non-Skia SourceSurfaces in DrawSurface.

Comment 49

[Bas Schouten (:bas.schouten)]

Comment on attachment 8336076 [details] [diff] [review]
part 11, v1: Stub out filter APIs on DrawTargetRecording and DrawTargetSkia

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

I'd prefer if we just uploaded the code needed from Moz2D and skip this step.

Comment 50

[Markus Stange [:mstange]]

Attachment: part 12, v1: Make sure we're only calling DrawSurface with SURFACE_DATA surfaces

Comment 51

[Markus Stange [:mstange]]

Attachment: part 11, v1: Make DrawTargetSkia::DrawSurface deal with SURFACE_DATA DataSourceSurfaces

Most of this code is from you, but I converted a few more places to use GetBitmapForSurface and I added a MOZ_CRASH in the non-SURFACE_DATA case so that we don't silently ignore incompatible surfaces. This behavior is consistent with the CG in the patch for bug 930956.

Comment 52

[Markus Stange [:mstange]]

Attachment: part 13, v1: Use FilterNodeSoftware in Skia.

Bas wrote this patch and I reviewed it.

Comment 53

[Markus Stange [:mstange]]

Attachment: part 14, v1: Support filter recording.

Bas wrote this patch and I reviewed it.

I made a small change: I renamed the TYPE_* enum values to ARGTYPE_* because some Mac system header (/usr/include/ConditionalMacros.h) does #define TYPE_BOOL 1. The compiler didn't tell me how that header was included and I didn't bother to find out.

Comment 54

[Bas Schouten (:bas.schouten)]

Comment on attachment 8336739 [details] [diff] [review]
part 12, v1: Make sure we're only calling DrawSurface with SURFACE_DATA surfaces

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

::: gfx/2d/FilterNodeSoftware.cpp
@@ +594,5 @@
>    printf("</pre>\n");
>  #endif
>  
> +  RefPtr<DataSourceSurface> dataSurface = result;
> +  if (dataSurface->GetType() != SURFACE_DATA) {

I'm confused, by this, GetDataSurface() on DataSourceSurface will simply return 'this'. If we're able to access Stride and Data on a surface, the type should always be SURFACE_DATA. But regardless, our drawing backends should simply be doing a GetDataSurface() on whatever they don't know how to draw. (This could be slow, but that's a risk for the caller) If they're not doing that that's a bug we should probably fix.

Comment 55

[Markus Stange [:mstange]]

Comment on attachment 8336739 [details] [diff] [review]
part 12, v1: Make sure we're only calling DrawSurface with SURFACE_DATA surfaces

With the patches in bug 943614 this part is no longer needed.

Comment 56

[Markus Stange [:mstange]]

https://tbpl.mozilla.org/?tree=Try&rev=27eeb006ada4

Comment 57

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5bc7ed8af53
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e9afaacf242
https://hg.mozilla.org/integration/mozilla-inbound/rev/c57ffc30cd43
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f39d041ccb
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a32026fcfcf
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f470ad20b6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d737d52c4cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/272bb40f463f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fe4a520bf31
https://hg.mozilla.org/integration/mozilla-inbound/rev/172e1c46f2d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/63c02ba3a94e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e59bb20fc8dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/77ba34a72f88

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5bc7ed8af53
https://hg.mozilla.org/mozilla-central/rev/6e9afaacf242
https://hg.mozilla.org/mozilla-central/rev/c57ffc30cd43
https://hg.mozilla.org/mozilla-central/rev/d1f39d041ccb
https://hg.mozilla.org/mozilla-central/rev/5a32026fcfcf
https://hg.mozilla.org/mozilla-central/rev/7f470ad20b6b
https://hg.mozilla.org/mozilla-central/rev/8d737d52c4cb
https://hg.mozilla.org/mozilla-central/rev/272bb40f463f
https://hg.mozilla.org/mozilla-central/rev/9fe4a520bf31
https://hg.mozilla.org/mozilla-central/rev/172e1c46f2d6
https://hg.mozilla.org/mozilla-central/rev/63c02ba3a94e
https://hg.mozilla.org/mozilla-central/rev/e59bb20fc8dc
https://hg.mozilla.org/mozilla-central/rev/77ba34a72f88

Comment 59

[Kevin Simons [:kevsim] (Telenor)]

Patch part 11 has a side-effect of making skia unusable on Linux because DrawSurface is called frequently with a cairo surface (from gfxXlibNativeRenderer.cpp and friends) and the patch introduces a MOZ_CRASH for that condition. The fact that these parts still use cairo seems to be why the chrome (tabs, etc.) look so bad on skia on linux at the moment.

I tried a quick hack of converting the cairo surfaces to DataSourceSurface in DrawTargetSkia and that fixes the crash and also makes the chrome look good. Obviously though this is not the optimal fix.

Should we however introduce this as a temporary work around until the hardcoded cairo parts can be removed from thebes for gtk/x11?

Comment 60

[Markus Stange [:mstange]]

Sounds good to me. Can you please open a new bug for this problem?

Comment 61

[Kevin Simons [:kevsim] (Telenor)]

Done: https://bugzilla.mozilla.org/show_bug.cgi?id=945314

Comment 62

[ISHIKAWA, Chiaki]

Hi,

I noticed an uninitialized value usage when I ran thunderbird (comm-central)
under valgrind.

The reported backtrace suggests that it is caused by 
function
DoUnpremultipcationCalculation_SSE2
(and eventually the inlined
DoUnpremultiplicationCalculation_SIMD )
introduced by patch(es) in this bugzilla entry.

I wonder if someone can take a look at 
Bug 964827 - Use of uninitialized value in 2D filtering code.

TIA

Comment 63

[u279076]

De-prioritizing QA verification of this bug. Please remove the [qa-] whiteboard tag and add the verifyme keyword if you think this needs our attention before Firefox 28 is released.