Add explicit color management.#2323
Conversation
Color management was introduced in Android O. Currently Fresco allows applications to specify whether a decoded bitmap should be transformed to SRGB color space or not at load time, and legacy APIs pass in false, meaning if applications don't use newer APIs, the decoded bitmap will have the encoded color space as the final color space if they have one. This doesn't not align with the world before Android O because before Android O, everything is essentially treated as SRGB inside the rendering pipeline no matter whether the images come with a color space. This patch, 1) Modifies the newer APIs to accept ColorSpace instead of a boolean, a more future-proof solution as mobile display technology evolves; 2) Updates legacy APIs to pass in null, meaning on newer devices (API level >= 26), applications that haven't managed color will always get the decoded bitmap in SRGB color space. Issue: facebook#2318
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
oprisnik
left a comment
oprisnik
left a comment
There was a problem hiding this comment.
Thank you for the pull request. Overall, this looks great. I've added some comments & a question but the change is almost good to go.
Please also sign the CLA :)
| bitmapConfig); | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| options.inPreferredColorSpace = colorSpace == null | ||
| ? ColorSpace.get(ColorSpace.Named.SRGB) : colorSpace; |
There was a problem hiding this comment.
As you've mentioned, this changes the default behavior for all images >= 26 to force SRGB instead of leaving inPreferredColorSpace at the default null value. However, this change also means that with this API it would not be possible to set inPreferredColorSpace=null.
I'm not sure if somebody would ever want to have this behavior but if we think that somebody might want this, it would be possible to just set the default ColorSpace in ImageDecodeOptions to SRGB and developers can then override it to null, which would enable the inPreferredColorSpace=null mode.
Existing calls that currently pass in null / false since they don't support color spaces could be changed to also pass in SRGB to be consistent.
I can't really think of a good example where this might be needed though, so this version here should be fine as well. What do you think?
There was a problem hiding this comment.
There's no benefit I can think of to get such behaviour in my humble opinion. One must be very sure about the encoded color space of the contents in order to be confident to skip this bit while also maintain color management in the pipeline. But in that case, the color space is already known in advance, and passing a ColorSpace would make the code more readable as well.
| * @param transformToSRGB whether to allow the color space to be transformed to sRGB | ||
| * @return this builder | ||
| */ | ||
| public ImageDecodeOptionsBuilder setTransformToSRGB(boolean transformToSRGB) { |
There was a problem hiding this comment.
In order to not break the API, we could deprecate this method and just set the color space accordingly (if we decide to support the null variant. Otherwise, this will be a no-op I suppose and we can just remove it.
There was a problem hiding this comment.
To make sure this library provides benefit of helping manage color, I would suggest removing this, and don't support the null variant.
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
Thanks for the feedback, I resolved the last comment and replied the first two comments. May I ask how to run the tests locally? Android Studio doesn't seem to allow me to run the tests. :) Also, the |
|
Yeah, the build was broken (I fixed it with 28de385) and our CI is also still acting up. Using Gradle / Android Studio usually works for test. I'll import your pull request and do some internal testing. Thanks again! |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@oprisnik has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Should I wait for your feedback? May I ask help for CI rerun? I am going to squash into one commit to avoid commit pollution as well. |
|
Ipy, don't worry about merging the commits. It will automatically be merged under the PR request including the summary when we land this internally. When landing we'll also be auto-formatting the change to reflect our code style. This includes flipping some annotations as our tools assume |
4 participants
Color management was introduced in Android O. Currently Fresco allows
applications to specify whether a decoded bitmap should be transformed to SRGB
color space or not at load time, and legacy APIs pass in false, meaning if
applications don't use newer APIs, the decoded bitmap will have the encoded
color space as the final color space if they have one. This doesn't not align
with the world before Android O because before Android O, everything is
essentially treated as SRGB inside the rendering pipeline no matter whether the
images come with a color space.
This patch, 1) Modifies the newer APIs to accept ColorSpace instead of a
boolean, a more future-proof solution as mobile display technology evolves; 2)
Updates legacy APIs to pass in null, meaning on newer devices (API level >=
26), applications that haven't managed color will always get the decoded bitmap
in SRGB color space.
Issue: #2318