[rust png] Use `SkPngRustEncoder` (if enabled) when encoding `SkImage`s. [chromium/src : main]
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 3 comments![]( "Open in Gerrit")
@fmalita, can you PTAL?
/cc @danieldilan as FYI
SK_API sk_sp<SkData> EncodePngAsSkData(GrDirectContext* context,
const SkImage& src,
int zlibCompressionLevel);The overload that accepts `zlibCompressionLevel` has only 1 caller, so I though that it is useful to have it as a separate overload (e.g. to easily see in code search Xrefs that there is indeed only 1 caller).
nullptr, *params->image, kZLibCompressionLevel);This is the only caller of (`skia/ext/codec_utils.h`) that requires support for custom `SkPngEncoder::Options`. (FWIW `//ui/gfx/codec/png_codec.cc` doesn't go through `skia/ext/codec_utils.h` but also supports options for custom compression levels or for adding comments.)
I note that `SkPngRustEncoder::CompressionLevel::kLow` is not quite the same as `fZlibLevel = 0` in `SkPngEncoder::Options`. But I think this should be fine (at least IMHO it shouldn't block this CL nor the Rust PNG trials). I'll loop the code owner just in case.
I remember that I promised to compression level of Rust `png` vs `libpng` on a randomly chose image - it's still on my TODO list. https://crates.io/crates/png advertises that it "includes a fast encoding mode powered by fdeflate that is dramatically faster than the fastest mode of libpng while simultaneously providing better compression ratio".
Related details
- Florin Malita
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 2 comments![]( "Open in Gerrit")
/cc @lizeb - see below
nullptr, *params->image, kZLibCompressionLevel);This is the only caller of (`skia/ext/codec_utils.h`) that requires support for custom `SkPngEncoder::Options`. (FWIW `//ui/gfx/codec/png_codec.cc` doesn't go through `skia/ext/codec_utils.h` but also supports options for custom compression levels or for adding comments.)
I note that `SkPngRustEncoder::CompressionLevel::kLow` is not quite the same as `fZlibLevel = 0` in `SkPngEncoder::Options`. But I think this should be fine (at least IMHO it shouldn't block this CL nor the Rust PNG trials). I'll loop the code owner just in case.
I remember that I promised to compression level of Rust `png` vs `libpng` on a randomly chose image - it's still on my TODO list. https://crates.io/crates/png advertises that it "includes a fast encoding mode powered by fdeflate that is dramatically faster than the fastest mode of libpng while simultaneously providing better compression ratio".
/cc @lizeb who authored this `case` in https://crrev.com/c/5645862 in July 2024.
I think the only next steps would be to watch the histograms from the CanvasHibernationSnapshotZstd.gcl study - I'll add the to the design doc at go/rusty-png-in-chromium-and-skia (done) and to the `RustyPng.gcl` study (see cl/705961197):
- 'Blink.Canvas.2DLayerBridge.Compression.DecompressionTime',
- 'Blink.Canvas.2DLayerBridge.Compression.Ratio',
- 'Blink.Canvas.2DLayerBridge.Compression.ThreadTime',
(I see that the histograms won't expire until May 2025.)
Please let me know if you have any feedback.
Related details
- Florin Malita
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florin Malita (Gerrit)
Florin Malita added 3 comments![]( "Open in Gerrit")
SK_API sk_sp<SkData> EncodePngAsSkData(GrDirectContext* context,
const SkImage& src,
int zlibCompressionLevel);The overload that accepts `zlibCompressionLevel` has only 1 caller, so I though that it is useful to have it as a separate overload (e.g. to easily see in code search Xrefs that there is indeed only 1 caller).
Acknowledged
const SkImage& src,Would it make more sense to take a `const SkImage*` instead, and add a null check here? I think that would eliminate some of the defensive checks in client code, and is also more aligned with `SkPngCoder::Encode`.
SkPngEncoder::Options options = {};
options.fZLibLevel = zlibCompressionLevel;nit: can we use c++20 designated initializers yet? I'm dying to write something like this :")
`const SkPngEncoder::Options options = { .fZlibLevel = zlibCompressionLevel };`
Related details
- Łukasz Anforowicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 4 comments![]( "Open in Gerrit")
Would it make more sense to take a `const SkImage*` instead, and add a null check here? I think that would eliminate some of the defensive checks in client code, and is also more aligned with `SkPngCoder::Encode`.
Done
SkPngEncoder::Options options = {};
options.fZLibLevel = zlibCompressionLevel;nit: can we use c++20 designated initializers yet? I'm dying to write something like this :")
`const SkPngEncoder::Options options = { .fZlibLevel = zlibCompressionLevel };`
Done.
FWIW https://en.cppreference.com/w/cpp/language/aggregate_initialization#Implicitly_initialized_elements says that
1. If the element has a default member initializer, the element is initialized from that initializer.
(since C++11)
2. Otherwise, if the element is not a reference, the element is copy-initialized from an empty initializer list.
Item 1 applies to `fFilterFlags` and `fICCProfile` and `fICCProfileDescription`. I _think_ that item 2 applies to `fComments` (i.e. that it will invoke the default constructor of `sk_sp<...>` and the copy that into `fComments`).
nullptr, *params->image, kZLibCompressionLevel);Łukasz AnforowiczThis is the only caller of (`skia/ext/codec_utils.h`) that requires support for custom `SkPngEncoder::Options`. (FWIW `//ui/gfx/codec/png_codec.cc` doesn't go through `skia/ext/codec_utils.h` but also supports options for custom compression levels or for adding comments.)
I note that `SkPngRustEncoder::CompressionLevel::kLow` is not quite the same as `fZlibLevel = 0` in `SkPngEncoder::Options`. But I think this should be fine (at least IMHO it shouldn't block this CL nor the Rust PNG trials). I'll loop the code owner just in case.
I remember that I promised to compression level of Rust `png` vs `libpng` on a randomly chose image - it's still on my TODO list. https://crates.io/crates/png advertises that it "includes a fast encoding mode powered by fdeflate that is dramatically faster than the fastest mode of libpng while simultaneously providing better compression ratio".
/cc @lizeb who authored this `case` in https://crrev.com/c/5645862 in July 2024.
I think the only next steps would be to watch the histograms from the CanvasHibernationSnapshotZstd.gcl study - I'll add the to the design doc at go/rusty-png-in-chromium-and-skia (done) and to the `RustyPng.gcl` study (see cl/705961197):
- 'Blink.Canvas.2DLayerBridge.Compression.DecompressionTime',
- 'Blink.Canvas.2DLayerBridge.Compression.Ratio',
- 'Blink.Canvas.2DLayerBridge.Compression.ThreadTime',
(I see that the histograms won't expire until May 2025.)
Please let me know if you have any feedback.
@lizeb, maybe one other thing to consider would be to keep [the `kCanvasHibernationSnapshotZstd` feature](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/canvas_hibernation_handler.cc;l=47-52;drc=2d2d0f30da5d041405838dbad97f02c146724b2c) and then (once Rusty PNG reaches Stable) we could do a 2x2 comparison:
```
| regular encode | zstd
----------+----------------+------
libpng | |
Rusty PNG | |
```
WDYT?
Related details
- Florin Malita
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florin Malita (Gerrit)
Florin Malita voted Code-Review+1![]( "Open in Gerrit")
- Łukasz Anforowicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 1 comment![]( "Open in Gerrit")
+@xiyuan from chrome/browser/ash/app_list/OWNERS - can you PTAL at the changes in this directory?
Related details
- Xiyuan Xia
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 1 comment![]( "Open in Gerrit")
+@fredmello from components/paint_preview/OWNERS - can you PTAL at the changes in this directory?
Related details
- Fred Mello
- Xiyuan Xia
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 1 comment![]( "Open in Gerrit")
+@rbpotter from //printing/OWNERS - can you PTAL at the changes in this directory?
Related details
- Fred Mello
- Rebekah Potter
- Xiyuan Xia
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 1 comment![]( "Open in Gerrit")
+@dcheng from content/renderer/OWNERS - can you PTAL at the changes in this directory? (I asked a random number generator for a number from 1 to 3, and it said that I should send the CL to the 1st person from the OWNERS file :-)
Related details
- Daniel Cheng
- Fred Mello
- Rebekah Potter
- Xiyuan Xia
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
1 comment
- Fred Mello
- Rebekah Potter
- Xiyuan Xia
- Łukasz Anforowicz
Xiyuan Xia (Gerrit)
Xiyuan Xia voted and added 2 comments![]( "Open in Gerrit")
Votes added by Xiyuan Xia
| Code-Review | +1 |
2 comments
lgtm
int zlibCompressionLevel);nit: `zlibCompressionLevel` -> `zlib_compression_level` ?
Related details
- Fred Mello
- Rebekah Potter
- Łukasz Anforowicz
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 1 comment![]( "Open in Gerrit")
nit: `zlibCompressionLevel` -> `zlib_compression_level` ?
Ooops. Done. My brain has significant inertia when switching between Skia and Chromium style guides :-P.
Related details
- Fred Mello
- Rebekah Potter
Rebekah Potter (Gerrit)
Rebekah Potter added 1 comment![]( "Open in Gerrit")
Redirecting to thestig@ for printing backend changes.
Related details
- Fred Mello
- Lei Zhang
- Łukasz Anforowicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 2 comments![]( "Open in Gerrit")
Redirecting to thestig@ for printing backend changes.
Redirecting to thestig@ for printing backend changes.
IIUC thestig@ won't be able to take a look until Dec 19th... Let me try @awscreen in the meantime. FWIW this is a more-or-less mechanical change, so it should be fairly straightforward to review.
@awscreen, can you PTAL at the //printing/common/metafile_utils.cc changes?
Related details
- Alan Screen
- Fred Mello
- Lei Zhang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Screen (Gerrit)
Alan Screen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alan Screen
| Code-Review | +1 |
1 comment
- Fred Mello
- Lei Zhang
- Łukasz Anforowicz
Calder Kitagawa (Gerrit)
Calder Kitagawa voted and added 1 comment![]( "Open in Gerrit")
Votes added by Calder Kitagawa
| Code-Review | +1 |
1 comment
- Fred Mello
- Łukasz Anforowicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Łukasz Anforowicz
| Commit-Queue | +2 |
2 comments
Thanks for the reviews everyone! @lizeb - I'll resolve the comment thread below and push the CL to CQ. I'll follow-up separately over email.
nullptr, *params->image, kZLibCompressionLevel);Łukasz AnforowiczThis is the only caller of (`skia/ext/codec_utils.h`) that requires support for custom `SkPngEncoder::Options`. (FWIW `//ui/gfx/codec/png_codec.cc` doesn't go through `skia/ext/codec_utils.h` but also supports options for custom compression levels or for adding comments.)
I note that `SkPngRustEncoder::CompressionLevel::kLow` is not quite the same as `fZlibLevel = 0` in `SkPngEncoder::Options`. But I think this should be fine (at least IMHO it shouldn't block this CL nor the Rust PNG trials). I'll loop the code owner just in case.
I remember that I promised to compression level of Rust `png` vs `libpng` on a randomly chose image - it's still on my TODO list. https://crates.io/crates/png advertises that it "includes a fast encoding mode powered by fdeflate that is dramatically faster than the fastest mode of libpng while simultaneously providing better compression ratio".
Łukasz Anforowicz/cc @lizeb who authored this `case` in https://crrev.com/c/5645862 in July 2024.
I think the only next steps would be to watch the histograms from the CanvasHibernationSnapshotZstd.gcl study - I'll add the to the design doc at go/rusty-png-in-chromium-and-skia (done) and to the `RustyPng.gcl` study (see cl/705961197):
- 'Blink.Canvas.2DLayerBridge.Compression.DecompressionTime',
- 'Blink.Canvas.2DLayerBridge.Compression.Ratio',
- 'Blink.Canvas.2DLayerBridge.Compression.ThreadTime',
(I see that the histograms won't expire until May 2025.)
Please let me know if you have any feedback.
@lizeb, maybe one other thing to consider would be to keep [the `kCanvasHibernationSnapshotZstd` feature](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/canvas_hibernation_handler.cc;l=47-52;drc=2d2d0f30da5d041405838dbad97f02c146724b2c) and then (once Rusty PNG reaches Stable) we could do a 2x2 comparison:
```
| regular encode | zstd
----------+----------------+------
libpng | |
Rusty PNG | |
```WDYT?
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[rust png] Use `SkPngRustEncoder` (if enabled) when encoding `SkImage`s.
This CL reroutes direct calls to `SkPngEncoder::Encode(...image...)` to
instead go through helper functions that offer the same functionality,
but select `SkPngEncoder` vs `SkPngRustEncoder` at runtime, based on the
`skia::kRustyPngFeature`.
- M cc/debug/picture_debug_util.cc
- M chrome/browser/ash/app_list/app_list_sort_browsertest.cc
- M components/paint_preview/common/serial_utils.cc
- M components/paint_preview/renderer/paint_preview_recorder_utils_unittest.cc
- M content/renderer/gpu_benchmarking_extension.cc
- M printing/common/metafile_utils.cc
- M skia/ext/codec_utils.cc
- M skia/ext/codec_utils.h
- M skia/ext/codec_utils_unittest.cc
- M third_party/blink/renderer/platform/graphics/canvas_hibernation_handler.cc
Code-Review: +1 by Xiyuan Xia, +1 by Florin Malita, +1 by Calder Kitagawa, +1 by Daniel Cheng, +1 by Alan Screen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |