Bug 710096 - Hold the monitor on the layer controller when adjusting and rendering the viewport metrics. r=Cwiiis a=java-only
authorPatrick Walton <pwalton@mozilla.com>
Tue, 13 Dec 2011 14:43:08 -0800
changeset 84171 58a2785556801c69aa7041f470f8b284cfcf1074
parent 84170 1f03a95b661ae74e668ed107670f0f1a72b1d60a
child 84172 9fae755f51032787bebda127e3bf39a9f364386b
push id519
push userakeybl@mozilla.com
push dateWed, 01 Feb 2012 00:38:35 +0000
treeherdermozilla-beta@788ea1ef610b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersCwiiis, java-only
bugs710096
milestone11.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 710096 - Hold the monitor on the layer controller when adjusting and rendering the viewport metrics. r=Cwiiis a=java-only
mobile/android/base/gfx/GeckoSoftwareLayerClient.java
mobile/android/base/gfx/LayerController.java
mobile/android/base/gfx/LayerRenderer.java
mobile/android/base/ui/PanZoomController.java
--- a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
+++ b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
@@ -157,38 +157,32 @@ public class GeckoSoftwareLayerClient ex
             // save and restore the viewport size stored in java; never let the
             // JS-side viewport dimensions override the java-side ones because
             // java is the One True Source of this information, and allowing JS
             // to override can lead to race conditions where this data gets clobbered.
             FloatSize viewportSize = getLayerController().getViewportSize();
             mGeckoViewport = new ViewportMetrics(viewportObject);
             mGeckoViewport.setSize(viewportSize);
 
-            mTileLayer.setOrigin(PointUtils.round(mGeckoViewport.getDisplayportOrigin()));
-            mTileLayer.setResolution(mGeckoViewport.getZoomFactor());
-
-            // Make sure LayerController metrics changes only happen in the
-            // UI thread.
-            final LayerController controller = getLayerController();
+            LayerController controller = getLayerController();
+            synchronized (controller) {
+                PointF displayportOrigin = mGeckoViewport.getDisplayportOrigin();
+                mTileLayer.setOrigin(PointUtils.round(displayportOrigin));
+                mTileLayer.setResolution(mGeckoViewport.getZoomFactor());
 
-            if (controller != null) {
-                controller.post(new Runnable() {
-                    @Override
-                    public void run() {
-                        if (onlyUpdatePageSize) {
-                            // Don't adjust page size when zooming unless zoom levels are
-                            // approximately equal.
-                            if (FloatUtils.fuzzyEquals(controller.getZoomFactor(), mGeckoViewport.getZoomFactor()))
-                                controller.setPageSize(mGeckoViewport.getPageSize());
-                        } else {
-                            controller.setViewportMetrics(mGeckoViewport);
-                            controller.notifyPanZoomControllerOfGeometryChange(true);
-                        }
-                    }
-                });
+                if (onlyUpdatePageSize) {
+                    // Don't adjust page size when zooming unless zoom levels are
+                    // approximately equal.
+                    if (FloatUtils.fuzzyEquals(controller.getZoomFactor(),
+                            mGeckoViewport.getZoomFactor()))
+                        controller.setPageSize(mGeckoViewport.getPageSize());
+                } else {
+                    controller.setViewportMetrics(mGeckoViewport);
+                    controller.notifyPanZoomControllerOfGeometryChange(true);
+                }
             }
         } catch (JSONException e) {
             Log.e(LOGTAG, "Bad viewport description: " + viewportDescription);
             throw new RuntimeException(e);
         }
     }
 
     /*
--- a/mobile/android/base/gfx/LayerController.java
+++ b/mobile/android/base/gfx/LayerController.java
@@ -53,22 +53,23 @@ import android.graphics.PointF;
 import android.graphics.Rect;
 import android.graphics.RectF;
 import android.util.Log;
 import android.view.MotionEvent;
 import android.view.GestureDetector;
 import android.view.ScaleGestureDetector;
 import android.view.View.OnTouchListener;
 import java.lang.Math;
-import java.util.ArrayList;
 
 /**
  * The layer controller manages a tile that represents the visible page. It does panning and
  * zooming natively by delegating to a panning/zooming controller. Touch events can be dispatched
  * to a higher-level view.
+ *
+ * Many methods require that the monitor be held, with a synchronized (controller) { ... } block.
  */
 public class LayerController {
     private static final String LOGTAG = "GeckoLayerController";
 
     private Layer mRootLayer;                   /* The root layer. */
     private LayerView mView;                    /* The main rendering view. */
     private Context mContext;                   /* The current context. */
     private ViewportMetrics mViewportMetrics;   /* The current viewport metrics. */
@@ -150,17 +151,18 @@ public class LayerController {
         Resources resources = mContext.getResources();
         int resourceID = resources.getIdentifier(name, "drawable", mContext.getPackageName());
         BitmapFactory.Options options = new BitmapFactory.Options();
         options.inScaled = false;
         return BitmapFactory.decodeResource(mContext.getResources(), resourceID, options);
     }
 
     /**
-     * The view calls this to indicate that the viewport changed size.
+     * The view calls this function to indicate that the viewport changed size. It must hold the
+     * monitor while calling it.
      *
      * TODO: Refactor this to use an interface. Expose that interface only to the view and not
      * to the layer client. That way, the layer client won't be tempted to call this, which might
      * result in an infinite loop.
      */
     public void setViewportSize(FloatSize size) {
         mViewportMetrics.setSize(size);
         setForceRedraw();
@@ -168,80 +170,94 @@ public class LayerController {
         if (mLayerClient != null)
             mLayerClient.viewportSizeChanged();
 
         notifyLayerClientOfGeometryChange();
         mPanZoomController.geometryChanged(true);
         mView.requestRender();
     }
 
+    /** Scrolls the viewport to the given point. You must hold the monitor while calling this. */
     public void scrollTo(PointF point) {
         mViewportMetrics.setOrigin(point);
         notifyLayerClientOfGeometryChange();
         mPanZoomController.geometryChanged(false);
         GeckoApp.mAppContext.repositionPluginViews(false);
         mView.requestRender();
     }
 
+    /** Scrolls the viewport by the given offset. You must hold the monitor while calling this. */
     public void scrollBy(PointF point) {
         PointF origin = mViewportMetrics.getOrigin();
         origin.offset(point.x, point.y);
         mViewportMetrics.setOrigin(origin);
 
         notifyLayerClientOfGeometryChange();
         mPanZoomController.geometryChanged(false);
         GeckoApp.mAppContext.repositionPluginViews(false);
         mView.requestRender();
     }
 
+    /** Sets the current viewport. You must hold the monitor while calling this. */
     public void setViewport(RectF viewport) {
         mViewportMetrics.setViewport(viewport);
         notifyLayerClientOfGeometryChange();
         mPanZoomController.geometryChanged(false);
         GeckoApp.mAppContext.repositionPluginViews(false);
         mView.requestRender();
     }
 
+    /** Sets the current page size. You must hold the monitor while calling this. */
     public void setPageSize(FloatSize size) {
         if (mViewportMetrics.getPageSize().fuzzyEquals(size))
             return;
 
         mViewportMetrics.setPageSize(size);
 
         // Page size is owned by the LayerClient, so no need to notify it of
         // this change.
         mPanZoomController.geometryChanged(false);
         mView.requestRender();
     }
 
     /**
      * Sets the entire viewport metrics at once. This function does not notify the layer client or
      * the pan/zoom controller, so you will need to call notifyLayerClientOfGeometryChange() or
-     * notifyPanZoomControllerOfGeometryChange() after calling this.
+     * notifyPanZoomControllerOfGeometryChange() after calling this. You must hold the monitor
+     * while calling this.
      */
     public void setViewportMetrics(ViewportMetrics viewport) {
         mViewportMetrics = new ViewportMetrics(viewport);
         GeckoApp.mAppContext.repositionPluginViews(false);
         mView.requestRender();
     }
 
+    /** Scales the viewport. You must hold the monitor while calling this. */
     public void scaleTo(float zoomFactor) {
         scaleWithFocus(zoomFactor, new PointF(0,0));
     }
 
+    /**
+     * Scales the viewport, keeping the given focus point in the same place before and after the
+     * scale operation. You must hold the monitor while calling this.
+     */
     public void scaleWithFocus(float zoomFactor, PointF focus) {
         mViewportMetrics.scaleTo(zoomFactor, focus);
 
         // We assume the zoom level will only be modified by the
         // PanZoomController, so no need to notify it of this change.
         notifyLayerClientOfGeometryChange();
         GeckoApp.mAppContext.repositionPluginViews(false);
         mView.requestRender();
     }
 
+    /**
+     * Sets the viewport origin and scales in one operation. You must hold the monitor while
+     * calling this.
+     */
     public void scaleWithOrigin(float zoomFactor, PointF origin) {
         mViewportMetrics.setOrigin(origin);
         scaleTo(zoomFactor);
     }
 
     public boolean post(Runnable action) { return mView.post(action); }
 
     public void setOnTouchListener(OnTouchListener onTouchListener) {
--- a/mobile/android/base/gfx/LayerRenderer.java
+++ b/mobile/android/base/gfx/LayerRenderer.java
@@ -127,83 +127,85 @@ public class LayerRenderer implements GL
         gl.glHint(GL10.GL_PERSPECTIVE_CORRECTION_HINT, GL10.GL_FASTEST);
         gl.glShadeModel(GL10.GL_SMOOTH);    /* FIXME: Is this needed? */
         gl.glDisable(GL10.GL_DITHER);
         gl.glEnable(GL10.GL_TEXTURE_2D);
     }
 
     /**
      * Called whenever a new frame is about to be drawn.
-     *
-     * FIXME: This is racy. Layers and page sizes can be modified by the pan/zoom controller while
-     * this is going on.
      */
     public void onDrawFrame(GL10 gl) {
         long frameStartTime = SystemClock.uptimeMillis();
 
         TextureReaper.get().reap(gl);
 
         LayerController controller = mView.getController();
-        Layer rootLayer = controller.getRoot();
-        RenderContext screenContext = createScreenContext(), pageContext = createPageContext();
+        RenderContext screenContext = createScreenContext();
+
+        synchronized (controller) {
+            Layer rootLayer = controller.getRoot();
+            RenderContext pageContext = createPageContext();
 
-        if (!pageContext.fuzzyEquals(mLastPageContext)) {
-            // the viewport or page changed, so show the scrollbars again
-            // as per UX decision
-            mVertScrollLayer.unfade();
-            mHorizScrollLayer.unfade();
-            mFadeRunnable.scheduleStartFade(ScrollbarLayer.FADE_DELAY);
-        } else if (mFadeRunnable.timeToFade()) {
-            boolean stillFading = mVertScrollLayer.fade() | mHorizScrollLayer.fade();
-            if (stillFading) {
-                mFadeRunnable.scheduleNextFadeFrame();
+            if (!pageContext.fuzzyEquals(mLastPageContext)) {
+                // the viewport or page changed, so show the scrollbars again
+                // as per UX decision
+                mVertScrollLayer.unfade();
+                mHorizScrollLayer.unfade();
+                mFadeRunnable.scheduleStartFade(ScrollbarLayer.FADE_DELAY);
+            } else if (mFadeRunnable.timeToFade()) {
+                boolean stillFading = mVertScrollLayer.fade() | mHorizScrollLayer.fade();
+                if (stillFading) {
+                    mFadeRunnable.scheduleNextFadeFrame();
+                }
             }
-        }
-        mLastPageContext = pageContext;
+            mLastPageContext = pageContext;
 
-        /* Update layers. */
-        if (rootLayer != null) rootLayer.update(gl);
-        mShadowLayer.update(gl);
-        mCheckerboardLayer.update(gl);
-        mFrameRateLayer.update(gl);
-        mVertScrollLayer.update(gl);
-        mHorizScrollLayer.update(gl);
-
-        /* Draw the background. */
-        gl.glClearColor(BACKGROUND_COLOR_R, BACKGROUND_COLOR_G, BACKGROUND_COLOR_B, 1.0f);
-        gl.glClear(GL10.GL_COLOR_BUFFER_BIT);
+            /* Update layers. */
+            if (rootLayer != null) rootLayer.update(gl);
+            mShadowLayer.update(gl);
+            mCheckerboardLayer.update(gl);
+            mFrameRateLayer.update(gl);
+            mVertScrollLayer.update(gl);
+            mHorizScrollLayer.update(gl);
 
-        /* Draw the drop shadow, if we need to. */
-        Rect pageRect = getPageRect();
-        RectF untransformedPageRect = new RectF(0.0f, 0.0f, pageRect.width(), pageRect.height());
-        if (!untransformedPageRect.contains(controller.getViewport()))
-            mShadowLayer.draw(pageContext);
+            /* Draw the background. */
+            gl.glClearColor(BACKGROUND_COLOR_R, BACKGROUND_COLOR_G, BACKGROUND_COLOR_B, 1.0f);
+            gl.glClear(GL10.GL_COLOR_BUFFER_BIT);
 
-        /* Draw the checkerboard. */
-        Rect scissorRect = transformToScissorRect(pageRect);
-        gl.glEnable(GL10.GL_SCISSOR_TEST);
-        gl.glScissor(scissorRect.left, scissorRect.top,
-                     scissorRect.width(), scissorRect.height());
+            /* Draw the drop shadow, if we need to. */
+            Rect pageRect = getPageRect();
+            RectF untransformedPageRect = new RectF(0.0f, 0.0f, pageRect.width(),
+                                                    pageRect.height());
+            if (!untransformedPageRect.contains(controller.getViewport()))
+                mShadowLayer.draw(pageContext);
 
-        mCheckerboardLayer.draw(screenContext);
+            /* Draw the checkerboard. */
+            Rect scissorRect = transformToScissorRect(pageRect);
+            gl.glEnable(GL10.GL_SCISSOR_TEST);
+            gl.glScissor(scissorRect.left, scissorRect.top,
+                         scissorRect.width(), scissorRect.height());
 
-        /* Draw the layer the client added to us. */
-        if (rootLayer != null)
-            rootLayer.draw(pageContext);
+            mCheckerboardLayer.draw(screenContext);
 
-        gl.glDisable(GL10.GL_SCISSOR_TEST);
+            /* Draw the layer the client added to us. */
+            if (rootLayer != null)
+                rootLayer.draw(pageContext);
+
+            gl.glDisable(GL10.GL_SCISSOR_TEST);
 
-        /* Draw the vertical scrollbar. */
-        IntSize screenSize = new IntSize(controller.getViewportSize());
-        if (pageRect.height() > screenSize.height)
-            mVertScrollLayer.draw(pageContext);
+            /* Draw the vertical scrollbar. */
+            IntSize screenSize = new IntSize(controller.getViewportSize());
+            if (pageRect.height() > screenSize.height)
+                mVertScrollLayer.draw(pageContext);
 
-        /* Draw the horizontal scrollbar. */
-        if (pageRect.width() > screenSize.width)
-            mHorizScrollLayer.draw(pageContext);
+            /* Draw the horizontal scrollbar. */
+            if (pageRect.width() > screenSize.width)
+                mHorizScrollLayer.draw(pageContext);
+        }
 
         /* Draw the FPS. */
         if (mShowFrameRate) {
             updateDroppedFrames(frameStartTime);
             try {
                 gl.glEnable(GL10.GL_BLEND);
                 mFrameRateLayer.draw(screenContext);
             } finally {
--- a/mobile/android/base/ui/PanZoomController.java
+++ b/mobile/android/base/ui/PanZoomController.java
@@ -555,17 +555,19 @@ public class PanZoomController
             } catch (JSONException e) {
                 Log.e(LOGTAG, "Error forming Gesture:Scroll message: " + e);
             }
 
             GeckoEvent e = new GeckoEvent("Gesture:Scroll", json.toString());
             GeckoAppShell.sendEventToGecko(e);
             mOverrideScrollAck = false;
         } else {
-            mController.scrollBy(new PointF(mX.displacement, mY.displacement));
+            synchronized (mController) {
+                mController.scrollBy(new PointF(mX.displacement, mY.displacement));
+            }
         }
 
         mX.displacement = mY.displacement = 0;
     }
 
     /* The callback that performs the bounce animation. */
     private class BounceRunnable implements Runnable {
         public void run() {
@@ -587,28 +589,32 @@ public class PanZoomController
 
             /* Finally, if there's nothing else to do, complete the animation and go to sleep. */
             finishBounce();
             finishAnimation();
         }
 
         /* Performs one frame of a bounce animation. */
         private void advanceBounce() {
-            float t = EASE_OUT_ANIMATION_FRAMES[mBounceFrame];
-            ViewportMetrics newMetrics = mBounceStartMetrics.interpolate(mBounceEndMetrics, t);
-            mController.setViewportMetrics(newMetrics);
-            mController.notifyLayerClientOfGeometryChange();
-            mBounceFrame++;
+            synchronized (mController) {
+                float t = EASE_OUT_ANIMATION_FRAMES[mBounceFrame];
+                ViewportMetrics newMetrics = mBounceStartMetrics.interpolate(mBounceEndMetrics, t);
+                mController.setViewportMetrics(newMetrics);
+                mController.notifyLayerClientOfGeometryChange();
+                mBounceFrame++;
+            }
         }
 
         /* Concludes a bounce animation and snaps the viewport into place. */
         private void finishBounce() {
-            mController.setViewportMetrics(mBounceEndMetrics);
-            mController.notifyLayerClientOfGeometryChange();
-            mBounceFrame = -1;
+            synchronized (mController) {
+                mController.setViewportMetrics(mBounceEndMetrics);
+                mController.notifyLayerClientOfGeometryChange();
+                mBounceFrame = -1;
+            }
         }
     }
 
     // The callback that performs the fling animation.
     private class FlingRunnable implements Runnable {
         public void run() {
             /*
              * The pan/zoom controller might have signaled to us that it wants to abort the
@@ -876,21 +882,24 @@ public class PanZoomController
          * factor toward 1.0.
          */
         float resistance = Math.min(mX.getEdgeResistance(), mY.getEdgeResistance());
         if (spanRatio > 1.0f)
             spanRatio = 1.0f + (spanRatio - 1.0f) * resistance;
         else
             spanRatio = 1.0f - (1.0f - spanRatio) * resistance;
 
-        float newZoomFactor = mController.getZoomFactor() * spanRatio;
+        synchronized (mController) {
+            float newZoomFactor = mController.getZoomFactor() * spanRatio;
 
-        mController.scrollBy(new PointF(mLastZoomFocus.x - detector.getFocusX(),
-                                        mLastZoomFocus.y - detector.getFocusY()));
-        mController.scaleWithFocus(newZoomFactor, new PointF(detector.getFocusX(), detector.getFocusY()));
+            mController.scrollBy(new PointF(mLastZoomFocus.x - detector.getFocusX(),
+                                            mLastZoomFocus.y - detector.getFocusY()));
+            PointF focus = new PointF(detector.getFocusX(), detector.getFocusY());
+            mController.scaleWithFocus(newZoomFactor, focus);
+        }
 
         mLastZoomFocus.set(detector.getFocusX(), detector.getFocusY());
 
         return true;
     }
 
     @Override
     public boolean onScaleBegin(ScaleGestureDetector detector) {