This is in java. My program's purpose is to take screen captures of the computer screen many times per second and find all pixels that have a special shade of red. Then, it finds the average location of all the red pixels.
To improve the efficiency of my image processing, I created 3 threads, each processing 1/4 of the pixels. These threads plus the original thread will thus process all the pixels. However, I keep getting an error in my avgLocation() method. It's a null pointer exception, and I assume it was because the other threads were changing the size of the list containing all the red pixels, which causes the program to access data of a pixel that isn't there. In an attempt to remedy this, I used .join on Thread1 and Thread2 after Thread2's code, then I used .join on Thread3 after its code segment. So, all the threads should be joined before I called the avgLocation method, but it still keeps giving a NullPointerException whenever a specific shade of red shows up on screen. Here is the stack trace
Exception in thread "main" java.lang.NullPointerException
at Images.avgLocation(Images.java:151)
at Images.processImage(Images.java:133)
at Images.main(Images.java:169)
Line 151 is
xSum = list.get(i)[0];
Line 133 is Here is my code:
import java.awt.AWTException;
import java.awt.Color;
import java.awt.Dimension;
import java.awt.Point;
import java.awt.Rectangle;
import java.awt.Robot;
import java.awt.Toolkit;
import java.awt.image.BufferedImage;
import java.awt.image.DataBufferInt;
import java.util.ArrayList;
public class Images {
//takes in an input image and a target color and a bound to check within, returns Point to target
public static Point processImage(BufferedImage img, Color color, int error) throws InterruptedException {
long start = System.nanoTime();
//bounds to check on color components, make sure all within [0,255]
int redLower = color.getRed() - error;
int redHigher = color.getRed() error;
int greenLower = color.getGreen() - error;
int greenHigher = color.getGreen() error;
int blueLower = color.getBlue() - error;
int blueHigher = color.getBlue() error;
//place all components within [0,255]
if (redLower < 0) redLower = 0;
if (redHigher > 255) redHigher = 255;
if (greenLower < 0) greenLower = 0;
if (greenHigher > 255) greenHigher = 255;
if (blueLower < 0) blueLower = 0;
if (blueHigher > 255) blueHigher = 255;
//create final variables to use for thread
int redLowerF = redLower;
int redHigherF = redHigher;
int greenLowerF = greenLower;
int greenHigherF = greenHigher;
int blueLowerF = blueLower;
int blueHigherF = blueHigher;
//data of image
int[] pixels = ((DataBufferInt)img.getRaster().getDataBuffer()).getData();
int width = img.getWidth();
int height = img.getHeight();
//stores locations
ArrayList <Integer[]> locations = new ArrayList <>();
for (int i = 0; i < pixels.length/4; i ) {
int p = pixels[i];
// get red
int r = (p >> 16) & 0xff;
// get green
int g = (p >> 8) & 0xff;
// get blue
int b = p & 0xff;
//check if all components within bounds
if (r >= redLower && r<=redHigher && g>=greenLower && g <= greenHigher && b >= blueLower && b <= blueHigher) {
Integer[] point = {i % height, i/height};
locations.add(point);
}
}
Thread thread1 = new Thread( () -> {
for (int i = pixels.length/4; i < pixels.length/2; i ) {
int p = pixels[i];
// get red
int r = (p >> 16) & 0xff;
// get green
int g = (p >> 8) & 0xff;
// get blue
int b = p & 0xff;
//check if all components within bounds
if (r >= redLowerF && r<=redHigherF && g>=greenLowerF && g <= greenHigherF && b >= blueLowerF && b <= blueHigherF) {
Integer[] point = {i % height, i/height};
locations.add(point);
}
}
});
thread1.start();
Thread thread2 = new Thread( () -> {
for (int i = pixels.length/2; i < pixels.length*3/4; i ) {
int p = pixels[i];
// get red
int r = (p >> 16) & 0xff;
// get green
int g = (p >> 8) & 0xff;
// get blue
int b = p & 0xff;
//check if all components within bounds
if (r >= redLowerF && r<=redHigherF && g>=greenLowerF && g <= greenHigherF && b >= blueLowerF && b <= blueHigherF) {
Integer[] point = {i % height, i/height};
locations.add(point);
}
}
});
thread2.start();
thread1.join();
thread2.join();
Thread thread3 = new Thread( () -> {
for (int i = pixels.length*3/4; i < pixels.length; i ) {
int p = pixels[i];
// get red
int r = (p >> 16) & 0xff;
// get green
int g = (p >> 8) & 0xff;
// get blue
int b = p & 0xff;
//check if all components within bounds
if (r >= redLowerF && r<=redHigherF && g>=greenLowerF && g <= greenHigherF && b >= blueLowerF && b <= blueHigherF) {
Integer[] point = {i % height, i/height};
locations.add(point);
}
}
});
thread3.start();
thread3.join();
long end = System.nanoTime();
System.out.println((end-start)/1000000);
return avgLocation(locations);
}
//given 2D array of locations, finds average location of set and returns as point
public static Point avgLocation (ArrayList<Integer[]> list) {
//if no points in list, return an impossible point on screen
if (list.size() == 0) {
return new Point (-100, -100);
}
//coordinates of average location (set to 100 to reveal bug easily)
int avgX = 100;
int avgY = 100;
int xSum = 0;
int ySum = 0;
//loop through array
for (int i = 0; i < list.size(); i ) {
//for each location, add up coordinates
xSum = list.get(i)[0];
ySum = list.get(i)[1];
}
avgX = xSum/list.size();
avgY = ySum/list.size();
return new Point (avgX, avgY);
}
public static void main (String[] args) {
Robot robot = null;
try {
robot = new Robot();
} catch (AWTException e) {
// TODO Auto-generated catch block
e.printStackTrace();
}
Dimension d = Toolkit.getDefaultToolkit().getScreenSize();
while (true) {
try {
processImage(robot.createScreenCapture(new Rectangle(0,0,d.width,d.height)), new Color (255, 0, 0), 10);
} catch (InterruptedException e) {
// TODO Auto-generated catch block
e.printStackTrace();
}
}
}
}
CodePudding user response：
Well, what's wrong with your thread synchronization is that you have none at all. Your attempts to avoid concurrent access to the ArrayList by using join() failed, because your still having thread1 and thread2 run concurrently. If you stop them from running at the same time, any problems with the ArrayList would disappear, but of course, you also wouldn’t have any benefit from using multiple threads at all.
Now, you could replace the list with a thread safe implementation or add manual synchronization for the access, but that’s not how you should do this. This synchronization adds an overhead which can be larger than any potential gain from calculating concurrently. After all, the arithmetic performed per pixel isn’t complex.
Instead, you should preferably let all threads use their own lists to which they can add without contention. Then, merge the lists after two threads completed their work.
To make this task easier, you should refactor the code to have one method having parameters to control the range to process, instead of having the same code four times.
Further, you can replace Integer[] with int[], to avoid boxing primitive values into objects.
// takes in an input image and a target color and a bound to check within,
// returns Point to target
public static Point processImage(BufferedImage img, Color color, int error)
throws InterruptedException, ExecutionException {
long start = System.nanoTime();
//bounds to check on color components, make sure all within [0,255]
int redLower = color.getRed() - error;
int redHigher = color.getRed() error;
int greenLower = color.getGreen() - error;
int greenHigher = color.getGreen() error;
int blueLower = color.getBlue() - error;
int blueHigher = color.getBlue() error;
//place all components within [0,255]
if (redLower < 0) redLower = 0;
if (redHigher > 255) redHigher = 255;
if (greenLower < 0) greenLower = 0;
if (greenHigher > 255) greenHigher = 255;
if (blueLower < 0) blueLower = 0;
if (blueHigher > 255) blueHigher = 255;
//data of image
int[] pixels = ((DataBufferInt)img.getRaster().getDataBuffer()).getData();
int width = img.getWidth();
int height = img.getHeight();
//stores locations
ArrayList <int[]> locations = processPixels(pixels, height, 0, pixels.length, 1,
redLower, redHigher, greenLower, greenHigher, blueLower, blueHigher);
long end = System.nanoTime();
System.out.println((end-start)/1000000);
return avgLocation(locations);
}
private static ArrayList<int[]> processPixels(
int[] pixels, int height, int from, int to, int divide,
int redLower, int redHigher, int greenLower, int greenHigher,
int blueLower, int blueHigher)
throws InterruptedException, ExecutionException {
List<FutureTask<ArrayList<int[]>>> jobs = new ArrayList<>();
while(divide > 0 && to - from > 1) {
int divideF = --divide;
int mid = (from to) >>> 1;
int jobFrom = from, jobTo = mid;
FutureTask<ArrayList<int[]>> f = new FutureTask<>(() -> processPixels(
pixels, height, jobFrom, jobTo, divideF,
redLower, redHigher, greenLower, greenHigher, blueLower, blueHigher));
new Thread(f).start();
jobs.add(f);
from = mid;
}
ArrayList<int[]> locations = new ArrayList<>();
for (int i = from; i < to; i ) {
int p = pixels[i];
// get red
int r = (p >> 16) & 0xff;
// get green
int g = (p >> 8) & 0xff;
// get blue
int b = p & 0xff;
//check if all components within bounds
if (r >= redLower && r<=redHigher && g>=greenLower && g <= greenHigher
&& b >= blueLower && b <= blueHigher) {
int[] point = {i % height, i/height};
locations.add(point);
}
}
for(FutureTask<ArrayList<int[]>> j: jobs) locations.addAll(j.get());
return locations;
}
//given 2D array of locations, finds average location of set and returns as point
public static Point avgLocation(ArrayList<int[]> list) {
//if no points in list, return an impossible point on screen
if (list.size() == 0) {
return new Point (-100, -100);
}
//coordinates of average location (set to 100 to reveal bug easily)
int avgX = 100, avgY = 100;
int xSum = 0, ySum = 0;
//loop through array
for (int i = 0; i < list.size(); i ) {
//for each location, add up coordinates
xSum = list.get(i)[0];
ySum = list.get(i)[1];
}
avgX = xSum/list.size();
avgY = ySum/list.size();
return new Point(avgX, avgY);
}
The processPixels will do the job of processing the specified range, but first divide the work by halving the range and spawn new threads to call the same method with one half while keeping the other. Since the method executed by the other threads will also create new threads according to the divide, there will be 2divide threads working on the job. So pass 0 for single threaded, 1 for two threads, 2 for four threads, 3 for eight, and so on.
All threads will perform their work, adding to a local list, and add the result of the sub tasks afterwards, if any.
But note that a similar concept has been implemented in Java already, plus dynamic adaptation to the number of available CPU cores and actual workload. You get it for free when using
// takes in an input image and a target color and a bound to check within,
// returns Point to target
public static Point processImage(BufferedImage img, Color color, int error) {
long start = System.nanoTime();
//bounds to check on color components, make sure all within [0,255]
int redLower = color.getRed() - error;
int redHigher = color.getRed() error;
int greenLower = color.getGreen() - error;
int greenHigher = color.getGreen() error;
int blueLower = color.getBlue() - error;
int blueHigher = color.getBlue() error;
//place all components within [0,255]
if (redLower < 0) redLower = 0;
if (redHigher > 255) redHigher = 255;
if (greenLower < 0) greenLower = 0;
if (greenHigher > 255) greenHigher = 255;
if (blueLower < 0) blueLower = 0;
if (blueHigher > 255) blueHigher = 255;
//create final variables to use for function
int redLowerF = redLower;
int redHigherF = redHigher;
int greenLowerF = greenLower;
int greenHigherF = greenHigher;
int blueLowerF = blueLower;
int blueHigherF = blueHigher;
//data of image
int[] pixels = ((DataBufferInt)img.getRaster().getDataBuffer()).getData();
int width = img.getWidth();
int height = img.getHeight();
List<int[]> locations = IntStream.range(0, pixels.length).parallel()
.filter(i -> {
int p = pixels[i];
// get red
int r = (p >> 16) & 0xff;
// get green
int g = (p >> 8) & 0xff;
// get blue
int b = p & 0xff;
//check if all components within bounds
return r >= redLowerF && r<=redHigherF && g>=greenLowerF && g <= greenHigherF
&& b >= blueLowerF && b <= blueHigherF;
})
.mapToObj(i -> new int[] {i % height, i / height})
.collect(Collectors.toList());
//stores locations
long end = System.nanoTime();
System.out.println((end-start)/1000000);
return avgLocation(locations);
}
//given 2D array of locations, finds average location of set and returns as point
public static Point avgLocation(List<int[]> list) {
//if no points in list, return an impossible point on screen
if (list.size() == 0) {
return new Point (-100, -100);
}
//coordinates of average location (set to 100 to reveal bug easily)
int avgX = 100, avgY = 100;
int xSum = 0, ySum = 0;
//loop through array
for (int i = 0; i < list.size(); i ) {
//for each location, add up coordinates
xSum = list.get(i)[0];
ySum = list.get(i)[1];
}
avgX = xSum/list.size();
avgY = ySum/list.size();
return new Point(avgX, avgY);
}
We can improve the code even more, as we do not need to express the points as int[] arrays, when we know the relationship between indices and coordinates:
public static Point processImage(BufferedImage img, Color color, int error) {
long start = System.nanoTime();
//bounds to check on color components, make sure all within [0,255]
int redLower = color.getRed() - error;
int redHigher = color.getRed() error;
int greenLower = color.getGreen() - error;
int greenHigher = color.getGreen() error;
int blueLower = color.getBlue() - error;
int blueHigher = color.getBlue() error;
//place all components within [0,255]
if (redLower < 0) redLower = 0;
if (redHigher > 255) redHigher = 255;
if (greenLower < 0) greenLower = 0;
if (greenHigher > 255) greenHigher = 255;
if (blueLower < 0) blueLower = 0;
if (blueHigher > 255) blueHigher = 255;
//create final variables to use for function
int redLowerF = redLower;
int redHigherF = redHigher;
int greenLowerF = greenLower;
int greenHigherF = greenHigher;
int blueLowerF = blueLower;
int blueHigherF = blueHigher;
//data of image
int[] pixels = ((DataBufferInt)img.getRaster().getDataBuffer()).getData();
int width = img.getWidth();
int height = img.getHeight();
int[] locations = IntStream.range(0, pixels.length).parallel()
.filter(i -> {
int p = pixels[i];
// get red
int r = (p >> 16) & 0xff;
// get green
int g = (p >> 8) & 0xff;
// get blue
int b = p & 0xff;
//check if all components within bounds
return r >= redLowerF && r<=redHigherF && g>=greenLowerF && g <= greenHigherF
&& b >= blueLowerF && b <= blueHigherF;
})
.toArray();
//stores locations
long end = System.nanoTime();
System.out.println((end-start)/1000000);
return avgLocation(locations, height);
}
private static Point avgLocation(int[] locations, int height) {
if (locations.length == 0) {
return new Point (-100, -100);
}
//coordinates of average location (set to 100 to reveal bug easily)
int avgX = 100, avgY = 100;
int xSum = 0, ySum = 0;
//loop through array
for (int i: locations) {
int x = i % height, y = i / height;
//for each location, add up coordinates
xSum = x;
ySum = y;
}
avgX = xSum/locations.length;
avgY = ySum/locations.length;
return new Point(avgX, avgY);
}
CodePudding user response：
ArrayList is not thread safe
Create a method like
static private void addToList (ArrayList<Integer[]> list, Integer [] p) {
synchronized (list) {
list.add(p);
}
}
which can be called in your Threads like
Image.addToList(locations, point);
CodePudding user response：
The threads concurrently modify and change the locations ArrayList by adding new elements. ArrayList is not suitable for such concurrent modification without some synchronisation.
However, there is CopyOnWriteArrayList that can support concurrent modification without synchronisation.
Try replacing line:
ArrayList<Integer[]> locations= new ArrayList<>();
With the following line:
final List<Integer []> locations = new CopyOnWriteArrayList<>();