Library and demo app - #6
rovertrack wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a reusable jpegtran Android library that wraps the existing native jpegtran code behind a high-level Java API (getProperties, rotate, crop, blur) and adds a new commons-jpegtran-demo app that exercises that API with interactive crop/blur overlays and pinch-to-zoom. Build tooling (AGP 8.6.0, Gradle 8.7) and per-module SDK levels are also updated to support the new modules.
Changes:
- New
jpegtranlibrary module:Jpegtranwrapper class,BlurRegion,RotationDegree,Properties, dedicated CPP copy withfr_free_nrw_commons_jpegtran_Jpegtran_*JNI names, multi-region pixelize support. - New
commons-jpegtran-demoapp withMainActivity,BlurOverlayView,CropOverlayView, plus resources/manifest, depending on:jpegtran. - Build upgrades: AGP 7.1.3 → 8.6.0, Gradle 7.2 → 8.7, library
compileSdk 35/minSdk 26,appnamespace added, IDE files updated.
Reviewed changes
Copilot reviewed 26 out of 45 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle | Includes the new commons-jpegtran-demo module. |
| build.gradle | Upgrades AGP plugin to 8.6.0. |
| gradle/wrapper/gradle-wrapper.properties | Upgrades Gradle wrapper to 8.7 with SHA-256. |
| jpegtran/build.gradle | Library config: compile/min SDK bumps, ndkBuild path, NDK version. |
| jpegtran/src/main/java/.../Jpegtran.java | High-level API: getProperties/rotate/crop/blur, background executor, sequential pixelize. |
| jpegtran/src/main/java/.../BlurRegion.java | Immutable blur region data class and option-string serializer. |
| jpegtran/src/main/java/.../RotationDegree.java | Enum of allowed rotations (90/180/270). |
| jpegtran/src/main/java/.../Properties.java | Public JPEG properties container returned by getProperties. |
| commons-jpegtran-demo/build.gradle | New demo app module configuration. |
| commons-jpegtran-demo/src/main/AndroidManifest.xml | Demo app manifest. |
| commons-jpegtran-demo/src/main/java/.../MainActivity.java | Demo flows for open/save/crop/rotate/blur using the library. |
| commons-jpegtran-demo/src/main/java/.../BlurOverlayView.java | Overlay with pinch zoom, multi-region draw, delete-X handles, region mapping. |
| commons-jpegtran-demo/src/main/java/.../CropOverlayView.java | Resizable crop overlay with handles, grid, and gesture exclusion. |
| commons-jpegtran-demo/src/main/res/* | Layouts, themes, strings, colors, launcher icons. |
| commons-jpegtran-demo/src/{test,androidTest}/.../Example*.java | Template-generated unit/instrumented tests. |
| app/build.gradle | Adds required namespace; bumps buildToolsVersion. |
| .gitignore | Replaces blanket .idea ignore with selective ignores. |
| .idea/* | IDE configuration updates (JDK 17, deployment selector, etc.). |
Files not reviewed (7)
- .idea/appInsightsSettings.xml: Language not supported
- .idea/compiler.xml: Language not supported
- .idea/deploymentTargetSelector.xml: Language not supported
- .idea/deviceManager.xml: Language not supported
- .idea/gradle.xml: Language not supported
- .idea/misc.xml: Language not supported
- .idea/vcs.xml: Language not supported
Comments suppressed due to low confidence (1)
jpegtran/build.gradle:41
- Several library classes import
org.jspecify.annotations.NonNull/Nullable(here,Jpegtran.java,BlurRegion.java, and the overlay views) but neitherjpegtran/build.gradlenorcommons-jpegtran-demo/build.gradledeclares an explicitorg.jspecify:jspecifydependency. Compilation currently relies on it being pulled in transitively (e.g. by AppCompat'sandroidx.annotation); if that ever changes the build will break. The existing app module usesandroidx.annotation.NonNullconsistently — consider standardizing on AndroidX annotations or addingimplementation 'org.jspecify:jspecify:<version>'to both modules' dependencies.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| jpegtran.getProperties(lloadUri, p -> { | ||
| // Display properties | ||
| TextView textView = (TextView) findViewById(R.id.text_view); | ||
| propertyStr = "File name : " + p.fileName + "\n"; | ||
| propertyStr += "File size : " + p.fileSize + "\n"; | ||
| propertyStr += "Width : " + p.width + "\n" + "Height : " + p.height + "\n"; | ||
| propertyStr += "MCU Width : " + p.MCU_Width + "\n" + "MCU Height : " + p.MCU_Height + "\n"; | ||
| propertyStr += "Color space : " + p.Color_space + "\n"; | ||
| textView.setText(propertyStr); | ||
|
|
||
| }); |
| final Cursor returnCursor = resolver.query(fileUri, null, null, null, null); | ||
| final String filename; | ||
| final long filesize; | ||
|
|
||
| if (returnCursor != null) { | ||
| int index = returnCursor.getColumnIndex(OpenableColumns.DISPLAY_NAME); | ||
| returnCursor.moveToFirst(); | ||
| filename = returnCursor.getString(index); | ||
| index = returnCursor.getColumnIndex(OpenableColumns.SIZE); | ||
| filesize = returnCursor.getLong(index); | ||
| returnCursor.close(); | ||
| } else { | ||
| filename = null; | ||
| filesize = 0; | ||
| } |
| final int component_num = retarray[2]; | ||
| final int mcu_width = retarray[3] * 8; | ||
| final int mcu_height = retarray[4] * 8; | ||
| final String[] color_space_str = {"Unknown", "Grayscale", "RGB", "YCbCr", "CMYK", "YCbCrK", "RGB", "YCbCr"}; |
| <string name="button_">Flip</string> | ||
| <string name="button_rotate">Rotate</string> | ||
| <string name="button_blur">blur</string> |
| public int MCU_Width; | ||
| /** | ||
| * Height of one MCU block of the JPEG, | ||
| * | ||
| */ | ||
| public int MCU_Height; | ||
| /** | ||
| * Color space of the JPEG | ||
| * <p> | ||
| * Unknown, Grayscale, RGB, YCbCr, CMYK, YCbCrK, RGB, YCbCr. | ||
| * | ||
| */ | ||
| public String Color_space; |
| public List<RectF> getDrawnRegions() { | ||
| return regions; | ||
| } |
| invalidate(); | ||
| } | ||
|
|
||
| protected List<BlurRegion> getMappedBlurRegions(ImageView imageView, BlurOverlayView overlayView) { |
| if (p != null) { | ||
| Bitmap bitmap = BitmapFactory.decodeFile(p.getAbsolutePath()); | ||
| runOnUiThread(() -> { |
| static Uri loadUri = null; | ||
| static String propertyStr = null; |
| try { | ||
| // Workaround : To update mediastore, execute empty write. | ||
| OutputStream outstream = resolver.openOutputStream(saveUri, "wa"); | ||
| outstream.flush(); | ||
| outstream.close(); | ||
| } catch (IOException e) { | ||
| // Discard exception | ||
| // Mediastore may not be updated, but no recovery operation and continuable. | ||
| } |
RitikaPahwa4444
left a comment
There was a problem hiding this comment.
Great start! The API mostly looks good. Had a few questions and suggestions, do share your thoughts on them :)
| } | ||
|
|
||
| android { | ||
| namespace 'github.kamemak.ajpegtran_example' |
There was a problem hiding this comment.
Namespace could be modified, right?
| FileInputStream fis = new FileInputStream(rparcelFd.getFileDescriptor()); | ||
| FileOutputStream fos = new FileOutputStream(wparcelFd.getFileDescriptor())) { | ||
| // Sequentially read and write to save uri. | ||
| byte[] buf = new byte[8192]; |
There was a problem hiding this comment.
Magic number? How would the usage look like in the Commons app?
| * File name of the JPEG. | ||
| * | ||
| */ | ||
| public String fileName; |
| * @param post callback receiving the output file, or null on error | ||
| */ | ||
| public void crop(@NonNull Uri fileUri, | ||
| @NonNull Integer width, @NonNull Integer height, |
There was a problem hiding this comment.
int or Integer with @NonNull? This made me curious: https://stackoverflow.com/questions/57226605/is-it-better-to-annotate-integer-with-nonnull-or-use-primitive-int
|
|
||
| defaultConfig { | ||
| minSdk 21 | ||
| minSdk 26 |
| final String color_space = (retarray[5] >= 0 && retarray[5] <= JCS_BG_YCC) | ||
| ? color_space_str[retarray[5]] | ||
| : color_space_str[0]; // Unknown | ||
| handler.post(() -> post.accept(new Properties(filename, filesize, width, height, mcu_width, mcu_height, color_space))); |
There was a problem hiding this comment.
We're not tracking component_num, is that not useful for us? We can remove it then.
| Log.d(TAG, "pixelize error: " + e.getMessage()); | ||
| if (tempA != null) tempA.delete(); | ||
| if (tempB != null) tempB.delete(); | ||
| handler.post(() -> post.accept(null)); |
There was a problem hiding this comment.
Our callback hides the failure message from the consumer and just makes the happy path visible, is that intended? I am comparing a generic toast with displaying what exactly went wrong (consumer can decide here if the message is too technical) - the second callback would have both onSuccess() and onFailure().
| final File finalTempFile = tempFile; | ||
| handler.post(() -> post.accept(finalTempFile)); | ||
| } else { | ||
| tempFile.delete(); |
There was a problem hiding this comment.
The library handles deletion of temp files on failures. Let's say I call rotate() thrice and all of them succeed. What would be the clean-up process here?
| import java.util.concurrent.Executors; | ||
| import java.util.function.Consumer; | ||
|
|
||
| public class Jpegtran { |
There was a problem hiding this comment.
What's the responsibility of this class? I feel it's handling a lot of things. Can this be split into subclasses? How about having an interface that the consumer would interact with, and this class implementing that interface?
|
Please make file I had to modify this: |
| @NonNull Consumer<File> post) { | ||
|
|
||
| // Build options from params. | ||
| String options = "-rotate " + rotation.getDegrees() + " -optimize -copy all"; |
There was a problem hiding this comment.
So we are basically calling the library's command-line-like interface?
Is there not a direct interface that we could call? That would be better for type-checking and could allow us to drop the command-line parsing code (this could be kept as a later improvement task though).
There was a problem hiding this comment.
Yeah, that's how it works in the author's ajpegtran as well!
Under the hood it's passing the argument to CLI!
There was a problem hiding this comment.
But I guess there is a function call behind the CLI, right?
There was a problem hiding this comment.
So you are saying modify the native C to take params instead of CLI options?
There was a problem hiding this comment.
Under the hood, matching -rotate calls select_transform(JXFORM_ROT_90) etc.
The idea would be to directly call select_transform rather than using the CLI interface. I believe that would be cleaner.
@RitikaPahwa4444 Any opinion on this?
There was a problem hiding this comment.
but that means we must expose that function too right!?
There was a problem hiding this comment.
Yes.
And all of the CLI-related code could be deleted.
There was a problem hiding this comment.
Ok works for me if it makes the best for library :)
but Iwill also wait for one approval from @RitikaPahwa4444.
There was a problem hiding this comment.
ok thanks Ritika
| Uri tempBUri = Uri.fromFile(tempB); | ||
|
|
||
| // apply blur sequentially for each region in the list. | ||
| for (int i = 0; i < regions.size(); i++) { |
There was a problem hiding this comment.
Is there no way to perform several regions pixelizations in the same JNI call? On low-end devices blurring many regions might take time if we read/write the picture once per region.
There was a problem hiding this comment.
As far as I have tested it can take upto 3-4 at a time in the CLI version, even if we passed it ir modify the underlying C source, then too it is applied sequentially!
If less JNI call is priority, we can make the native C do the heavylifting!
|
The blur is perfect, other blocks untouched. |
Is there any particular image tester you are using? |
|
Somehow in this PR's jpegtran app when I click "Open" and select a picture, the metadata is shown but nothing else. |
Is it the upstream app or the one which was created to test out library. The details you are describing is of the app which was upstream kamemak authored! Which only opens, shows metadata and saves with hardcoded options. |
|
Ah thanks Rishan, very useful, I was using the wrong app indeed! I will uninstall the upstream app. |
|
Was pretty sure that was what happened 👍 |
|
Also it's ok if you dont test this much, this week I'm gonna be restructuring based on you recommendation of exposing methods per feature! |
|
Rotation and crop also work well: The above UI is https://github.com/nicolas-raoul/jpeg_diff_android |




library module :
Jpegtranwhich has its own copy of the CPP source files so no naming conflicts occurs.simplified the API, the consumer just call the required features such as Crop, Blur, Rotate no need to pass in CMD commands.
added support for multiple blur regions, which is nothing but multiple pixelize commands firing sequentially.
at present only supports get_properties, Crop, Rotate, Blur.
every feature runs in background thread.
Demo app:
Demo:
az_recorder_20260529_020800.mp4