-
Notifications
You must be signed in to change notification settings - Fork 136
Fix NPE while making the screenshot bitmap mutable #1005
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem can be solved way more easily, see the comments for details.
Thanks for the contribution!
@@ -105,7 +105,11 @@ public static void startCalibration(@NonNull Context context, | |||
sCalibrationImage = bitmap; | |||
} else { | |||
// Make a mutable copy of the bitmap so we can draw on it with a Canvas | |||
sCalibrationImage = bitmap.copy(sCalibrationImage.getConfig() ,true); | |||
sCalibrationImage = bitmap.copy(Bitmap.Config.ARGB_8888 ,true); | |||
if (sCalibrationImage == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is useless since the copy can't return a null bitmap.
Please remove this added lines since they're not helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be NULL according to the reference:
If the conversion is not supported, or the allocator fails, then this returns NULL
but yea, I think it's a pretty rare case and maybe just doesn't worth bothering at all
@@ -105,7 +105,11 @@ public static void startCalibration(@NonNull Context context, | |||
sCalibrationImage = bitmap; | |||
} else { | |||
// Make a mutable copy of the bitmap so we can draw on it with a Canvas | |||
sCalibrationImage = bitmap.copy(sCalibrationImage.getConfig() ,true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace this line with this instead:
sCalibrationImage = bitmap.copy(bitmap.getConfig(), true);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also what I thought originally but I checked the reference and realized getConfig() could actually return NULL.
If the bitmap's internal config is in one of the public formats, return that config, otherwise return null.
Maybe I just make an if there
@@ -105,7 +105,11 @@ public static void startCalibration(@NonNull Context context, | |||
sCalibrationImage = bitmap; | |||
} else { | |||
// Make a mutable copy of the bitmap so we can draw on it with a Canvas | |||
sCalibrationImage = bitmap.copy(sCalibrationImage.getConfig() ,true); | |||
sCalibrationImage = bitmap.copy(Bitmap.Config.ARGB_8888 ,true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Bitmap.Config.ARGB_8888
might be a waste of space if the screenshot bitmap used a lower color configuration.
See the previous suggestion to copy the configuration from the screenshot bitmap.
Amended |
sCalibrationImage is null at this point, causing NPE when invoking getConfig()
stack trace: