Skip to content
This repository was archived by the owner on Aug 8, 2022. It is now read-only.

Commit 8b9dfd6

Browse files
DanielJetteDanielJette
authored
Merge pull request #239 from Shopify/228-irregular-bounds
Fix Issue #228: Fix IndexOutOfBoundsException when processing uneven image buffer chunks
2 parents 41922e1 + 6cc646a commit 8b9dfd6

7 files changed

Lines changed: 132 additions & 21 deletions

File tree

‎.github/workflows/library_build.yml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,3 +67,10 @@ jobs:
6767
# Run test Gradle task
6868
- name: Run JVM Tests
6969
run: ./gradlew :Library:test
70+
71+
- name: Archive test results
72+
if: ${{ always() }}
73+
uses: actions/upload-artifact@v2
74+
with:
75+
name: junit-test-results
76+
path: ./Library/build/reports/tests/**/**

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@
66

77
#### Bug fixes
88

9+
- https://github.com/Shopify/android-testify/issues/228, https://github.com/Shopify/android-testify/issues/215
10+
Account for uneven processing chunk sizes. As Testify processes, it divides the images into chunks for faster, parallel processing.
11+
A bug in the original code assumed that each processing chunk would be equally sized. This caused an out-of-bounds exception in any case where the number of pixels in the image could not be evenly divided.
12+
913
- https://github.com/Shopify/android-testify/issues/216
1014
You can now use `ScreenshotRule.setExactness()` in conjunction with `ScreenshotRule.defineExclusionRects()`. You can now define both an exclusion area and an exactness threshold.
1115

‎Library/build.gradle‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ android {
6666
implementation "androidx.test:runner:${versions.androidx.test.runner}"
6767
implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8:${versions.kotlin}"
6868
implementation "com.github.ajalt:colormath:${versions.colormath}"
69-
implementation("org.jetbrains.kotlinx:kotlinx-coroutines-android:1.4.3") {
69+
implementation("org.jetbrains.kotlinx:kotlinx-coroutines-android:${versions.kotlinx}") {
7070
exclude group: 'org.jetbrains.kotlin', module: 'kotlin-stdlib-jdk8'
7171
exclude group: 'org.jetbrains.kotlin', module: 'kotlin-stdlib-jdk7'
7272
exclude group: 'org.jetbrains.kotlin', module: 'kotlin-stdlib'
@@ -75,6 +75,7 @@ android {
7575

7676
testImplementation "org.mockito:mockito-core:${versions.mockito2}"
7777
testImplementation "com.nhaarman.mockitokotlin2:mockito-kotlin:${versions.mockitokotlin}"
78+
testImplementation "org.jetbrains.kotlinx:kotlinx-coroutines-test:${versions.kotlinx}"
7879

7980
androidTestImplementation "androidx.test.ext:junit:${versions.androidx.test.junit}"
8081
androidTestImplementation "androidx.test:runner:${versions.androidx.test.runner}"

‎Library/src/main/java/com/shopify/testify/internal/processor/BitmapExtentions.kt‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package com.shopify.testify.internal.processor
22

33
import android.graphics.Bitmap
4+
import androidx.annotation.VisibleForTesting
5+
import kotlinx.coroutines.CoroutineDispatcher
46
import kotlinx.coroutines.asCoroutineDispatcher
57
import java.util.concurrent.Executors
68

@@ -13,7 +15,17 @@ fun ParallelPixelProcessor.TransformResult.createBitmap(): Bitmap {
1315
)
1416
}
1517

16-
var numberOfCores = Runtime.getRuntime().availableProcessors()
18+
private val numberOfAvailableCores = Runtime.getRuntime().availableProcessors()
19+
20+
@VisibleForTesting
21+
var maxNumberOfChunkThreads = numberOfAvailableCores
22+
23+
@VisibleForTesting
24+
var _executorDispatcher: CoroutineDispatcher? = null
25+
1726
val executorDispatcher by lazy {
18-
Executors.newFixedThreadPool(numberOfCores).asCoroutineDispatcher()
27+
if (_executorDispatcher == null) {
28+
_executorDispatcher = Executors.newFixedThreadPool(numberOfAvailableCores).asCoroutineDispatcher()
29+
}
30+
_executorDispatcher!!
1931
}

‎Library/src/main/java/com/shopify/testify/internal/processor/ParallelPixelProcessor.kt‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ class ParallelPixelProcessor private constructor() {
4545

4646
private fun getChunkData(width: Int, height: Int): ChunkData {
4747
val size = width * height
48-
val chunkSize = size / numberOfCores
48+
val chunkSize = size / maxNumberOfChunkThreads
4949
val chunks = ceil(size.toFloat() / chunkSize.toFloat()).toInt()
5050
return ChunkData(size, chunks, chunkSize)
5151
}
@@ -55,7 +55,9 @@ class ParallelPixelProcessor private constructor() {
5555
launch(executorDispatcher) {
5656
(0 until chunkData.chunks).map { chunk ->
5757
async {
58-
for (i in (chunk * chunkData.chunkSize) until ((chunk + 1) * chunkData.chunkSize)) {
58+
val start = chunk * chunkData.wholeChunkSize
59+
val end = start + chunkData.getSizeOfChunk(chunk)
60+
for (i in start until end) {
5961
if (!fn(chunk, i)) break
6062
}
6163
}
@@ -113,8 +115,17 @@ class ParallelPixelProcessor private constructor() {
113115
private data class ChunkData(
114116
val size: Int,
115117
val chunks: Int,
116-
val chunkSize: Int
117-
)
118+
val wholeChunkSize: Int
119+
) {
120+
fun getSizeOfChunk(chunk: Int) = if (isLastChunk(chunk) && isUnevenChunkSize()) {
121+
size % wholeChunkSize
122+
} else {
123+
wholeChunkSize
124+
}
125+
126+
private fun isLastChunk(chunk: Int) = (chunk == chunks - 1)
127+
private fun isUnevenChunkSize(): Boolean = (size % wholeChunkSize != 0)
128+
}
118129

119130
private data class ImageBuffers(
120131
val width: Int,

‎Library/src/test/java/com/shopify/testify/ParallelPixelProcessorTest.kt‎

Lines changed: 89 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,63 +1,138 @@
11
package com.shopify.testify
22

33
import android.graphics.Bitmap
4+
import com.nhaarman.mockitokotlin2.any
45
import com.nhaarman.mockitokotlin2.doReturn
56
import com.nhaarman.mockitokotlin2.mock
67
import com.nhaarman.mockitokotlin2.whenever
78
import com.shopify.testify.internal.processor.ParallelPixelProcessor
8-
import com.shopify.testify.internal.processor.numberOfCores
9+
import com.shopify.testify.internal.processor._executorDispatcher
10+
import com.shopify.testify.internal.processor.maxNumberOfChunkThreads
11+
import kotlinx.coroutines.Dispatchers
12+
import kotlinx.coroutines.ExperimentalCoroutinesApi
13+
import kotlinx.coroutines.ObsoleteCoroutinesApi
14+
import kotlinx.coroutines.newSingleThreadContext
15+
import kotlinx.coroutines.test.resetMain
16+
import kotlinx.coroutines.test.setMain
17+
import org.junit.After
918
import org.junit.Assert.assertEquals
19+
import org.junit.Assert.assertTrue
1020
import org.junit.Before
1121
import org.junit.Test
22+
import java.util.concurrent.atomic.AtomicInteger
1223

24+
@ObsoleteCoroutinesApi
25+
@ExperimentalCoroutinesApi
1326
class ParallelPixelProcessorTest {
1427

28+
private val mainThreadSurrogate = newSingleThreadContext("UI thread")
29+
1530
companion object {
1631
const val WIDTH = 1080
1732
const val HEIGHT = 2220
1833
}
1934

20-
private fun mockBitmap(): Bitmap {
35+
private fun mockBitmap(width: Int = WIDTH, height: Int = HEIGHT): Bitmap {
2136
return mock<Bitmap>().apply {
22-
doReturn(WIDTH).whenever(this).height
23-
doReturn(HEIGHT).whenever(this).width
37+
doReturn(width).whenever(this).height
38+
doReturn(height).whenever(this).width
39+
doReturn(0xffffffff.toInt()).whenever(this).getPixel(any(), any())
2440
}
2541
}
2642

27-
private val expectedX = (0 until WIDTH).flatMap { (0 until HEIGHT).toList() }
28-
private val expectedY = (0 until WIDTH).flatMap { y -> (0 until HEIGHT).map { y } }
2943
private lateinit var pixelProcessor: ParallelPixelProcessor
3044

45+
private fun forceSingleThreadedExecution() {
46+
Dispatchers.setMain(mainThreadSurrogate)
47+
_executorDispatcher = Dispatchers.Main
48+
}
49+
3150
@Before
3251
fun setUp() {
52+
forceSingleThreadedExecution()
3353
pixelProcessor = ParallelPixelProcessor
3454
.create()
3555
.baseline(mockBitmap())
3656
.current(mockBitmap())
3757
}
3858

59+
@After
60+
fun tearDown() {
61+
Dispatchers.resetMain()
62+
mainThreadSurrogate.close()
63+
}
64+
3965
@Test
4066
fun default() {
41-
numberOfCores = 1
67+
maxNumberOfChunkThreads = 1
68+
69+
val index = AtomicInteger(0)
70+
pixelProcessor.analyze { _, _, _ ->
71+
index.incrementAndGet()
72+
true
73+
}
74+
assertEquals(WIDTH * HEIGHT, index.get())
75+
}
76+
77+
@Test
78+
fun twoCores() {
79+
maxNumberOfChunkThreads = 2
80+
81+
val index = AtomicInteger(0)
82+
pixelProcessor.analyze { _, _, _ ->
83+
index.incrementAndGet()
84+
true
85+
}
86+
87+
assertEquals(WIDTH * HEIGHT, index.get())
88+
}
89+
90+
@Test
91+
fun oddNumberOfCores() {
92+
maxNumberOfChunkThreads = 7
93+
94+
val index = AtomicInteger(0)
95+
pixelProcessor.analyze { _, _, _ ->
96+
index.incrementAndGet()
97+
true
98+
}
99+
100+
assertEquals(WIDTH * HEIGHT, index.get())
101+
}
102+
103+
@Test
104+
fun oddNumberOfPixels() {
105+
maxNumberOfChunkThreads = 2
106+
107+
pixelProcessor = ParallelPixelProcessor
108+
.create()
109+
.baseline(mockBitmap(3, 3))
110+
.current(mockBitmap(3, 3))
111+
112+
val expected = mutableSetOf(
113+
0 to 0, 1 to 0, 2 to 0,
114+
0 to 1, 1 to 1, 2 to 1,
115+
0 to 2, 1 to 2, 2 to 2
116+
)
42117

43-
var index = 0
118+
val index = AtomicInteger(0)
44119
pixelProcessor.analyze { _, _, (x, y) ->
45-
assertEquals(expectedX[index], x)
46-
assertEquals(expectedY[index], y)
47-
index++
120+
assertTrue(expected.remove(x to y))
121+
index.incrementAndGet()
48122
true
49123
}
124+
assertEquals(9, index.get())
125+
assertTrue(expected.isEmpty())
50126
}
51127

52128
private fun assertPosition(index: Int, position: Pair<Int, Int>) {
53-
val width = 1080
54-
val (x, y) = pixelProcessor.getPosition(index, width)
129+
val (x, y) = pixelProcessor.getPosition(index, WIDTH)
55130
assertEquals(position, x to y)
56131
}
57132

58133
@Test
59134
fun multicoreChunks() {
60-
numberOfCores = 2
135+
maxNumberOfChunkThreads = 2
61136
assertPosition(7, 7 to 0)
62137
assertPosition(500, 500 to 0)
63138
assertPosition(1500, 420 to 1)

‎build.gradle‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ buildscript {
2323
'colormath' : '1.4.1', // https://github.com/ajalt/colormath/releases
2424
'dokka' : '1.4.32', // https://github.com/Kotlin/dokka/releases
2525
'kotlin' : '1.4.31', // https://kotlinlang.org/
26+
'kotlinx' : '1.4.3', // https://github.com/Kotlin/kotlinx.coroutines/releases
2627
'ktlint' : '0.36.0', // https://github.com/pinterest/ktlint
2728
'material' : '1.3.0', // https://material.io/develop/android/docs/getting-started/
2829
'mockito2' : '3.3.3', // https://github.com/mockito/mockito/releases

0 commit comments

Comments
 (0)