[camera] Add setImageQuality for JPEG compression control#11155
[camera] Add setImageQuality for JPEG compression control#11155Bolling88 wants to merge 23 commits intoflutter:mainfrom
Conversation
Bolling88
requested review from
LouiseHsu,
bparrishMines,
camsim99 and
hellohuanlin
as code owners
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the setImageQuality method to control JPEG compression quality for still image captures across the camera plugin. The implementation is federated, with platform-specific logic for Android (Camera2 and CameraX) and iOS. The Android Camera2 implementation uses CaptureRequest.JPEG_QUALITY. The CameraX implementation recreates the ImageCapture use case to apply the quality setting, preserving any locked capture orientation. The iOS implementation re-encodes JPEG images to the specified quality while retaining EXIF metadata. The changes are accompanied by updates to the platform interface, method channels, and comprehensive tests for each platform.
Bolling88
force-pushed
the
fix-set-quality
branch
from
8c7c979 to
52b4cff
Compare
|
Thanks for the contribution!
In the future, please use our checklist rather than replacing it with one you have invented. Our checklist is standardized for a reason.
Please provide the actual issue this is related to. That's not even a |
Sorry for that! My first contribution, and still learning! Regarding the issue, I just took a placeholder and forgot to change it. This is rather a feature improvement than resolving an issue, but I found and linked a closed issue where one of the comments requested the possibility to set image quality. If that is not enough, can I just create a new issue? |
As with almost every project, we track bug reports and feature requests in the same issue tracker.
An issue that was fixed seven years ago does not provide an explanation of what current use case needs new APIs. If there isn't an issue covering the use case you are trying to address, then you should file one. |
Thank you for your patience. I have created a new issue ticket that clearly explains the problem/feature. |
packages/camera/camera/CHANGELOG.md
Outdated
| @@ -1,3 +1,7 @@ | |||
| ## 0.12.1 | |||
|
|
|||
| * Adds `setImageQuality` for controlling JPEG compression quality. | |||
There was a problem hiding this comment.
is this function JPEG only? if so, better to put it in the function name
There was a problem hiding this comment.
The API is intentionally named setImageQuality rather than setJpegQuality for a couple of reasons:
- It's consistent with Flutter's existing
image_pickerplugin which usesimageQualitywith the same 1–100 int scale. - It's more future-proof — if quality control is ever extended to other formats (e.g. HEIF), the API name still makes sense.
- From the app developer's perspective, they're setting the quality of the captured image, regardless of the underlying format.
The current implementation applies to JPEG only (HEIF passes through unmodified), but the abstraction level feels right for a cross-platform API.
| let savePhotoDelegate = SavePhotoDelegate( | ||
| path: path, | ||
| ioQueue: photoIOQueue, | ||
| imageQuality: fileExtension == "jpg" && imageQuality < 100 ? imageQuality : nil, |
There was a problem hiding this comment.
what if file extension is jpeg?
does nil quality mean 100% quality?
can quality be a floating point number?
There was a problem hiding this comment.
Good questions!
- File extension: The extension is always set by us in
captureToFile(either"heif"or"jpg"), so"jpeg"won't occur. But I've changed the check tofileExtension != "heif"which is more robust and format-oriented. - nil quality:
nilmeans no quality override was requested — the original photo data is written as-is without re-encoding. This is the default path for users who never callsetImageQuality. - Float: The int 1–100 scale is consistent with Flutter's
image_pickerplugin (imageQualityparameter) and with Android's nativeCaptureRequest.JPEG_QUALITY(byte 0–100) and CameraX'sImageCapture.Builder.setJpegQuality(int). We convert to float internally on iOS withCGFloat(quality) / 100.0. A cross-platform API should use a single consistent type, and int 1–100 is the natural fit for the Android side.
| guard let quality = self?.imageQuality, quality < 100 else { | ||
| return photo.fileDataRepresentation() | ||
| } | ||
| guard let originalData = photo.fileDataRepresentation() else { |
There was a problem hiding this comment.
it seems fileDataRepresentation is used in both case. can reduce duplicate code by putting it in the guard statement above
There was a problem hiding this comment.
Good catch! Refactored to call fileDataRepresentation() once at the top, then conditionally re-encode. Done in the latest commit.
| guard let originalData = photo.fileDataRepresentation() else { | ||
| return nil | ||
| } | ||
| return Self.reencodeJPEG(data: originalData, quality: quality) |
There was a problem hiding this comment.
is it guaranteed to be JPEG format at this point? it is not clear from reading this chunk of code, without looking at the caller site.
There was a problem hiding this comment.
Yes — the caller (DefaultCamera.captureToFile) only passes a non-nil imageQuality when the format is JPEG (checked via fileExtension != "heif"). I've added an inline comment in the latest commit to make this guarantee visible without needing to check the caller.
There was a problem hiding this comment.
This seems to be quite brittle and could very well surprise us down the road. The imageQuality naming suggest that it could be extended to other formats such as HEIF, as you said previously, but in code we are making the assumption that it must be JPEG.
There was a problem hiding this comment.
Good point about brittleness. I've added a runtime JPEG format guard inside reencodeJPEG itself — it now checks CGImageSourceGetType and returns the original data unchanged if the source isn't JPEG. This way the method is self-contained and safe even if it's called with non-JPEG data in the future.
Also addressed the other two comments in the same commit:
- Added inline comments for all
CGImageDestinationCreateWithDataparameters (thenilis "no additional options") - Added a comment on the
mutableData as Datareturn explaining it contains the finalized JPEG afterCGImageDestinationFinalize
There was a problem hiding this comment.
I'd recommend to move the branching logic to the caller, so that the helper method reencodeJPEG is more focused on just JPEG.
switch type {
case jpeg:
helper that compresses JPEG
case _:
return original image
}
There was a problem hiding this comment.
Makes sense! Removed the format check from reencodeJPEG — it now focuses purely on JPEG re-encoding. The format branching is already handled by the caller (DefaultCamera.captureToFile only passes imageQuality for JPEG captures via the fileFormat == .jpeg check).
There was a problem hiding this comment.
What you did here is in the exact same state discussed in my previous concern (https://github.com/flutter/packages/pull/11155/changes#r2962038676). I was talking about the immediate caller of reencodeJPEG.
There was a problem hiding this comment.
Sorry about that! I misunderstood — you meant the immediate caller of reencodeJPEG, not the higher-level caller.
Fixed: photoOutput now explicitly checks the data format using CGImageSourceGetType before calling reencodeJPEG. Non-JPEG data returns unchanged without entering the re-encoding path at all.
| guard | ||
| let destination = CGImageDestinationCreateWithData( | ||
| mutableData as CFMutableData, | ||
| "public.jpeg" as CFString, |
There was a problem hiding this comment.
why hardcode filename? and also wanna double check jpg vs jpeg since you used both in this PR
There was a problem hiding this comment.
The `"public.jpeg"` string was the UTI for the output format passed to `CGImageDestinationCreateWithData`. I've replaced it with `UTType.jpeg.identifier` in the latest commit which is cleaner.
Regarding `jpg` vs `jpeg` — the only place we use `"jpg"` is the file extension (set by us in `captureToFile`), and `"public.jpeg"` / `UTType.jpeg` is the standard UTI for the JPEG format. These are different things (file extension vs format identifier) so the difference is expected.
| mutableData as CFMutableData, | ||
| "public.jpeg" as CFString, | ||
| 1, | ||
| nil) |
There was a problem hiding this comment.
can you add comment on what these params are
There was a problem hiding this comment.
Added doc comments to the reencodeJPEG method with - Parameters: section explaining both data and quality. Done in the latest commit.
There was a problem hiding this comment.
Sorry i meant params of CGImageDestinationCreateWithData (e.g. what does nil mean?)
There was a problem hiding this comment.
Added inline comments for each parameter. The nil is documented as "no additional options" — Apple's API accepts an optional dictionary for type-specific options, but none are needed for basic JPEG encoding.
There was a problem hiding this comment.
Ah sorry, I misunderstood the first time! Added inline comments for all four CGImageDestinationCreateWithData parameters in the latest commit:
mutableData as CFMutableData— output buffer for the encoded image"public.jpeg" as CFString— UTI for JPEG format1— imageCount: single imagenil— no additional type-specific options (Apple's API accepts an optional dictionary here, but none are needed for basic JPEG encoding)
| } | ||
|
|
||
| var properties = metadata | ||
| properties[kCGImageDestinationLossyCompressionQuality] = CGFloat(quality) / 100.0 |
There was a problem hiding this comment.
it looks like quality can be a random floating point, and it doesn't have to be a multiple of 0.01? If so, a better API would be just to pass in a float, just like this Apple API.
There was a problem hiding this comment.
The int 1–100 scale was chosen to be consistent across all platforms:
- Android Camera2:
CaptureRequest.JPEG_QUALITYuses a byte (0–100) - Android CameraX:
ImageCapture.Builder.setJpegQuality(int)uses int 1–100 - Flutter's own
image_pickerplugin:imageQualityuses int 0–100
Using a float would match the iOS-native API but would be inconsistent with the Android side and existing Flutter conventions. The conversion CGFloat(quality) / 100.0 is straightforward and lossless for all int values in the 1–100 range.
| return data | ||
| } | ||
|
|
||
| return mutableData as Data |
There was a problem hiding this comment.
Any reason why we can't use simpler APIs such as https://developer.apple.com/documentation/uikit/uiimage/jpegdata(compressionquality:)?
There was a problem hiding this comment.
UIImage.jpegData(compressionQuality:) strips all EXIF metadata (GPS coordinates, camera info, orientation, etc.). For a camera plugin, preserving this metadata is important — users expect captured photos to retain location data, orientation, and other properties.
CGImageDestination lets us re-encode the pixels at a lower quality while copying all original metadata properties through CGImageSourceCopyPropertiesAtIndex. I've added a doc comment to reencodeJPEG explaining this rationale.
There was a problem hiding this comment.
This is worth documenting, in case of future cleanup that "simplifies" it into a bad state. Could you add a comment?
There was a problem hiding this comment.
Good call. Added a comment explaining that mutableData contains the finalized JPEG after CGImageDestinationFinalize and the cast to Data is intentional.
There was a problem hiding this comment.
Done! Added a comment above the return explaining that mutableData contains the finalized JPEG after CGImageDestinationFinalize, so future readers won't be tempted to "simplify" it away.
There was a problem hiding this comment.
I'm not sure if this is mentioned anywhere else, but is there a reason we cant set AVCapturePhotoSettings with a jpeg quality. Something like:
let settings = AVCapturePhotoSettings(format: [
AVVideoCodecKey: AVVideoCodecType.jpeg,
AVVideoCompressionPropertiesKey: [
AVVideoQualityKey: 0.9 // quality 0-1
]
])There was a problem hiding this comment.
The re-encoding approach preserves EXIF metadata and works regardless of the capture pipeline format. AVCapturePhotoSettings quality only works when explicitly capturing as JPEG.
There was a problem hiding this comment.
Yes, but isn't the the re-encoding approach unnecessary if the output is already set to jpeg? If someone is setting the jpeg image quality, then I would assume they would want to output a jpeg image anyways.
There was a problem hiding this comment.
Yeah, I guess we can safely assume that now since we also changed the method naming. Will fix!
bparrishMines
added
the
federated: all_changes
| let savePhotoDelegate = SavePhotoDelegate( | ||
| path: path, | ||
| ioQueue: photoIOQueue, | ||
| imageQuality: fileExtension != "heif" && imageQuality < 100 ? imageQuality : nil, |
There was a problem hiding this comment.
hmm, I dislike that imageQuality is doing any arbitrary filtering on the file extension at this layer. Could we maybe it handle it in the init?
There was a problem hiding this comment.
also, can we guarantee its a .heif file at this point? how about .heic files?
There was a problem hiding this comment.
Good points, both of you!
I've changed the check to use the fileFormat enum directly (fileFormat == .jpeg) instead of comparing extension strings. This avoids string-matching and makes the intent clear — and also addresses the .heic concern since we're no longer comparing file extensions at all.
The filtering stays at the caller level rather than moving into SavePhotoDelegate's init because the delegate shouldn't need to know about file formats — it just receives optional quality and re-encodes if set. This keeps SavePhotoDelegate focused on the save/encode responsibility while the caller decides when re-encoding is appropriate.
Regarding .heic — the format decision is made earlier in this method via the fileFormat enum (line 718). The extension is always set by us: either "heif" or "jpg". There are no .heic files in this flow.
camsim99
left a comment
camsim99
left a comment
There was a problem hiding this comment.
Reviewed camera_android and camera_android_camerax. Both LGTM overall, thanks for implementing this feature! Just left a couple of requests concerning a default value and some tests.
|
|
||
| /** Controls the JPEG compression quality on the {@link android.hardware.camera2} API. */ | ||
| public class JpegQualityFeature extends CameraFeature<Integer> { | ||
| private int currentSetting = 100; |
There was a problem hiding this comment.
Could currentSetting reflect the actual current setting by making the default requestBuilder.get(CaptureRequest.JPEG_QUALITY) instead of 100?
There was a problem hiding this comment.
Good question! The feature doesn't have access to a CaptureRequest.Builder at construction time — the builder is only passed to updateBuilder() during capture. Also, the feature is lazily created only when setImageQuality is first called, at which point setValue() is called immediately with the user's requested quality, so the default of 100 is never actually applied to the builder.
That said, 100 as the default makes sense semantically — it means "maximum quality" which is the expected behavior when quality hasn't been explicitly set.
There was a problem hiding this comment.
SGTM, thanks for the clarity :)
...android/src/main/java/io/flutter/plugins/camera/features/jpegquality/JpegQualityFeature.java
Show resolved
Hide resolved
...amera_android_camerax/android/src/test/java/io/flutter/plugins/camerax/ImageCaptureTest.java
Show resolved
Hide resolved
camsim99
left a comment
camsim99
left a comment
There was a problem hiding this comment.
Android implementations LGTM!
|
|
||
| /** Controls the JPEG compression quality on the {@link android.hardware.camera2} API. */ | ||
| public class JpegQualityFeature extends CameraFeature<Integer> { | ||
| private int currentSetting = 100; |
There was a problem hiding this comment.
SGTM, thanks for the clarity :)
LouiseHsu
left a comment
LouiseHsu
left a comment
There was a problem hiding this comment.
lgtm on the ios side! Thanks for making those changes!
|
@Bolling88 looks like you still have some temporary dependency_overrides in the pubspec yamls - once you remove those we can probably merge :) |
| guard let originalData = photo.fileDataRepresentation() else { | ||
| return nil | ||
| } | ||
| return Self.reencodeJPEG(data: originalData, quality: quality) |
There was a problem hiding this comment.
This seems to be quite brittle and could very well surprise us down the road. The imageQuality naming suggest that it could be extended to other formats such as HEIF, as you said previously, but in code we are making the assumption that it must be JPEG.
| mutableData as CFMutableData, | ||
| "public.jpeg" as CFString, | ||
| 1, | ||
| nil) |
There was a problem hiding this comment.
Sorry i meant params of CGImageDestinationCreateWithData (e.g. what does nil mean?)
| return data | ||
| } | ||
|
|
||
| return mutableData as Data |
There was a problem hiding this comment.
This is worth documenting, in case of future cleanup that "simplifies" it into a bad state. Could you add a comment?
- Deduplicate fileDataRepresentation() call in photoOutput - Add comment explaining why CGImageDestination is used over UIImage.jpegData (EXIF metadata preservation) - Add doc comments to reencodeJPEG parameters - Use UTType.jpeg.identifier instead of hardcoded "public.jpeg" string - Use format-based check (fileExtension != "heif") instead of "jpg" string match - Add inline comment clarifying JPEG format guarantee from caller
UniformTypeIdentifiers requires iOS 14+, but camera_avfoundation supports iOS 13+. Use the equivalent "public.jpeg" string literal.
Check fileFormat == .jpeg instead of comparing file extension strings, as suggested in review. This is cleaner and avoids reliance on specific extension values.
- Adds JpegQualityFeatureTest with tests for getDebugName, getValue, setValue, checkIsSupported, and updateBuilder. - Adds CameraX test verifying jpegQuality is set when provided to ImageCapture constructor.
- Add runtime JPEG format verification in reencodeJPEG to guard against future misuse with non-JPEG data - Add inline comments for CGImageDestinationCreateWithData parameters - Document mutableData as Data return to prevent accidental simplification
Per review feedback, reencodeJPEG should focus purely on JPEG re-encoding. The format branching is already handled by the caller (DefaultCamera.captureToFile only sets imageQuality for JPEG captures), so the redundant format check inside reencodeJPEG is removed.
Move the format verification into the immediate caller of reencodeJPEG rather than relying on the implicit contract from captureToFile. Uses CGImageSourceGetType to verify the data is JPEG before re-encoding.
Bolling88
force-pushed
the
fix-set-quality
branch
from
c9ca716 to
9ba9b11
Compare
|
Hi team, thanks for all the reviews and approvals! 🙏 Now that we have the needed approvals, what are the next steps for landing this? As I understand the federated plugin process:
Should I split the platform interface changes into a separate PR for landing first, or is there a preferred way to handle this? Happy to do whatever works best for the team. |
The overall design and cross-platform code still need review by @bparrishMines before this proceeds.
Once that happens, the next steps are to follow the documented process. |
| return data | ||
| } | ||
|
|
||
| return mutableData as Data |
There was a problem hiding this comment.
I'm not sure if this is mentioned anywhere else, but is there a reason we cant set AVCapturePhotoSettings with a jpeg quality. Something like:
let settings = AVCapturePhotoSettings(format: [
AVVideoCodecKey: AVVideoCodecType.jpeg,
AVVideoCompressionPropertiesKey: [
AVVideoQualityKey: 0.9 // quality 0-1
]
])| /// Sets the JPEG compression quality for still image capture. | ||
| /// | ||
| /// The [quality] must be between 1 (lowest) and 100 (highest). | ||
| Future<void> setImageQuality(int cameraId, int quality) { |
There was a problem hiding this comment.
I think this would be a better fit in createCameraWithSettings because we already set video settings there. But it looks like we added setImageFileFormat as a method so this should probably follow the convention.
I worry this method clashes with setImageFileFormat. What happens if another file format is set when this method is called? Would this value be ignored without an error? It looks like all platforms use jpeg and only iOS supports setImageFileFormat currently, but this could change.
I think this could be renamed to setJpegImageQuality and update the doc comments that this does nothing if the file format is not set to jpeg. Same for the method in the app-facing CameraController.
There was a problem hiding this comment.
@Bolling88 Please don't use AI to generate your review responses. More verbose is not better in review discussions.
There was a problem hiding this comment.
Good call — renamed to setJpegImageQuality across all packages. Followed the setImageFileFormat pattern of using a standalone method rather than createCameraWithSettings.
There was a problem hiding this comment.
@Bolling88 Deleting comments that people have already replied to and then posting new variants of them is very disruptive for anyone trying to follow the review. Also, comments can be edited, or at least they can by humans.
Are you using an agent to post review responses directly?
There was a problem hiding this comment.
Ah sorry! I will keep that in mind. I formulated the responses myself, with the help of AI to fix them up, since I am not very articulate, and asked it to edit my existing comments above, but it seems it failed at that. I will make sure not to rely on it when posting new replies or at least editing existing ones.
There was a problem hiding this comment.
and asked it to edit my existing comments above, but it seems it failed at that
Please take a moment to review our new AI contribution guidelines (which were added since this PR was opened). There should always be a human directly in the loop of any AI interaction with a PR.
with the help of AI to fix them up, since I am not very articulate
FWIW, I do not know a single person on the Flutter team who would rather read AI-generated or AI-edited review comments than actual human thoughts as expressed by a human.
Bolling88
force-pushed
the
fix-set-quality
branch
from
d505b56 to
3e38ef0
Compare
bparrishMines
left a comment
bparrishMines
left a comment
There was a problem hiding this comment.
LGTM with just a few nits
I'll add the CICD label
Co-authored-by: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com>
Co-authored-by: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com>
Asserts that the quality value is between 1 and 100 in `setJpegImageQuality` to ensure valid input before invoking the method channel.
Platform interface changes for adding JPEG compression quality control to the camera plugin. This is the platform interface subset of #11155, split out per the federated plugin contribution process. Fixes flutter/flutter#183229 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
camera_platform_interface 2.13.0 is now published with setJpegImageQuality, so path overrides can be dropped. Implementation package overrides remain until those packages are published. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
camera_platform_interface 2.13.0 is now published, so I've dropped the path override for |
6 participants
Adds
setImageQuality(int quality)(1–100 scale) across the camera plugin ecosystem, allowing developers to control JPEG compression quality for still image capture. Users who do not callsetImageQualityretain the platform's native default behavior.Packages changed
camera_platform_interfacecameraCameraController.setImageQualitywith 1–100 range validationcamera_androidJpegQualityFeatureusingCaptureRequest.JPEG_QUALITY(Camera2)camera_android_cameraxImageCaptureviaBuilder.setJpegQuality; preserves locked capture orientationcamera_avfoundationCGImageSource/CGImageDestination(ImageIO), gated on JPEG format onlyPlatform implementation details
JpegQualityFeaturesetsCaptureRequest.JPEG_QUALITYon the capture request builder. Lazily registered so users who never callsetImageQualitykeep the device HAL's native default.setJpegQualityatImageCaptureconstruction time, sosetImageQualityunbinds the currentImageCaptureand recreates it with the requested quality. Locked capture orientation is preserved across recreation.CGImageDestinationwithkCGImageDestinationLossyCompressionQuality, preserving all EXIF metadata from the original capture. HEIF captures are passed through unmodified.Fixes flutter/flutter#183229
Pre-Review Checklist
[shared_preferences]///).Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2