Skip to content

Commit

Permalink
[rust png] Integrate cICP support into SkPngRustCodec.
Browse files Browse the repository at this point in the history
This CL propagates the data from the `cICP` chunk into
`SkEncodedInfo::ICCProfile`.

Dependencies:

* This CL unblocks enabling the `PNGTests.cicp` test in the Chromium CL
  at https://crrev.com/c/6013356.
* This CL depends on the new `SkColorSpace::MakeCICP` API that was
  recently added in https://crrev.com/c/5999738.

This CL patches the `png` crate in a way that corresponds to the
following upstream PRs (rolling this CL into Chromium depends on
https://crrev.com/c/5999738 which included the same patches):

* image-rs/image-png#526 - baseline to make
  subsequent patches easier to apply
* image-rs/image-png#528 - `mDCv` and `cLLI`
* image-rs/image-png#529 - `cICP`

Bug: chromium:376758571
Change-Id: I1e550b1eba4d629ff4337d85daa5656ac03f3a5a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/917216
Commit-Queue: Łukasz Anforowicz <[email protected]>
Reviewed-by: Daniel Dilan <[email protected]>
  • Loading branch information
anforowicz authored and SkCQ committed Nov 21, 2024
1 parent b33556c commit 608f252
Show file tree
Hide file tree
Showing 9 changed files with 573 additions and 3 deletions.
3 changes: 3 additions & 0 deletions WORKSPACE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,9 @@ crates_repository(
"@//experimental/rust_png/patches:0105-Fix-a-subset-of-issues-identified-by-cargo-clippy.patch",
"@//experimental/rust_png/patches:0106-Avoid-infinite-loop-when-retrying-after-earlier-fata.patch",
"@//experimental/rust_png/patches:0107-New-API-Reader.next_frame_control-for-advancing-to-t.patch",
"@//experimental/rust_png/patches:0201-Add-EXIF-and-ICC-encoding-and-fix-chunk-order.patch",
"@//experimental/rust_png/patches:0301-Add-support-for-parsing-mDCv-and-cLLi-chunks.-528.patch",
"@//experimental/rust_png/patches:0302-Add-support-for-parsing-cICP-chunks.-529.patch",
],
)],
},
Expand Down
6 changes: 3 additions & 3 deletions experimental/rust_png/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,6 @@ TODO(https://crbug.com/356875275): Add support for running older tests
* `SkPngCodec`:
- No APNG support.
* `SkPngRustCodec`:
- No support to decode a `fSubset` of pixels (https://crbug.com/362830091).
- `sBIT` chunk is ignored (https://crbug.com/359279096)
- No CICP support.
* `SkPngRustCodec` differences - see
https://issues.chromium.org/issues?q=parentid:362829876%2B):
29 changes: 29 additions & 0 deletions experimental/rust_png/ffi/FFI.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,13 @@ mod ffi {
bx: &mut f32,
by: &mut f32,
) -> bool;
fn try_get_cicp_chunk(
self: &Reader,
primaries_id: &mut u8,
transfer_id: &mut u8,
matrix_id: &mut u8,
is_full_range: &mut bool,
) -> bool;
fn try_get_gama(self: &Reader, gamma: &mut f32) -> bool;
fn has_iccp_chunk(self: &Reader) -> bool;
fn get_iccp_chunk(self: &Reader) -> &[u8];
Expand Down Expand Up @@ -316,6 +323,28 @@ impl Reader {
}
}

/// If the decoded PNG image contained a `cICP` chunk then
/// `try_get_cicp_chunk` returns `true` and populates the out
/// parameters. Otherwise, returns `false`.
fn try_get_cicp_chunk(
&self,
primaries_id: &mut u8,
transfer_id: &mut u8,
matrix_id: &mut u8,
is_full_range: &mut bool,
) -> bool {
match self.reader.info().coding_independent_code_points.as_ref() {
None => false,
Some(cicp) => {
*primaries_id = cicp.color_primaries;
*transfer_id = cicp.transfer_function;
*matrix_id = cicp.matrix_coefficients;
*is_full_range = cicp.is_video_full_range_image;
true
}
}
}

/// If the decoded PNG image contained a `gAMA` chunk then `try_get_gama`
/// returns `true` and populates the `gamma` out parameter. Otherwise,
/// returns `false`.
Expand Down
28 changes: 28 additions & 0 deletions experimental/rust_png/impl/SkPngRustCodec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <utility>

#include "experimental/rust_png/ffi/FFI.rs.h"
#include "include/core/SkColorSpace.h"
#include "include/core/SkStream.h"
#include "include/private/SkEncodedInfo.h"
#include "include/private/base/SkAssert.h"
Expand Down Expand Up @@ -97,6 +98,33 @@ std::unique_ptr<SkEncodedInfo::ICCProfile> CreateColorProfile(const rust_png::Re
// `src/codec/SkPngCodec.cpp` but has been refactored to use Rust inputs
// instead of `libpng`.

// Considering the `cICP` chunk first, because the spec at
// https://www.w3.org/TR/png-3/#cICP-chunk says: "This chunk, if understood
// by the decoder, is the highest-precedence color chunk."
uint8_t cicpPrimariesId = 0;
uint8_t cicpTransferId = 0;
uint8_t cicpMatrixId = 0;
bool cicpIsFullRange = false;
if (reader.try_get_cicp_chunk(cicpPrimariesId, cicpTransferId, cicpMatrixId, cicpIsFullRange)) {
// https://www.w3.org/TR/png-3/#cICP-chunk says "RGB is currently the
// only supported color model in PNG, and as such Matrix Coefficients
// shall be set to 0."
//
// According to SkColorSpace::MakeCICP narrow range images are rare and
// therefore not supported.
if (cicpMatrixId == 0 && cicpIsFullRange) {
sk_sp<SkColorSpace> colorSpace =
SkColorSpace::MakeCICP(static_cast<SkNamedPrimaries::CicpId>(cicpPrimariesId),
static_cast<SkNamedTransferFn::CicpId>(cicpTransferId));
if (colorSpace) {
skcms_ICCProfile colorProfile;
skcms_Init(&colorProfile);
colorSpace->toProfile(&colorProfile);
return SkEncodedInfo::ICCProfile::Make(colorProfile);
}
}
}

if (reader.has_iccp_chunk()) {
// `SkData::MakeWithCopy` is resilient against 0-sized inputs, so
// no need to check `rust_slice.empty()` here.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
From 6bbdb506da254033aaa60975851e5fae2a7b4c42 Mon Sep 17 00:00:00 2001
From: Jonathan Behrens <[email protected]>
Date: Sun, 20 Oct 2024 12:03:53 -0700
Subject: [PATCH] Add EXIF and ICC encoding and fix chunk order

---
src/chunk.rs | 2 ++
src/common.rs | 30 ++++++++++++++++++++++--------
2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/chunk.rs b/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/chunk.rs
index 39578a4..3908313 100644
--- a/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/chunk.rs
+++ b/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/chunk.rs
@@ -35,6 +35,8 @@ pub const gAMA: ChunkType = ChunkType(*b"gAMA");
pub const sRGB: ChunkType = ChunkType(*b"sRGB");
/// ICC profile chunk
pub const iCCP: ChunkType = ChunkType(*b"iCCP");
+/// EXIF metadata chunk
+pub const eXIf: ChunkType = ChunkType(*b"eXIf");
/// Latin-1 uncompressed textual data
pub const tEXt: ChunkType = ChunkType(*b"tEXt");
/// Latin-1 compressed textual data
diff --git a/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/common.rs b/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/common.rs
index a3b6c27..259a2b1 100644
--- a/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/common.rs
+++ b/third_party/rust/chromium_crates_io/vendor/png-0.17.14/src/common.rs
@@ -507,6 +507,8 @@ pub struct Info<'a> {
pub srgb: Option<SrgbRenderingIntent>,
/// The ICC profile for the image.
pub icc_profile: Option<Cow<'a, [u8]>>,
+ /// The EXIF metadata for the image.
+ pub exif_metadata: Option<Cow<'a, [u8]>>,
/// tEXt field
pub uncompressed_latin1_text: Vec<TEXtChunk>,
/// zTXt field
@@ -537,6 +539,7 @@ impl Default for Info<'_> {
source_chromaticities: None,
srgb: None,
icc_profile: None,
+ exif_metadata: None,
uncompressed_latin1_text: Vec::new(),
compressed_latin1_text: Vec::new(),
utf8_text: Vec::new(),
@@ -631,6 +634,7 @@ impl Info<'_> {
data[9] = self.color_type as u8;
data[12] = self.interlaced as u8;
encoder::write_chunk(&mut w, chunk::IHDR, &data)?;
+
// Encode the pHYs chunk
if let Some(pd) = self.pixel_dims {
let mut phys_data = [0; 9];
@@ -643,14 +647,6 @@ impl Info<'_> {
encoder::write_chunk(&mut w, chunk::pHYs, &phys_data)?;
}

- if let Some(p) = &self.palette {
- encoder::write_chunk(&mut w, chunk::PLTE, p)?;
- };
-
- if let Some(t) = &self.trns {
- encoder::write_chunk(&mut w, chunk::tRNS, t)?;
- }
-
// If specified, the sRGB information overrides the source gamma and chromaticities.
if let Some(srgb) = &self.srgb {
let gamma = crate::srgb::substitute_gamma();
@@ -665,11 +661,29 @@ impl Info<'_> {
if let Some(chrms) = self.source_chromaticities {
chrms.encode(&mut w)?;
}
+ if let Some(iccp) = &self.icc_profile {
+ encoder::write_chunk(&mut w, chunk::iCCP, iccp)?;
+ }
}
+
+ if let Some(exif) = &self.exif_metadata {
+ encoder::write_chunk(&mut w, chunk::eXIf, exif)?;
+ }
+
if let Some(actl) = self.animation_control {
actl.encode(&mut w)?;
}

+ // The position of the PLTE chunk is important, it must come before the tRNS chunk and after
+ // many of the other metadata chunks.
+ if let Some(p) = &self.palette {
+ encoder::write_chunk(&mut w, chunk::PLTE, p)?;
+ };
+
+ if let Some(t) = &self.trns {
+ encoder::write_chunk(&mut w, chunk::tRNS, t)?;
+ }
+
for text_chunk in &self.uncompressed_latin1_text {
text_chunk.encode(&mut w)?;
}
--
2.47.0.199.ga7371fff76-goog

Loading

0 comments on commit 608f252

Please sign in to comment.