3

On Android, I've subclassed SurfaceView and the resulting view is working fine in most cases. However, roughly 1% of all users report an ANR problem with this implementation.

Apparently, there's an edge case where the SurfaceView fails due to some problem, probably a deadlock.

Unfortunately, I don't know what's wrong with my implementation of onDraw(...) and onTouchEvent(...) or how to improve the code. Can you help?

"main" prio=5 tid=1 MONITOR
| group="main" sCount=1 dsCount=0 obj=0x41920e88 self=0x4190f8d0
| sysTid=13407 nice=0 sched=0/0 cgrp=apps handle=1074618708
| state=S schedstat=( 50780242971 27570770290 130442 ) utm=4254 stm=824 core=0
at com.my.package.util.HandCards.onTouchEvent(SourceFile:~188)
- waiting to lock <0x45b91988> (a android.view.SurfaceView$4) held by tid=18 (Thread-14297)
at android.view.View.dispatchTouchEvent(View.java:7837)
at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2216)
at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:1917)
at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2216)
at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:1917)
at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2216)
at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:1917)
at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2216)
at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:1917)
at com.android.internal.policy.impl.PhoneWindow$DecorView.superDispatchTouchEvent(PhoneWindow.java:2075)
at com.android.internal.policy.impl.PhoneWindow.superDispatchTouchEvent(PhoneWindow.java:1522)
at android.app.Activity.dispatchTouchEvent(Activity.java:2458)
at com.android.internal.policy.impl.PhoneWindow$DecorView.dispatchTouchEvent(PhoneWindow.java:2023)
at android.view.View.dispatchPointerEvent(View.java:8017)
at android.view.ViewRootImpl$ViewPostImeInputStage.processPointerEvent(ViewRootImpl.java:3966)
at android.view.ViewRootImpl$ViewPostImeInputStage.onProcess(ViewRootImpl.java:3845)
at android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:3405)
at android.view.ViewRootImpl$InputStage.onDeliverToNext(ViewRootImpl.java:3455)
at android.view.ViewRootImpl$InputStage.forward(ViewRootImpl.java:3424)
at android.view.ViewRootImpl$AsyncInputStage.forward(ViewRootImpl.java:3531)
at android.view.ViewRootImpl$InputStage.apply(ViewRootImpl.java:3432)
at android.view.ViewRootImpl$AsyncInputStage.apply(ViewRootImpl.java:3588)
at android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:3405)
at android.view.ViewRootImpl$InputStage.onDeliverToNext(ViewRootImpl.java:3455)
at android.view.ViewRootImpl$InputStage.forward(ViewRootImpl.java:3424)
at android.view.ViewRootImpl$InputStage.apply(ViewRootImpl.java:3432)
at android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:3405)
at android.view.ViewRootImpl.deliverInputEvent(ViewRootImpl.java:5554)
at android.view.ViewRootImpl.doProcessInputEvents(ViewRootImpl.java:5534)
at android.view.ViewRootImpl.enqueueInputEvent(ViewRootImpl.java:5505)
at android.view.ViewRootImpl$WindowInputEventReceiver.onInputEvent(ViewRootImpl.java:5634)
at android.view.InputEventReceiver.dispatchInputEvent(InputEventReceiver.java:185)
at android.os.MessageQueue.nativePollOnce(Native Method)
at android.os.MessageQueue.next(MessageQueue.java:138)
at android.os.Looper.loop(Looper.java:196)
at android.app.ActivityThread.main(ActivityThread.java:5135)
at java.lang.reflect.Method.invokeNative(Native Method)
at java.lang.reflect.Method.invoke(Method.java:515)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:878)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:694)
at dalvik.system.NativeStart.main(Native Method)

...

"Thread-14297" prio=5 tid=18 SUSPENDED
| group="main" sCount=1 dsCount=0 obj=0x45ba6358 self=0x76036b38
| sysTid=21120 nice=0 sched=0/0 cgrp=apps handle=1979936656
| state=S schedstat=( 48296386737 3088012659 22649 ) utm=4691 stm=138 core=0
#00 pc 00021adc /system/lib/libc.so (__futex_syscall3+8)
#01 pc 0000f074 /system/lib/libc.so (__pthread_cond_timedwait_relative+48)
#02 pc 0000f0d4 /system/lib/libc.so (__pthread_cond_timedwait+64)
#03 pc 0005655f /system/lib/libdvm.so
#04 pc 00056b21 /system/lib/libdvm.so (dvmChangeStatus(Thread*, ThreadStatus)+34)
#05 pc 00050fd7 /system/lib/libdvm.so (dvmCallJNIMethod(unsigned int const*, JValue*, Method const*, Thread*)+406)
#06 pc 00000214 /dev/ashmem/dalvik-jit-code-cache (deleted)
at android.graphics.Canvas.native_drawBitmap(Native Method)
at android.graphics.Canvas.drawBitmap(Canvas.java:1202)
at com.my.package.util.HandCards.a(SourceFile:178)
at com.my.package.util.HandCards.onDraw(SourceFile:136)
at com.my.package.util.d.run(SourceFile:36)

Where HandCards.onTouchEvent(SourceFile:~188) is:

synchronized (mRenderThread.getSurfaceHolder()) {

And HandCards.a(SourceFile:178) is:

canvas.drawBitmap(drawCardBitmap, null, mDrawingRect, mGraphicsPaint);

The full code for the SurfaceView subclass is:

public class HandCards extends SurfaceView implements SurfaceHolder.Callback {

    /** Opacity of the shadow layer that hides other cards when one card is highlighted and covers all cards when it's another player's turn (where 0 is transparent and 255 is opaque) */
    private static final int SHADOW_ALPHA = 150;
    private static SparseArray<Bitmap> mCardCache = new SparseArray<Bitmap>(); // cache array for own card bitmaps
    private HandThread mRenderThread;
    private volatile List<Card> mCards;
    private volatile int mCardCount;
    private volatile int mScreenWidth;
    private volatile int mScreenHeight;
    private volatile int mCardWidth;
    private volatile int mCardHeight;
    private volatile int mHighlightedCard = -1;
    private CardClickCallback mCardClickCallback;
    private volatile int mBlattID = 1;
    private volatile int mCurrentCardSpacing;
    private final Paint mGraphicsPaint;
    private final Paint mShadowPaint;
    private final Rect mDrawingRect;
    private volatile int mTouchEventAction;
    private volatile int mTouchEventCard;
    private Bitmap drawCardBitmap;
    private volatile int mOnDrawX1;
    private final BitmapFactory.Options mBitmapOptions;
    private volatile boolean mIsActive = true;
    private final int[] mCardSelection = new int[GameState.MAX_SWAP_CARDS];
    /** Indicates that the card view is currently used for choosing some cards to create a selection */
    private volatile boolean mIsChooseMode;
    /** Holds the index of the selected card that will be replaced next if all selection slots are full */
    private volatile int mNextReplacePosition;
    /** Used only locally in drawCard() but is declared here to save repeated allocations */
    private volatile int mCardOffsetY;
    private volatile int mRequiredSelectionCount;

    public HandCards(Context activityContext, AttributeSet attributeSet) {
        super(activityContext, attributeSet);
        getHolder().addCallback(this);
        setFocusable(true); // touch events should be processed by this class
        mCards = new ArrayList<Card>();
        mGraphicsPaint = new Paint();
        mGraphicsPaint.setAntiAlias(true);
        mGraphicsPaint.setFilterBitmap(true);
        mShadowPaint = new Paint();
        mShadowPaint.setARGB(SHADOW_ALPHA, 20, 20, 20);
        mShadowPaint.setAntiAlias(true);
        mBitmapOptions = new BitmapFactory.Options();
        mBitmapOptions.inInputShareable = true;
        mBitmapOptions.inPurgeable = true;
        mDrawingRect = new Rect();
    }

    public Card getCard(int location) throws Exception {
        if (mCards != null) {
            synchronized (mCards) {
                return mCards.get(location); // card may not be found (throw exception then)
            }
        }
        return null;
    }

    public static Bitmap cardCacheGet(int key) {
        synchronized (mCardCache) {
            return mCardCache.get(key);
        }
    }

    public static void cardCachePut(int key, Bitmap object) {
        synchronized (mCardCache) {
            mCardCache.put(key, object);
        }
    }

    public int[] getSelectedCards() {
        return mCardSelection;
    }

    public void setActive(boolean active) {
        if (mCardSelection != null) {
            for (int i = 0; i < GameState.MAX_SWAP_CARDS; i++) { // loop through all slots for selected cards
                mCardSelection[i] = -1; // unset the slot so that it is empty by default
            }
        }
        mIsActive = active;
    }

    public boolean isActive() {
        return mIsActive;
    }

    public void setChooseMode(boolean active, int swapCardCount) {
        mNextReplacePosition = 0;
        mIsChooseMode = active;
        mRequiredSelectionCount = swapCardCount;
    }

    public boolean isChooseMode() {
        return mIsChooseMode;
    }

    public void stopThread() {
        if (mRenderThread != null) {
            mRenderThread.setRunning(false);
        }
    }

    @Override
    public void onDraw(Canvas canvas) {
        if (canvas != null) {
            synchronized (mCards) {
                mCardCount = mCards.size();
                canvas.drawColor(Color.BLACK);
                if (mCardCount > 0) {
                    mCurrentCardSpacing = Math.min(mScreenWidth/mCardCount, mCardWidth);
                    for (int c = 0; c < mCardCount; c++) {
                        if (c != mHighlightedCard || !isActive()) {
                            try {
                                drawCard(canvas, mCards.get(c).getDrawableID(mBlattID), false, c*mCurrentCardSpacing, c*mCurrentCardSpacing+mCardWidth, c);
                            }
                            catch (Exception e) { }
                        }
                    }
                    if (mHighlightedCard > -1 && isActive()) {
                        mOnDrawX1 = Math.min(mHighlightedCard*mCurrentCardSpacing, mScreenWidth-mCardWidth);
                        try {
                            drawCard(canvas, mCards.get(mHighlightedCard).getDrawableID(mBlattID), true, mOnDrawX1, mOnDrawX1+mCardWidth, mHighlightedCard);
                        }
                        catch (Exception e) { }
                    }
                    else if (!isActive()) {
                        drawCard(canvas, 0, true, 0, mScreenWidth, 0);
                    }
                }
            }
        }
    }

    private void drawCard(Canvas canvas, int resourceID, boolean highlighted, int xLeft, int xRight, int cardPosition) {
        if (canvas != null) {
            try {
                if (highlighted) {
                    canvas.drawRect(0, 0, mScreenWidth, mScreenHeight, mShadowPaint);
                }
                if (resourceID != 0) {
                    drawCardBitmap = cardCacheGet(resourceID);
                    if (drawCardBitmap == null) {
                        drawCardBitmap = BitmapFactory.decodeResource(getResources(), resourceID, mBitmapOptions);
                        cardCachePut(resourceID, drawCardBitmap);
                    }
                    mCardOffsetY = 0; // by default draw all cards right at the bottom (without highlighting by position)
                    if (mCardSelection != null) {
                        for (int i = 0; i < GameState.MAX_SWAP_CARDS; i++) { // loop through all slots for selected cards
                            if (mCardSelection[i] == cardPosition) { // if current card has been selected (in that slot)
                                mCardOffsetY = mScreenHeight*1/4; // lift the card by one quarter to highlight it
                                break; // card has already been detected to be selected so stop here
                            }
                        }
                    }
                    mDrawingRect.set(xLeft, mCardOffsetY, xRight, mCardHeight+mCardOffsetY);
                    canvas.drawBitmap(drawCardBitmap, null, mDrawingRect, mGraphicsPaint);
                }
            }
            catch (Exception e) { }
        }
    }

    @Override
    public boolean onTouchEvent(MotionEvent event) {
        if (mRenderThread == null) { return false; }
        synchronized (mRenderThread.getSurfaceHolder()) { // synchronized so that there are no concurrent accesses
            mTouchEventAction = event.getAction();
            if (isActive()) {
                if (mTouchEventAction == MotionEvent.ACTION_DOWN || mTouchEventAction == MotionEvent.ACTION_MOVE) {
                    if (event.getY() >= 0 && event.getY() < mScreenHeight) {
                        mTouchEventCard = (int) event.getX()/mCurrentCardSpacing;
                        if (mTouchEventCard > -1 && mTouchEventCard < mCardCount) {
                            mHighlightedCard = mTouchEventCard;
                        }
                        else {
                            mHighlightedCard = -1;
                        }
                    }
                    else {
                        mHighlightedCard = -1;
                    }
                }
                else if (mTouchEventAction == MotionEvent.ACTION_UP) {
                    if (mCardClickCallback != null && mHighlightedCard > -1 && mHighlightedCard < mCardCount) {
                        if (isChooseMode()) { // card has been chosen as a swap card
                            int freeSelectionIndex = -1; // remember the index of a free selection slot (default = none available)
                            for (int i = 0; i < mRequiredSelectionCount; i++) { // loop through all allowed slots for selected cards
                                if (mCardSelection[i] == mHighlightedCard) { // if this card has already been selected
                                    mCardSelection[i] = -1; // unselect the card
                                    freeSelectionIndex = -2; // mark that there is no need to select a new card
                                    break; // slot of current card has already been found so stop here
                                }
                                else if (mCardSelection[i] == -1 && freeSelectionIndex == -1) { // if slot is still available and no free slot has been found yet
                                    freeSelectionIndex = i; // remember the index of this free slot
                                }
                            }
                            if (freeSelectionIndex > -2) { // if a new card is to be placed in the selection array
                                if (freeSelectionIndex >= 0) { // if a free slot was available
                                    mCardSelection[freeSelectionIndex] = mHighlightedCard; // just place the card there
                                }
                                else { // if no free slot was available anymore
                                    mCardSelection[mNextReplacePosition] = mHighlightedCard; // replace another card in one of the slots
                                    mNextReplacePosition = (mNextReplacePosition+1) % mRequiredSelectionCount; // advance the cursor that points to the slot which will be replaced next
                                }
                            }
                        }
                        else { // card has been selected to be played on the table
                            try {
                                mCardClickCallback.chooseCard(mCards.get(mHighlightedCard));
                            }
                            catch (Exception e) {
                                // index was out of mCards' bounds (just ignore this, user may tap on card again)
                            }
                        }
                    }
                    mHighlightedCard = -1;
                }
            }
            else {
                try {
                    mCardClickCallback.resyncManually();
                }
                catch (Exception e) { }
            }
        }
        return true;
    }

    @Override
    public void surfaceChanged(SurfaceHolder arg0, int arg1, int arg2, int arg3) { }

    public void setCards(List<Card> currentCards) {
        synchronized (mCards) {
            mCards.clear();
            mCards.addAll(currentCards);
        }
    }

    @Override
    public void surfaceCreated(SurfaceHolder arg0) {
        mScreenWidth = getWidth();
        mScreenHeight = getHeight();
        mCardHeight = mScreenHeight;
        mCardWidth = mCardHeight*99/150;
        mCurrentCardSpacing = mCardWidth;
        mRenderThread = new HandThread(getHolder(), this);
        mRenderThread.setRunning(true);
        mRenderThread.start();
    }

    @Override
    public void surfaceDestroyed(SurfaceHolder holder) {
        boolean retry = true;
        mRenderThread.setRunning(false); // stop thread
        while (retry) { // wait for thread to close
            try {
                mRenderThread.join();
                retry = false;
            }
            catch (InterruptedException e) { }
        }
    }

    public synchronized void setCardClickCallback(CardClickCallback callback) {
        mCardClickCallback = callback;
    }

    public void setBlattID(int blattID) {
        mBlattID = blattID;
    }

}

Then, there's also the render thread:

public class HandThread extends Thread {

    private final SurfaceHolder mSurfaceHolder;
    private final HandCards mSurface;
    private volatile boolean mRunning = false;

    public HandThread(SurfaceHolder surfaceHolder, HandCards surface) {
        mSurfaceHolder = surfaceHolder;
        mSurface = surface;
    }

    public SurfaceHolder getSurfaceHolder() {
        return mSurfaceHolder;
    }

    public void setRunning(boolean run) {
        mRunning = run;
    }

    @Override
    public void run() {
        Canvas c;
        while (mRunning) {
            c = null;
            try {
                c = mSurfaceHolder.lockCanvas(null);
                synchronized (mSurfaceHolder) {
                    if (c != null) {
                        mSurface.onDraw(c);
                    }
                }
            }
            finally { // when exception is thrown above we may not leave the surface in an inconsistent state
                if (c != null) {
                    try {
                        mSurfaceHolder.unlockCanvasAndPost(c);
                    }
                    catch (Exception e) { }
                }
            }
        }
    }

}
caw
  • 30,999
  • 61
  • 181
  • 291
  • This is a shot in the dark, the SDK reference mentions using worker threads instead of using the system thread to prevent locking up the system UI thread causing an ANR. – user4317867 Mar 03 '15 at 03:28

1 Answers1

6

The ANR is happening because your onTouchEvent() method is synchronizing on a lock held by tid=18, an unnamed thread known only as Thread-14297.

Many people follow a pattern where, at the point they lock the SurfaceView canvas, they also lock the SurfaceHolder object. It's a bad idea to synchronize on objects with public visibility, and an even worse idea to synchronize on objects shared with the GUI framework, so it's sad that this pattern persists. (But I digress.)

You're drawing in an overridden onDraw() method, which doesn't make sense if you're drawing from a renderer thread -- the onDraw() method is used by the View hierarchy and would be called from the UI thread, but here it's clearly being called from elsewhere. You should call it something else, maybe just myDraw(). (But I digress.)

Thread-14297 is in the "suspended" state, which means it was executing but stopped when the stack trace was captured. Since the topmost frame is a native method -- which don't get suspended by the VM -- it was probably entering or exiting the frame. The system and user time for the thread, shown in ticks as "utm=" and "stm=" values, are fairly low, suggesting that it's not doing excessive CPU work. Unless, of course, your render thread is a one-shot, in which case it was fairly busy (and may not be done yet).

The good news is that you don't appear to be deadlocked. The render thread is just running slowly. Or, perhaps, you have a loop that is failing to exit (though none is apparent from the posted code). On a slow device, with lots of other activity on the system, and a big mCards list, it could get starved for CPU and fail to respond quickly. Assuming you're following the common pattern and locking the SurfaceHolder when you grab the Canvas, your onTouchEvent() is going to lock up the UI thread for the full duration of the draw. The ANR summary in logcat usually lists recent thread activity levels; if you have access to that, the information could tell you how busy the render thread has been.

Not all ANRs are fatal. If the app becomes permanently unresponsive, that's very different from a temporary ANR that clears up when the user hits "wait". Do you know which kind this is?

You need to:

  1. Re-evaluate your data synchronization. Use shorter windows and maybe a read-write lock to pass data around. Poke around in java.util.concurrent. Stalling the UI thread for an extended period is bad.
  2. Determine why your rendering seems to be taking a long time, and whether it's just running slowly or spinning forever.
fadden
  • 51,356
  • 5
  • 116
  • 166
  • Thanks a lot! I've added the code of my render thread to the question. As you can see, I'm indeed calling `onDraw()` there. As of your second "But I digress", should I stop calling `onDraw()` from the thread and just hope that the system calls `onDraw()` automatically? Or should I rename both the definition and the call to just `draw()` without having any overriden `onDraw()` method then? I thought that, if I'm using a render thread, it's not important whether the drawing takes a long time or not. That's what the thread is for, isn't it? – caw Mar 03 '15 at 14:56
  • And as of your second "But I digress", what changes should I apply? Which locks should I remove and what objects should I synchronize on, instead? – caw Mar 03 '15 at 14:57
  • For #1, `onDraw()` is used for custom Views (http://developer.android.com/training/custom-views/index.html). Since you're drawing on the Surface, not the View, you don't want to override `onDraw()`. Just rename it. #2 is more difficult to answer, as it's impossible to say what all you need without knowing exactly what your program does. However, it's not necessary to lock the SurfaceHolder -- the `lockCanvas()` method prevents SurfaceView from yanking the Surface out from under you. Create an Object that is only visible to your threads, and hold the lock for as brief a time as possible. – fadden Mar 03 '15 at 16:44
  • Thanks! Now that I've renamed it to `draw(Canvas canvas)`, I've seen that this is also an existing method that I can override. Is this what you want when drawing on `SurfaceView` from a separate thread? http://developer.android.com/reference/android/view/SurfaceView.html#draw%28android.graphics.Canvas%29 – caw Mar 03 '15 at 17:27
  • Should I remove `synchronized (mSurfaceHolder) { }` from the `run()` method inside of the `Thread`? Or should I remove `synchronized (mRenderThread.getSurfaceHolder()) { }` from the `onTouchEvent(MotionEvent event)` method in the `SurfaceView` itself? Or both? Sorry for my lack of understanding. – caw Mar 03 '15 at 17:30
  • Whoops, `draw()` was a mistake (answer updated). For the synchronization, don't lock out the UI thread for the entire draw sequence. One approach is to use message passing instead of sharing state. For example, `onTouchEvent()` could simply forward the event to the render thread through a Handler, which would become responsible for all of the state changes associated with user events. Since one thread would have exclusive access to the state, no `synchronized` statements are required. Grafika (https://github.com/google/grafika) uses this a lot; see e.g. "hardware scaler exerciser" UI. – fadden Mar 03 '15 at 18:09
  • Thank you! So from what I understood so far, the following changes (with regard to the original code in the question) have to be made: (1) Replace the definition and call of `onDraw(Canvas canvas)` with something like `doDraw(Canvas canvas)` (2) In `onTouchEvent(...)`, synchronize on something like `private final Object mTouchLock = new Object()` instead of `mRenderThread.getSurfaceHolder()` (3) Use `lockCanvas()` instead of `lockCanvas(null)` in the render thread (4) Remove `synchronized (mSurfaceHolder) { }` from the render thread -- Can you (or someone else) say whether this is correct? – caw Mar 03 '15 at 19:57