From 9fa99145a9e91cdb693f7d0ef18e53bf8b57702e Mon Sep 17 00:00:00 2001 From: Jayant Chowdhary Date: Wed, 16 Sep 2020 10:26:53 -0700 Subject: [PATCH 1/2] camera2: Improve code readability for FrameNumberTracker. Bug: 168713666 Test: builds Test: GCA (basic validity) Change-Id: Id0f9b5a9a6363f40daa9384ee5f01b47d36ad139 Signed-off-by: Jayant Chowdhary (cherry picked from commit 91f7bc0b3b1605c46b64ecf41ede357192df39df) --- .../camera2/impl/FrameNumberTracker.java | 85 +++++++++++-------- 1 file changed, 50 insertions(+), 35 deletions(-) diff --git a/core/java/android/hardware/camera2/impl/FrameNumberTracker.java b/core/java/android/hardware/camera2/impl/FrameNumberTracker.java index 27f8a61b8999d..97c79f6e13740 100644 --- a/core/java/android/hardware/camera2/impl/FrameNumberTracker.java +++ b/core/java/android/hardware/camera2/impl/FrameNumberTracker.java @@ -37,12 +37,16 @@ public class FrameNumberTracker { /** the completed frame number for each type of capture results */ private long[] mCompletedFrameNumber = new long[CaptureRequest.REQUEST_TYPE_COUNT]; - /** the skipped frame numbers that don't belong to each type of capture results */ - private final LinkedList[] mSkippedOtherFrameNumbers = + /** the frame numbers that don't belong to each type of capture results and are yet to be seen + * through an updateTracker() call. Each list holds a list of frame numbers that should appear + * with request types other than that, to which the list corresponds. + */ + private final LinkedList[] mPendingFrameNumbersWithOtherType = new LinkedList[CaptureRequest.REQUEST_TYPE_COUNT]; - /** the skipped frame numbers that belong to each type of capture results */ - private final LinkedList[] mSkippedFrameNumbers = + /** the frame numbers that belong to each type of capture results which should appear, but + * haven't yet.*/ + private final LinkedList[] mPendingFrameNumbers = new LinkedList[CaptureRequest.REQUEST_TYPE_COUNT]; /** frame number -> request type */ @@ -53,8 +57,8 @@ public class FrameNumberTracker { public FrameNumberTracker() { for (int i = 0; i < CaptureRequest.REQUEST_TYPE_COUNT; i++) { mCompletedFrameNumber[i] = CameraCaptureSession.CaptureCallback.NO_FRAMES_CAPTURED; - mSkippedOtherFrameNumbers[i] = new LinkedList(); - mSkippedFrameNumbers[i] = new LinkedList(); + mPendingFrameNumbersWithOtherType[i] = new LinkedList(); + mPendingFrameNumbers[i] = new LinkedList(); } } @@ -69,19 +73,20 @@ public class FrameNumberTracker { mCompletedFrameNumber[requestType] = errorFrameNumber; removeError = true; } else { - if (!mSkippedFrameNumbers[requestType].isEmpty()) { - if (errorFrameNumber == mSkippedFrameNumbers[requestType].element()) { + if (!mPendingFrameNumbers[requestType].isEmpty()) { + if (errorFrameNumber == mPendingFrameNumbers[requestType].element()) { mCompletedFrameNumber[requestType] = errorFrameNumber; - mSkippedFrameNumbers[requestType].remove(); + mPendingFrameNumbers[requestType].remove(); removeError = true; } } else { for (int i = 1; i < CaptureRequest.REQUEST_TYPE_COUNT; i++) { int otherType = (requestType + i) % CaptureRequest.REQUEST_TYPE_COUNT; - if (!mSkippedOtherFrameNumbers[otherType].isEmpty() && errorFrameNumber - == mSkippedOtherFrameNumbers[otherType].element()) { + if (!mPendingFrameNumbersWithOtherType[otherType].isEmpty() + && errorFrameNumber + == mPendingFrameNumbersWithOtherType[otherType].element()) { mCompletedFrameNumber[requestType] = errorFrameNumber; - mSkippedOtherFrameNumbers[otherType].remove(); + mPendingFrameNumbersWithOtherType[otherType].remove(); removeError = true; break; } @@ -182,7 +187,7 @@ public class FrameNumberTracker { * It validates that all previous frames of the same category have arrived. * * If there is a gap since previous frame number of the same category, assume the frames in - * the gap are other categories and store them in the skipped frame number queue to check + * the gap are other categories and store them in the pending frame number queue to check * against when frames of those categories arrive. */ private void updateCompletedFrameNumber(long frameNumber, @@ -199,25 +204,29 @@ public class FrameNumberTracker { if (frameNumber < maxOtherFrameNumberSeen) { // if frame number is smaller than completed frame numbers of other categories, // it must be: - // - the head of mSkippedFrameNumbers for this category, or - // - in one of other mSkippedOtherFrameNumbers - if (!mSkippedFrameNumbers[requestType].isEmpty()) { - // frame number must be head of current type of mSkippedFrameNumbers if - // mSkippedFrameNumbers isn't empty. - if (frameNumber < mSkippedFrameNumbers[requestType].element()) { + // - the head of mPendingFrameNumbers for this category, or + // - in one of other mPendingFrameNumbersWithOtherType + if (!mPendingFrameNumbers[requestType].isEmpty()) { + // frame number must be head of current type of mPendingFrameNumbers if + // mPendingFrameNumbers isn't empty. + Long pendingFrameNumberSameType = mPendingFrameNumbers[requestType].element(); + if (frameNumber == pendingFrameNumberSameType) { + // frame number matches the head of the pending frame number queue. + // Do this before the inequality checks since this is likely to be the common + // case. + mPendingFrameNumbers[requestType].remove(); + } else if (frameNumber < pendingFrameNumberSameType) { throw new IllegalArgumentException("frame number " + frameNumber + " is a repeat"); - } else if (frameNumber > mSkippedFrameNumbers[requestType].element()) { + } else { throw new IllegalArgumentException("frame number " + frameNumber + " comes out of order. Expecting " - + mSkippedFrameNumbers[requestType].element()); + + pendingFrameNumberSameType); } - // frame number matches the head of the skipped frame number queue. - mSkippedFrameNumbers[requestType].remove(); } else { - // frame number must be in one of the other mSkippedOtherFrameNumbers. - int index1 = mSkippedOtherFrameNumbers[otherType1].indexOf(frameNumber); - int index2 = mSkippedOtherFrameNumbers[otherType2].indexOf(frameNumber); + // frame number must be in one of the other mPendingFrameNumbersWithOtherType. + int index1 = mPendingFrameNumbersWithOtherType[otherType1].indexOf(frameNumber); + int index2 = mPendingFrameNumbersWithOtherType[otherType2].indexOf(frameNumber); boolean inSkippedOther1 = index1 != -1; boolean inSkippedOther2 = index2 != -1; if (!(inSkippedOther1 ^ inSkippedOther2)) { @@ -225,33 +234,39 @@ public class FrameNumberTracker { + " is a repeat or invalid"); } - // We know the category of frame numbers in skippedOtherFrameNumbers leading up - // to the current frame number. Move them into the correct skippedFrameNumbers. + // We know the category of frame numbers in pendingFrameNumbersWithOtherType leading + // up to the current frame number. The destination is the type which isn't the + // requestType* and isn't the src. Move them into the correct pendingFrameNumbers. + // * : This is since frameNumber is the first frame of requestType that we've + // received in the 'others' list, since for each request type frames come in order. + // All the frames before frameNumber are of the same type. They're not of + // 'requestType', neither of the type of the 'others' list they were found in. The + // remaining option is the 3rd type. LinkedList srcList, dstList; int index; if (inSkippedOther1) { - srcList = mSkippedOtherFrameNumbers[otherType1]; - dstList = mSkippedFrameNumbers[otherType2]; + srcList = mPendingFrameNumbersWithOtherType[otherType1]; + dstList = mPendingFrameNumbers[otherType2]; index = index1; } else { - srcList = mSkippedOtherFrameNumbers[otherType2]; - dstList = mSkippedFrameNumbers[otherType1]; + srcList = mPendingFrameNumbersWithOtherType[otherType2]; + dstList = mPendingFrameNumbers[otherType1]; index = index2; } for (int i = 0; i < index; i++) { dstList.add(srcList.removeFirst()); } - // Remove current frame number from skippedOtherFrameNumbers + // Remove current frame number from pendingFrameNumbersWithOtherType srcList.remove(); } } else { // there is a gap of unseen frame numbers which should belong to the other - // 2 categories. Put all the skipped frame numbers in the queue. + // 2 categories. Put all the pending frame numbers in the queue. for (long i = Math.max(maxOtherFrameNumberSeen, mCompletedFrameNumber[requestType]) + 1; i < frameNumber; i++) { - mSkippedOtherFrameNumbers[requestType].add(i); + mPendingFrameNumbersWithOtherType[requestType].add(i); } } From 47a455af03a0ac8da182a37e3a98928dc273dcbb Mon Sep 17 00:00:00 2001 From: Jayant Chowdhary Date: Fri, 18 Sep 2020 01:28:25 -0700 Subject: [PATCH 2/2] camera2: Remove partial result nodes for error frames. When there's an capture error reported for a frame, remove its nodes in mPartialResults map node, since they'll not be used by clients of FrameTracker. This should also reduce slow memory pressure build up because of mPartialResult growth as and when there are error frames which have some partial results as well. Bug: 167944895 Test: GCA on Pixel2, constant mode changes don't show steady state increase in mPartialResults map size (Basic validity) Change-Id: I6de585deb24039321310ddbd5dccd9119b25b23d Signed-off-by: Jayant Chowdhary (cherry picked from commit 91e64e2d3a71e2dad5d2f8efca0721dbf4f9b235) --- .../camera2/impl/FrameNumberTracker.java | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/core/java/android/hardware/camera2/impl/FrameNumberTracker.java b/core/java/android/hardware/camera2/impl/FrameNumberTracker.java index 97c79f6e13740..7b6a457411f35 100644 --- a/core/java/android/hardware/camera2/impl/FrameNumberTracker.java +++ b/core/java/android/hardware/camera2/impl/FrameNumberTracker.java @@ -70,30 +70,29 @@ public class FrameNumberTracker { int requestType = (int) pair.getValue(); Boolean removeError = false; if (errorFrameNumber == mCompletedFrameNumber[requestType] + 1) { - mCompletedFrameNumber[requestType] = errorFrameNumber; removeError = true; + } + // The error frame number could have also either been in the pending list or one of the + // 'other' pending lists. + if (!mPendingFrameNumbers[requestType].isEmpty()) { + if (errorFrameNumber == mPendingFrameNumbers[requestType].element()) { + mPendingFrameNumbers[requestType].remove(); + removeError = true; + } } else { - if (!mPendingFrameNumbers[requestType].isEmpty()) { - if (errorFrameNumber == mPendingFrameNumbers[requestType].element()) { - mCompletedFrameNumber[requestType] = errorFrameNumber; - mPendingFrameNumbers[requestType].remove(); + for (int i = 1; i < CaptureRequest.REQUEST_TYPE_COUNT; i++) { + int otherType = (requestType + i) % CaptureRequest.REQUEST_TYPE_COUNT; + if (!mPendingFrameNumbersWithOtherType[otherType].isEmpty() && errorFrameNumber + == mPendingFrameNumbersWithOtherType[otherType].element()) { + mPendingFrameNumbersWithOtherType[otherType].remove(); removeError = true; - } - } else { - for (int i = 1; i < CaptureRequest.REQUEST_TYPE_COUNT; i++) { - int otherType = (requestType + i) % CaptureRequest.REQUEST_TYPE_COUNT; - if (!mPendingFrameNumbersWithOtherType[otherType].isEmpty() - && errorFrameNumber - == mPendingFrameNumbersWithOtherType[otherType].element()) { - mCompletedFrameNumber[requestType] = errorFrameNumber; - mPendingFrameNumbersWithOtherType[otherType].remove(); - removeError = true; - break; - } + break; } } } if (removeError) { + mCompletedFrameNumber[requestType] = errorFrameNumber; + mPartialResults.remove(errorFrameNumber); iter.remove(); } }