Codereview#2 of 1 (Garrido) from 14 (Romanello)#122
Codereview#2 of 1 (Garrido) from 14 (Romanello)#122Nintenking wants to merge 486 commits intoPrandiniUniPD:codereviewfrom
Conversation
ingredientsActivity is started from resultActivit in the menu.
…FromDb Download images from DB
Better visualization showing similarity Sorting by similarity Moved processing into a thread Showing progress Dialog
moved button from menu into a floating action button in result activity
# Conflicts: # app/src/main/AndroidManifest.xml # app/src/main/java/unipd/se18/ocrcamera/AdapterTestAlterations.java # app/src/main/java/unipd/se18/ocrcamera/AdapterTestElement.java # app/src/main/java/unipd/se18/ocrcamera/PhotoTester.java # app/src/main/java/unipd/se18/ocrcamera/TestElement.java
| Mat brightnessMat = new Mat(); | ||
|
|
||
| //Changes the format of the matrix into a RGB one | ||
| Imgproc.cvtColor(imageMat, brightnessMat, Imgproc.COLOR_RGBA2RGB); |
There was a problem hiding this comment.
I think you did this for being able to split the color using the next few lines with Core.split right? Shouldn't be better to add this to the comment?
| */ | ||
| private BrightnessValue isBright(Mat imageMat){ | ||
| //Converts the image into a matrix | ||
| Mat brightnessMat = new Mat(); |
There was a problem hiding this comment.
I suggest to change the name of this to something like rgbImageMat, in this way I know that I'm working with an RGB image.
Useful also because at line 193 reading the parameter of the switch I know that I'm working with an image
| case CV_64F: bit = 64; break; | ||
| default: return BrightnessValue.IMAGE_IS_OK; | ||
| } | ||
| //Calculate the percentage of the brightness |
There was a problem hiding this comment.
A better description or documentation for what you are doing here should be useful to understand correctly this part of the code.
| Imgproc.cvtColor(imageMat, brightnessMat, Imgproc.COLOR_RGBA2RGB); | ||
|
|
||
| //Obtain 3 different matrix with the 3 elemental colors | ||
| List<Mat> color = new ArrayList<>(); |
There was a problem hiding this comment.
I suggest to change this to imageColors. In this what I know that I'm working with the image that I want to work with and not with any other object containing colors
| case CV_32S: bit = 32; break; | ||
| case CV_32F: bit = 32; break; | ||
| case CV_64F: bit = 64; break; | ||
| default: return BrightnessValue.IMAGE_IS_OK; |
There was a problem hiding this comment.
BrightnessValue is an enum and BrightnessValue.IMAGE_IS_OK corresponds to 0.
The cases contain all the possible bits of an image, right? When I don't get one of them I return IMAGE_IS_OK but if there is an error in the image and opencv is not able to read its depth this is not correct because I can't know if the brightness of the image is really ok.
There was a problem hiding this comment.
We understood that we'll always have 8 bit images so we are removing the switch
| * @return the image with the modified brightness | ||
| * @author Thomas Porro(g1), Giovanni Fasan(g1), Oscar Garrido(g1) | ||
| */ | ||
| private Bitmap editBright(Bitmap image){ |
There was a problem hiding this comment.
I suggest to change the name of the method to something like "autoAdjustImageBrightness" because I would use "editBright" for methods where I have to tell the new brightness.
| bright = IPUtils.conversionBitmapToMat(image); | ||
| } catch (ConversionFailedException error){ | ||
| Log.e(TAG, error.getErrorMessage()); | ||
| return image; |
There was a problem hiding this comment.
I suggest to return null in this case and manage the exception from where you call editBright() or create a custom exception because is not correct to return an image even if it doesn't have been processed.
There was a problem hiding this comment.
I thought to create a different object that contains both the image and the TAG, saying if the image was analyzed or not
| return IPUtils.conversionMatToBitmap(modifiedMat); | ||
| } catch (ConversionFailedException error){ | ||
| Log.e(TAG, error.getErrorMessage()); | ||
| return image; |
There was a problem hiding this comment.
Same comment as line 244
| return IPUtils.conversionMatToBitmap(modifiedMat); | ||
| } catch (ConversionFailedException error){ | ||
| Log.e(TAG, error.getErrorMessage()); | ||
| return image; |
There was a problem hiding this comment.
Same comment as line 244
| final int step = 15; | ||
|
|
||
| //Converts the image into a matrix | ||
| Mat bright; |
There was a problem hiding this comment.
I suggest to change the name of this object to "imageMap" because as is I can't understand that I'm working with the image
| case IMAGE_TOO_BRIGHT: | ||
| Log.d(TAG, "Case==IMAGE_TOO_BRIGHT"); | ||
| //Darkens the colour's brightness until it's in an optimal value | ||
| for(double changeBrightness = 0; changeBrightness != maxBrightness; |
There was a problem hiding this comment.
Maybe I misunderstood but In this way I can end to an infinite loop (avoided by
if(isBright(modifiedMat) == BrightnessValue.IMAGE_IS_OK)) , changeBrightness is always different from maxBrightness. I suggest to add a minBrightness=-240 value for example and use
for(double changeBrightness = 0; changeBrightness >= minBrightness ; changeBrightness -= step)
What do you think?
| Log.d(TAG, "Case==IMAGE_TOO_DARK"); | ||
| //The variables are explained in the case above | ||
| //Lightens the colour's brightness until it's in an optimal value | ||
| for(double changeBrightness = 0; changeBrightness != maxBrightness; |
There was a problem hiding this comment.
Maybe I misunderstood also here but In this way I can end to an infinite loop (avoided also here by
if(isBright(modifiedMat) == BrightnessValue.IMAGE_IS_OK)).
I suggest to use
for(double changeBrightness = 0; changeBrightness <= maxBrightness; changeBrightness += step)
What do you think?
There was a problem hiding this comment.
We are making the switch smaller and avoiding the for cycles
| int maxLap = -16777216; | ||
|
|
||
| //Threshold above which the color is out of focus | ||
| final int threshold = -6118750; |
There was a problem hiding this comment.
I suggest to change the name of this to outOfFocusThreshold for a more clear idea of what this is variable is for
| imageMat = IPUtils.conversionBitmapToMat(image); | ||
| } catch (ConversionFailedException error){ | ||
| Log.e(TAG, error.getErrorMessage()); | ||
| return false; |
There was a problem hiding this comment.
Why returning false if the image has not yet been processed? I suggest to return null in this case and manage the exception from where you call isBlurred() or create a custom exception.
There was a problem hiding this comment.
We created an enum to manage the different situations
| laplacianImage = IPUtils.conversionMatToBitmap(laplacianMat8Bit); | ||
| } catch (ConversionFailedException error){ | ||
| Log.e(TAG, error.getErrorMessage()); | ||
| return false; |
There was a problem hiding this comment.
Same as line 336
| public boolean isBlurred(Bitmap image) { | ||
|
|
||
| //Total number of color | ||
| int maxLap = -16777216; |
There was a problem hiding this comment.
I suggest to add to the comment why you are instantiating this to -16777216
| ); | ||
|
|
||
| //Searches the pixel that has the highest colour range in the RGB format | ||
| for(int pixel : pixels){ |
There was a problem hiding this comment.
I don't understand this part. What are the integer values inside pixels[]?? The brightness of each pixel or a special value obtained using the laplacianImage? At the end of the for cycle maxLap what contains? The brightest pixel?
There was a problem hiding this comment.
We read in various articles that a way to estimate the sharpness of an image is to apply a Laplace filter and then pick the maximum value. I'll add this information as a comment in the code
| Mat greenLuminance= new Mat(); | ||
| Core.multiply(color.get(1), new Scalar(0.7152), greenLuminance); | ||
| Mat blueLuminance = new Mat(); | ||
| Core.multiply(color.get(2), new Scalar(0.0722), blueLuminance); |
There was a problem hiding this comment.
I suggest to create constant variables for these values
|
Should be ok now, if you think it is correct could you close the pull request so we can merge the module in the Demo? |
8 participants
Hi Stefano,
here is the code for the review. The methods I'd ask you to look at are:
-"BrightnessValue isBright(Mat imageMat)",
All 3 of them are inside the class PreProcessing.java
I've also left in this pull request the whole ImageProcessing Module, perhaps you can more easily understand the methods this way.
Thank you for your time