Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 2e39add5 authored by Sal Savage's avatar Sal Savage
Browse files

Always notify on node content, even if something goes wrong 

Problem: Some GetFolderItems requests can fail, timeout, or abort, and
we don't always notify the MediaBrowsers of the fact we're as done as
we're going to be with their request. This causes the UI in some clients
to indefinitely assume our service is downloading contents because we've
detached and never notified of contents.

Solution: GetFolderItems is _meant_ to represent exactly one fetch
cycle, so we can move the code more in line with that. We'll always
set the node to cached on exist of the state, and notify clients of the
final contents. In this way, clients will always get _something_,
whether its an empty list or the contents we have so far.

Tag: #stability
Bug: 279516561
Test: atest BluetoothInstrumentationTests

Please enter the commit message for your changes. Lines starting

Change-Id: I6793c61ca91c0ca04aea7b9fc168cf641dee05f3
parent c292061b

android/app/src/com/android/bluetooth/avrcpcontroller/AvrcpControllerStateMachine.java

+17 −4
Original line number Original line Diff line number Diff line
@@ -799,7 +799,7 @@ class AvrcpControllerStateMachine extends StateMachine { 
        private void processAvailablePlayerChanged() {

        private void processAvailablePlayerChanged() {

            logD("processAvailablePlayerChanged");

            logD("processAvailablePlayerChanged");

            mBrowseTree.mRootNode.setCached(false);

            mBrowseTree.mRootNode.setCached(false);

            mBrowseTree.mRootNode.setExpectedChildren(255);

            mBrowseTree.mRootNode.setExpectedChildren(BrowseTree.DEFAULT_FOLDER_SIZE);

            BluetoothMediaBrowserService.notifyChanged(mBrowseTree.mRootNode);

            BluetoothMediaBrowserService.notifyChanged(mBrowseTree.mRootNode);

            removeUnusedArtworkFromBrowseTree();

            removeUnusedArtworkFromBrowseTree();

            requestContents(mBrowseTree.mRootNode);

            requestContents(mBrowseTree.mRootNode);
@@ -831,7 +831,15 @@ class AvrcpControllerStateMachine extends StateMachine { 




            if (mBrowseNode == null) {

            if (mBrowseNode == null) {

                transitionTo(mConnected);

                transitionTo(mConnected);

            } else if (!mBrowsingConnected) {

                Log.w(TAG, "GetFolderItems: Browsing not connected, node=" + mBrowseNode);

                transitionTo(mConnected);

            } else {

            } else {

                int scope = mBrowseNode.getScope();

                if (scope == AvrcpControllerService.BROWSE_SCOPE_PLAYER_LIST

                        || scope == AvrcpControllerService.BROWSE_SCOPE_NOW_PLAYING) {

                    mBrowseNode.setExpectedChildren(BrowseTree.DEFAULT_FOLDER_SIZE);

                }

                mBrowseNode.setCached(false);

                mBrowseNode.setCached(false);

                navigateToFolderOrRetrieve(mBrowseNode);

                navigateToFolderOrRetrieve(mBrowseNode);

            }

            }
@@ -868,7 +876,6 @@ class AvrcpControllerStateMachine extends StateMachine { 
                        // If we have fetched all the elements or if the remotes sends us 0 elements

                        // If we have fetched all the elements or if the remotes sends us 0 elements

                        // (which can lead us into a loop since mCurrInd does not proceed) we simply

                        // (which can lead us into a loop since mCurrInd does not proceed) we simply

                        // abort.

                        // abort.

                        mBrowseNode.setCached(true);

                        transitionTo(mConnected);

                        transitionTo(mConnected);

                    } else {

                    } else {

                        // Fetch the next set of items.

                        // Fetch the next set of items.
@@ -964,7 +971,7 @@ class AvrcpControllerStateMachine extends StateMachine { 
                case MESSAGE_INTERNAL_CMD_TIMEOUT:

                case MESSAGE_INTERNAL_CMD_TIMEOUT:

                    // We have timed out to execute the request, we should simply send

                    // We have timed out to execute the request, we should simply send

                    // whatever listing we have gotten until now.

                    // whatever listing we have gotten until now.

                    Log.w(TAG, "TIMEOUT");

                    Log.w(TAG, "GetFolderItems: Timeout waiting for download, node=" + mBrowseNode);

                    transitionTo(mConnected);

                    transitionTo(mConnected);

                    break;

                    break;
@@ -972,7 +979,6 @@ class AvrcpControllerStateMachine extends StateMachine { 
                    // If we have gotten an error for OUT OF RANGE we have

                    // If we have gotten an error for OUT OF RANGE we have

                    // already sent all the items to the client hence simply

                    // already sent all the items to the client hence simply

                    // transition to Connected state here.

                    // transition to Connected state here.

                    mBrowseNode.setCached(true);

                    transitionTo(mConnected);

                    transitionTo(mConnected);

                    break;

                    break;
@@ -1097,6 +1103,13 @@ class AvrcpControllerStateMachine extends StateMachine { 
        public void exit() {

        public void exit() {

            logd("GetFolderItems: fetch complete, node=" + mBrowseNode);

            logd("GetFolderItems: fetch complete, node=" + mBrowseNode);

            removeMessages(MESSAGE_INTERNAL_CMD_TIMEOUT);

            removeMessages(MESSAGE_INTERNAL_CMD_TIMEOUT);



            // Whatever we have, notify on it so the UI doesn't hang

            if (mBrowseNode != null) {

                mBrowseNode.setCached(true);

                notifyChanged(mBrowseNode);

            }



            mBrowseNode = null;

            mBrowseNode = null;

            super.exit();

            super.exit();

        }

        }

android/app/src/com/android/bluetooth/avrcpcontroller/BrowseTree.java

+4 −2
Original line number Original line Diff line number Diff line
@@ -58,6 +58,8 @@ public class BrowseTree { 
    public static final String NOW_PLAYING_PREFIX = "NOW_PLAYING";

    public static final String NOW_PLAYING_PREFIX = "NOW_PLAYING";

    public static final String PLAYER_PREFIX = "PLAYER";

    public static final String PLAYER_PREFIX = "PLAYER";





    public static final int DEFAULT_FOLDER_SIZE = 255;



    // Static instance of Folder ID <-> Folder Instance (for navigation purposes)

    // Static instance of Folder ID <-> Folder Instance (for navigation purposes)

    @VisibleForTesting

    @VisibleForTesting

    final HashMap<String, BrowseNode> mBrowseMap = new HashMap<String, BrowseNode>();

    final HashMap<String, BrowseNode> mBrowseMap = new HashMap<String, BrowseNode>();
@@ -85,7 +87,7 @@ public class BrowseTree { 
        }

        }





        mRootNode.mBrowseScope = AvrcpControllerService.BROWSE_SCOPE_PLAYER_LIST;

        mRootNode.mBrowseScope = AvrcpControllerService.BROWSE_SCOPE_PLAYER_LIST;

        mRootNode.setExpectedChildren(255);

        mRootNode.setExpectedChildren(DEFAULT_FOLDER_SIZE);





        mNavigateUpNode = new BrowseNode(new AvrcpItem.Builder()

        mNavigateUpNode = new BrowseNode(new AvrcpItem.Builder()

                .setUuid(UP).setTitle(UP).setBrowsable(true).build());

                .setUuid(UP).setTitle(UP).setBrowsable(true).build());
@@ -94,7 +96,7 @@ public class BrowseTree { 
                .setUuid(NOW_PLAYING_PREFIX).setTitle(NOW_PLAYING_PREFIX)

                .setUuid(NOW_PLAYING_PREFIX).setTitle(NOW_PLAYING_PREFIX)

                .setBrowsable(true).build());

                .setBrowsable(true).build());

        mNowPlayingNode.mBrowseScope = AvrcpControllerService.BROWSE_SCOPE_NOW_PLAYING;

        mNowPlayingNode.mBrowseScope = AvrcpControllerService.BROWSE_SCOPE_NOW_PLAYING;

        mNowPlayingNode.setExpectedChildren(255);

        mNowPlayingNode.setExpectedChildren(DEFAULT_FOLDER_SIZE);

        mBrowseMap.put(ROOT, mRootNode);

        mBrowseMap.put(ROOT, mRootNode);

        mBrowseMap.put(NOW_PLAYING_PREFIX, mNowPlayingNode);

        mBrowseMap.put(NOW_PLAYING_PREFIX, mNowPlayingNode);

android/app/tests/unit/src/com/android/bluetooth/avrcpcontroller/AvrcpControllerStateMachineTest.java

+91 −0
Original line number Original line Diff line number Diff line
@@ -1817,4 +1817,95 @@ public class AvrcpControllerStateMachineTest { 
        Assert.assertTrue(nowPlaying.isCached());

        Assert.assertTrue(nowPlaying.isCached());

        assertNowPlayingList(updatedNowPlayingList);

        assertNowPlayingList(updatedNowPlayingList);

    }

    }



    /**

     * Test making a browse request where results don't come back within the timeout window. The

     * node should still be notified on.

     */

    @Test

    public void testMakeBrowseRequestWithTimeout_contentsCachedAndNotified() {

        setUpConnectedState(true, true);

        sendAudioFocusUpdate(AudioManager.AUDIOFOCUS_GAIN);



        //Set something arbitrary for the current Now Playing list

        List<AvrcpItem> nowPlayingList = new ArrayList<AvrcpItem>();

        nowPlayingList.add(makeNowPlayingItem(1, "Song 1"));

        setNowPlayingList(nowPlayingList);

        clearInvocations(mAvrcpControllerService);



        // Invalidate the contents by doing a new fetch

        BrowseTree.BrowseNode nowPlaying = mAvrcpStateMachine.findNode("NOW_PLAYING");

        mAvrcpStateMachine.requestContents(nowPlaying);

        TestUtils.waitForLooperToFinishScheduledTask(mAvrcpStateMachine.getHandler().getLooper());



        // Request for new contents should be sent

        verify(mAvrcpControllerService, times(1)).getNowPlayingListNative(

                eq(mTestAddress), eq(0), eq(19));

        Assert.assertFalse(nowPlaying.isCached());



        // Send timeout on our own instead of waiting 10 seconds

        mAvrcpStateMachine.sendMessage(AvrcpControllerStateMachine.MESSAGE_INTERNAL_CMD_TIMEOUT);

        TestUtils.waitForLooperToFinishScheduledTask(mAvrcpStateMachine.getHandler().getLooper());



        // Node should be set to cached and notified on

        assertNowPlayingList(new ArrayList<AvrcpItem>());

        Assert.assertTrue(nowPlaying.isCached());



        // See that state from BluetoothMediaBrowserService is updated to null (i.e. empty)

        MediaSessionCompat session = BluetoothMediaBrowserService.getSession();

        Assert.assertNotNull(session);

        MediaControllerCompat controller = session.getController();

        Assert.assertNotNull(controller);

        List<MediaSessionCompat.QueueItem> queue = controller.getQueue();

        Assert.assertNull(queue);

    }



    /**

     * Test making a browse request with a null node. The request should not generate any native

     * layer browse requests.

     */

    @Test

    public void testNullBrowseRequest_requestDropped() {

        setUpConnectedState(true, true);

        sendAudioFocusUpdate(AudioManager.AUDIOFOCUS_GAIN);

        clearInvocations(mAvrcpControllerService);

        mAvrcpStateMachine.requestContents(null);

        TestUtils.waitForLooperToFinishScheduledTask(mAvrcpStateMachine.getHandler().getLooper());

        verifyNoMoreInteractions(mAvrcpControllerService);

    }



    /**

     * Test making a browse request with browsing disconnected. The request should not generate any

     * native layer browse requests.

     */

    @Test

    public void testBrowseRequestWhileDisconnected_requestDropped() {

        final String rootName = "__ROOT__";

        setUpConnectedState(true, false);

        sendAudioFocusUpdate(AudioManager.AUDIOFOCUS_GAIN);

        clearInvocations(mAvrcpControllerService);

        BrowseTree.BrowseNode deviceRoot = mAvrcpStateMachine.findNode(rootName);

        mAvrcpStateMachine.requestContents(deviceRoot);

        TestUtils.waitForLooperToFinishScheduledTask(mAvrcpStateMachine.getHandler().getLooper());

        verifyNoMoreInteractions(mAvrcpControllerService);

    }



    /**

     * Queue a control channel connection event, a request while browse is disconnected, a browse

     * connection event, and then another browse request and be sure the final request still is sent

     */

    @Test

    public void testBrowseRequestWhileDisconnectedThenRequestWhileConnected_secondRequestSent() {

        final String rootName = "__ROOT__";

        setUpConnectedState(true, false);

        sendAudioFocusUpdate(AudioManager.AUDIOFOCUS_GAIN);

        clearInvocations(mAvrcpControllerService);

        BrowseTree.BrowseNode deviceRoot = mAvrcpStateMachine.findNode(rootName);

        mAvrcpStateMachine.requestContents(deviceRoot);

        // issues a player list fetch

        mAvrcpStateMachine.connect(StackEvent.connectionStateChanged(true, true));

        TestUtils.waitForLooperToBeIdle(mAvrcpStateMachine.getHandler().getLooper());

        verify(mAvrcpControllerService, times(1)).getPlayerListNative(

                eq(mTestAddress), eq(0), eq(19));

    }

}
 
}