search

image-rs / image-png

PNG decoding and encoding library in pure Rust
https://docs.rs/png
Apache License 2.0
347 stars 139 forks source link

End-to-end decoding benchmarks of paletted PNG images. #453

Closed anforowicz closed 5 months ago

anforowicz commented 5 months ago

@fintelia, can you PTAL?

Special thanks to @Shnatsel for pointing out the mistake I made in my earlier attempt to cover expand_paletted in end-to-end benchmarks. Thanks!

fintelia commented 5 months ago

Could we use a smaller test image? 8 MB is pretty large for a file checked into the git repo

Shnatsel commented 5 months ago

FWIW the larger the decompressed image is, the more the palette expansion pass dominates the runtime. On 1024x1024 images it's about 20% of the time, on the map image it's about 40%, on the zune-png benchmarking image shared at the start of #393 it's about 60%.

I think it is possible to create an image e.g. with large areas of flat fill that would be small when compressed, decompress into an image with large dimensions, and have the palette pass account for a large portion of the image decoding time.

Or maybe the palette pass should just be benchmarked separately.

Shnatsel commented 5 months ago

I suggest using this image instead: Stadt_Onex_2021_posterized_med

It still spends 40% of the time in palette expansion while only being 1.4MB in size (as opposed to the 8MB image proposed originally).

anforowicz commented 5 months ago

FWIW, I feel that it is desirable to have a somewhat realistic paletted test image that also highlights the performance of expand_paletted. FWIW using palette reduces the size of the image from 9,408,318 bytes to 8,396,692 (and so it seems reasonable that this optimization would be used in the "real world" + shows that the original "real world" image is even bigger).

OTOH, if we decide that 8MB is unreasonable, then we can try testing with existing benchmarks and a function-level benchmark that I am working on at https://github.com/anforowicz/image-png/tree/palette-benchmarks-func2. We would still want to land the current PR to ensure that expand_paletted is actuall covered by the benchmarks. (I think that other set of changes is ready for review, but I'll hold off with opening a pull request until we've figure out what to do with the current one under review. This is mostly because the performance measurements in the other one use the current PR as the baseline.)

Edit: Ooops... it seems that I missed @Shnatsel's reply above, before posting my comment... :-/

anforowicz commented 5 months ago

I suggest using this image instead: Stadt_Onex_2021_posterized_med

It still spends 40% of the time in palette expansion while only being 1.4MB in size (as opposed to the 8MB image proposed originally).

Thanks - let me switch to this image.

anforowicz commented 5 months ago

I suggest using this image instead: Stadt_Onex_2021_posterized_med It still spends 40% of the time in palette expansion while only being 1.4MB in size (as opposed to the 8MB image proposed originally).

Thanks - let me switch to this image.

Actually, I wasn't able to find the image that you are referring to. But maybe this is fine - I just grabbed https://commons.wikimedia.org/wiki/File:Stadt_Onex_2021.png at a slightly smaller resolution and converted to png:IHDR.color_type: 3 (Indexed) using ImageMagick. The resulting file has 1,146,578 bytes and png::decoder::expand_paletted takes 22.84% of time.

anforowicz commented 5 months ago

@fintelia, can you PTAL again?