688613 - disable questionable mobile optimizations

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

- disable NN filtering and go back to bilinear filtering which gives better results and seems to perform reasonably well (looking for more test cases and devices, but I don't see a performance difference on my Galaxy S 2)
- use 24-bit surfaces by default, recent devices have 24-bit frame buffers and using 16-bit surfaces isn't any faster best case, and can hit horrible slow paths worst case

Comment 3

[Andreas Gal :gal]

Comment on attachment 561884 [details] [diff] [review]
patch

cjones, who is a good reviewer for this?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I would recommend breaking these into separate patches and requesting
review from the author noted in the blame.  If the author can give a
testcase where the patch makes a measurable difference, we should
consider that against possible quality tradeoff.  If there's no
testcase, let's just land it until data says otherwise (and we can add
regression tests!).

>diff --git a/gfx/layers/Layers.cpp b/gfx/layers/Layers.cpp
>      useIntermediateSurface = PR_TRUE;   
>-    } else if (
>-#ifdef MOZ_GFX_OPTIMIZE_MOBILE
>-        !contTransform.PreservesAxisAlignedRectangles()) {
>-#else
>-        contTransform.HasNonIntegerTranslation()) {
>-#endif
>+    } else if (contTransform.HasNonIntegerTranslation()) {

I think this was working around a problem with the way we implemented
zoom for fennec in the layer tree, which we've fixed.  Pretty sure it
can go.

>diff --git a/gfx/layers/ThebesLayerBuffer.cpp b/gfx/layers/ThebesLayerBuffer.cpp
>@@ -83,21 +83,16 @@ ThebesLayerBuffer::DrawBufferQuadrant(gf
> 
>-#ifdef MOZ_GFX_OPTIMIZE_MOBILE
>-  gfxPattern::GraphicsFilter filter = gfxPattern::FILTER_NEAREST;
>-  pattern->SetFilter(filter);
>-#endif
>-

This is on the critical repaint path in the UI when we're using CPU
compositing.  Since this landed, bilinear scaling has been optimized
in pixman so it might be fast enough (on chips with NEON).  Need data.

>diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp
>@@ -708,23 +708,21 @@ BasicThebesLayer::PaintThebes(gfxContext
>-#ifndef MOZ_GFX_OPTIMIZE_MOBILE
>     gfxMatrix transform;
>     if (!GetEffectiveTransform().CanDraw2D(&transform) ||
>         transform.HasNonIntegerTranslation()) {
>       flags |= ThebesLayerBuffer::PAINT_WILL_RESAMPLE;
>     }
>-#endif

I think this is just trying to skip the transform check to avoid dealing with
floats, because we always set PAINT_WILL_RESAMPLE on shadowed layers.  We
should check HasShadowManager() instead of ifdef hackery.

>diff --git a/gfx/layers/ipc/ShadowLayers.cpp b/gfx/layers/ipc/ShadowLayers.cpp
> static gfxASurface::gfxImageFormat
> OptimalFormatFor(gfxASurface::gfxContentType aContent)
> {
>   switch (aContent) {
>   case gfxASurface::CONTENT_COLOR:
>-#ifdef MOZ_GFX_OPTIMIZE_MOBILE
>-    return gfxASurface::ImageFormatRGB16_565;
>-#else
>     return gfxASurface::ImageFormatRGB24;
>-#endif

Evidence suggests going to r8g8b8 is likely to be a win on modern HW,
especially with GL compositing.  We might need to tune this for older
HW.  Until we have a good way to check memory bandwidth dynamically
(i.e. autotune) and a solid perf-measurement infra, I'm fine with
going to 24 bit.

>diff --git a/gfx/layers/opengl/ThebesLayerOGL.cpp b/gfx/layers/opengl/ThebesLayerOGL.cpp
>@@ -438,25 +438,21 @@ BasicBufferOGL::BeginPaint(ContentType a
>     if (mode == Layer::SURFACE_COMPONENT_ALPHA) {
>-#ifdef MOZ_GFX_OPTIMIZE_MOBILE
>-      mode = Layer::SURFACE_SINGLE_CHANNEL_ALPHA;
>-#else

I don't understand what this is doing.

>@@ -745,26 +741,24 @@ ThebesLayerOGL::RenderLayer(int aPreviou
>   PRUint32 flags = 0;
>-#ifndef MOZ_GFX_OPTIMIZE_MOBILE
>   gfxMatrix transform2d;
>   if (GetEffectiveTransform().Is2D(&transform2d)) {
>     if (transform2d.HasNonIntegerTranslation()) {
>       flags |= ThebesLayerBufferOGL::PAINT_WILL_RESAMPLE;
>     }
>   } else {
>     flags |= ThebesLayerBufferOGL::PAINT_WILL_RESAMPLE;
>   }
>-#endif

Same hack as in BasicThebesLayer::PaintThebes.

>diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
>   PRBool ignoreScale = PR_FALSE;
>-#ifdef MOZ_GFX_OPTIMIZE_MOBILE
>-  ignoreScale = PR_TRUE;
>-#endif

I don't understand that one well enough to comment.

>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>@@ -4405,21 +4405,17 @@ PresShell::RenderDocument(const nsRect& 
>   aThebesContext->NewPath();
>-#ifdef MOZ_GFX_OPTIMIZE_MOBILE
>-  aThebesContext->Rectangle(r, PR_TRUE);
>-#else
>   aThebesContext->Rectangle(r);
>-#endif
> 

This used to be on the critical repaint path with the tilecanvas
frontend, but we don't really care about this anymore.  I think this
would have caused seaming anyway in that frontend.

>diff --git a/layout/generic/nsObjectFrame.cpp b/layout/generic/nsObjectFrame.cpp
>@@ -1628,22 +1628,16 @@ nsObjectFrame::BuildLayer(nsDisplayListB
>-#ifdef MOZ_GFX_OPTIMIZE_MOBILE
>-    if (!aManager->IsCompositingCheap()) {
>-      // Pixman just horrible with bilinear filter scaling
>-      filter = gfxPattern::FILTER_NEAREST;
>-    }
>-#endif

Pretty sure the NEON optimizations landed after this was pushed.  Need
data.  We don't really care about this on android either way atm,
though with dougt's/blassey's stuff we might soon.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> I would recommend breaking these into separate patches and requesting
> review from the author noted in the blame.  If the author can give a
> testcase where the patch makes a measurable difference, we should
> consider that against possible quality tradeoff.  If there's no
> testcase, let's just land it until data says otherwise (and we can add
> regression tests!).

I agree with this

Comment 6

[Andreas Gal :gal]

Sounds easy enough, will do

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> >diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp
> >@@ -708,23 +708,21 @@ BasicThebesLayer::PaintThebes(gfxContext
> >-#ifndef MOZ_GFX_OPTIMIZE_MOBILE
> >     gfxMatrix transform;
> >     if (!GetEffectiveTransform().CanDraw2D(&transform) ||
> >         transform.HasNonIntegerTranslation()) {
> >       flags |= ThebesLayerBuffer::PAINT_WILL_RESAMPLE;
> >     }
> >-#endif
> 

Looking back over bug 634759 reminded me what's going on here.  We have an optimization in which we can "rotate" a buffer when repainting part of it after scrolling, instead of moving all the pixels.  Then when drawing, we "unrotate" the buffer by blitting the rotated parts to where they're supposed to be.  This can cause issues when the buffer is painted with a scale transform because we'll resample across the rotation boundaries, causing artifacts.

The decision in bug 634759 was to live with these artifacts since they'll be temporary (the next content repaint after the zoom will eliminate them).  It's very easy to find tests where this makes a difference.  I'm not a big fan of the ifdef but it's not a big deal either.

We should definitely add a comment here.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:bhood, could you have a look please?
For more information, please visit auto_nag documentation.