V9 Change log and read me #559

andrews05 · 2023-09-27T01:17:19Z

I haven't updated the actual read me yet, only the template. I'm unsure what we should do about the benchmarks. I don't have a machine with more than 4 primary cores, which I don't think makes a good benchmark.
Do you think we still need them? It currently only shows speed difference vs OptiPNG, though I think compression is equally notable.
We could always drop them for now and add them back some other time.

AlexTMjugador · 2023-09-28T11:37:47Z

I'm unsure what we should do about the benchmarks. I don't have a machine with more than 4 primary cores, which I don't think makes a good benchmark. Do you think we still need them? It currently only shows speed difference vs OptiPNG, though I think compression is equally notable.

Since there are many PNG optimizers out there, and the main deciding factors between them are speed and compression, I think it is important to keep the benchmarks up to date, even if only comparing performance with OptiPNG is somewhat limited. I could generate some updated numbers on an Intel Core i7-12700 CPU (20 logical cores, 8 performance cores) if that's okay with you!

andrews05 · 2023-09-28T19:45:20Z

Yes, that'd be great! I might see if I can update the script to simplify the way it works with the read me...

Did you have any comments on the change log or read me?

andrews05 · 2023-09-28T20:33:53Z

Right, I've updated the compare script with a couple of key changes:

It operates on the actual README file, so we don't need the separate template.
It no longer requires strip-ansi, so we don't need the node package files.

ace-dent · 2023-09-29T11:39:13Z

README.template.md

@@ -41,28 +41,25 @@ Oxipng follows Semantic Versioning.
 Oxipng is a command-line utility. Basic usage looks similar to the following:

 ```
-oxipng -o 4 -i 1 --strip safe *.png
+oxipng -o 4 --alpha --strip safe *.png


Please don't suggest --alpha for newbies. It should not be the default. I recently got caught out on some specialist pre-multiplied alpha case, where this caused a breaking change...

Don't worry, it's not the default - this is just an example 🙂
For web or personal use (the majority of use cases), I do think it's a good suggestion. PNG isn't technically supposed to be pre-multiplied (according to the specs) and situations where alpha optimisation is breaking are indeed specialist as you say.

I've changed the wording to say it's an example for web usage.

README.template.md

AlexTMjugador · 2023-09-29T19:27:12Z

Did you have any comments on the change log or read me?

Other than what @ace-dent suggested, I think the documents are fine 👍

I'll try to do some benchmarks soon, although it's likely I'll be busy these next days.

XhmikosR · 2023-10-02T18:21:22Z

README.md

 - Strip: Used to remove metadata info from processed images. Used via `--strip [safe,all]`.
 Can save a few kilobytes if you don't need the metadata. "Safe" removes only metadata that
 will never affect rendering of the image. "All" removes all metadata that is not critical
 to the image. You can also pass a comma-separated list of specific metadata chunks to remove.
 `-s` can be used as a shorthand for `--strip safe`.

-More advanced options can be found by running `oxipng -h`.
+More advanced options can be found by running `oxipng -h`, or viewed [here](MANUAL.txt).


Not related to this line, but I think the deploy workflow needs to include this file too maybe?

https://github.com/shssoichiro/oxipng/blob/master/.github/workflows/deploy.yml#L57

Good point, I'll mention that in #563.

Please add a note:

Comment on parameters being case sensitive.

Example of short vs long parameter usage, e.g. -Psavvo6 == --pretend --strip=safe --alpha --verbose --verbose --opt=6.
This fixes Tolerate lowercase -z parameter #539.

Also suggest pointing user to --help (not -h), for extended help.

@ace-dent done

AlexTMjugador · 2023-10-09T20:49:42Z

I have just ran the benchmarks on a computer with a Debian sid Linux 6.5.0-2-amd64 kernel, an Intel Core i7-12700 CPU (12C 20T; stock clocks and cooler, but with turbo duration and power limits unlimited by motherboard firmware), and DDR5-5200 RAM in dual channel configuration. I used OptiPNG 0.7.7 as packaged by Debian. I compiled OxiPNG by running cargo build --release on this repository at commit c16519b38b0519988db625913be919d4f0e42f5d, using rustc 1.74.0-nightly (7b4d9e155 2023-09-28). No desktop environment or non-essential background processes were running during the benchmark.

The results were as follows:

Benchmark 1: ./target/release/oxipng -P ./tests/files/rgb_16_should_be_grayscale_8.png
  Time (mean ± σ):      59.6 ms ±   7.7 ms    [User: 77.4 ms, System: 3.6 ms]
  Range (min … max):    53.3 ms …  89.9 ms    32 runs
 
Benchmark 2: optipng -simulate ./tests/files/rgb_16_should_be_grayscale_8.png
  Time (mean ± σ):     132.4 ms ±   0.8 ms    [User: 132.5 ms, System: 0.6 ms]
  Range (min … max):   131.8 ms … 134.4 ms    22 runs
 
Summary
  ./target/release/oxipng -P ./tests/files/rgb_16_should_be_grayscale_8.png ran
    2.22 ± 0.29 times faster than optipng -simulate ./tests/files/rgb_16_should_be_grayscale_8.png

Benchmark 1: ./target/release/oxipng -o4 -P ./tests/files/rgb_16_should_be_grayscale_8.png
  Time (mean ± σ):      88.7 ms ±   4.3 ms    [User: 270.3 ms, System: 11.0 ms]
  Range (min … max):    86.8 ms … 109.4 ms    26 runs
 
Benchmark 2: optipng -o 4 -simulate ./tests/files/rgb_16_should_be_grayscale_8.png
  Time (mean ± σ):     444.9 ms ±   0.3 ms    [User: 444.8 ms, System: 0.7 ms]
  Range (min … max):   444.4 ms … 445.6 ms    10 runs
 
Summary
  ./target/release/oxipng -o4 -P ./tests/files/rgb_16_should_be_grayscale_8.png ran
    5.01 ± 0.25 times faster than optipng -o 4 -simulate ./tests/files/rgb_16_should_be_grayscale_8.png

In general, the relative performance of OxiPNG over OptiPNG seems to have remained similar over time.

andrews05 · 2023-10-09T20:58:25Z

Cool, thanks!
The outcome is expected - I've tried to keep the performance curve roughly the same in all the big changes I've made.
Do you want to commit the updated Read Me?

AlexTMjugador · 2023-10-09T21:10:30Z

Yeah, I'll push a commit here with the updated readme!

andrews05 · 2023-10-09T21:23:10Z

@AlexTMjugador The compare.sh command should update the Read Me itself, did you not use that?

AlexTMjugador · 2023-10-09T21:39:38Z

@AlexTMjugador The compare.sh command should update the Read Me itself, did you not use that?

I used it, but I didn't like how it formatted my CPU name as "12th Gen Intel(R) Core(TM) i7-12700" and did not make distinctions about the number of performance and efficiency cores, so I rewrote that a bit to look nicer in my opinion.

andrews05 · 2023-10-09T22:52:01Z

Ah, fair enough. Do we really need to keep the old benchmarks though?

AlexTMjugador · 2023-10-10T12:54:42Z

Do we really need to keep the old benchmarks though?

I think they are useful for historical purposes. There is no harm in keeping them around for now.

ace-dent · 2023-10-10T14:01:45Z

My 2¢: Old benchmarks have zero value to users of the contemporary release. Even the latest benchmarks are just indicative of performance.
If dev's need the info for some kind of performance analysis(?), it can be seen in git or retested easily.

There is no harm in keeping them ...

Adds more noise for a new user to scan through when first encountering oxipng.

AlexTMjugador · 2023-10-10T14:33:12Z

It's a bit more convenient to have the old benchmarks readily available than upon bisecting or checking the Git logs, and they are clearly labeled as old and not shown by default. But I don't have a strong opinion either way, and I don't mind removing them if appropriate.

andrews05 · 2023-10-10T19:17:24Z

Let's do this!

Co-authored-by: Alejandro González <me@alegon.dev>

andrews05 requested a review from AlexTMjugador September 27, 2023 01:17

ace-dent reviewed Sep 29, 2023

View reviewed changes

README.template.md Outdated Show resolved Hide resolved

andrews05 force-pushed the v9 branch from 3737b8b to f489e32 Compare September 30, 2023 02:48

XhmikosR reviewed Oct 2, 2023

View reviewed changes

AlexTMjugador force-pushed the v9 branch from b9326ac to df24bdf Compare October 9, 2023 21:19

andrews05 added 3 commits October 10, 2023 12:44

Update changelog

e00f165

Remove readme template

0b1fe34

Bump version

a8b0ca1

andrews05 force-pushed the v9 branch from df24bdf to f95ded6 Compare October 9, 2023 23:48

andrews05 and others added 4 commits October 10, 2023 12:58

Update compare script

db7f238

Improve wording around alpha optimization

0d7a29a

Add updated benchmarks to readme

555c6a8

Add note about short and long options.

4ed4bdd

andrews05 force-pushed the v9 branch from f95ded6 to 4ed4bdd Compare October 10, 2023 00:49

andrews05 marked this pull request as ready for review October 10, 2023 00:54

Update MSRV

d4c4413

AlexTMjugador approved these changes Oct 10, 2023

View reviewed changes

andrews05 merged commit e1db84f into master Oct 10, 2023
13 checks passed

andrews05 deleted the v9 branch October 10, 2023 19:18

Pr0methean pushed a commit to Pr0methean/oxipng that referenced this pull request Dec 1, 2023

V9 Change log and read me (shssoichiro#559)

56d6948

Co-authored-by: Alejandro González <me@alegon.dev>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V9 Change log and read me #559

V9 Change log and read me #559

andrews05 commented Sep 27, 2023 •

edited

Loading

AlexTMjugador commented Sep 28, 2023

andrews05 commented Sep 28, 2023 •

edited

Loading

andrews05 commented Sep 28, 2023

ace-dent Sep 29, 2023

andrews05 Sep 30, 2023 •

edited

Loading

AlexTMjugador commented Sep 29, 2023 •

edited

Loading

XhmikosR Oct 2, 2023

andrews05 Oct 2, 2023

ace-dent Oct 8, 2023

andrews05 Oct 10, 2023

AlexTMjugador commented Oct 9, 2023

andrews05 commented Oct 9, 2023

AlexTMjugador commented Oct 9, 2023

andrews05 commented Oct 9, 2023

AlexTMjugador commented Oct 9, 2023

andrews05 commented Oct 9, 2023

AlexTMjugador commented Oct 10, 2023

ace-dent commented Oct 10, 2023 •

edited

Loading

AlexTMjugador commented Oct 10, 2023

andrews05 commented Oct 10, 2023

V9 Change log and read me #559

V9 Change log and read me #559

Conversation

andrews05 commented Sep 27, 2023 • edited Loading

AlexTMjugador commented Sep 28, 2023

andrews05 commented Sep 28, 2023 • edited Loading

andrews05 commented Sep 28, 2023

ace-dent Sep 29, 2023

Choose a reason for hiding this comment

andrews05 Sep 30, 2023 • edited Loading

Choose a reason for hiding this comment

AlexTMjugador commented Sep 29, 2023 • edited Loading

XhmikosR Oct 2, 2023

Choose a reason for hiding this comment

andrews05 Oct 2, 2023

Choose a reason for hiding this comment

ace-dent Oct 8, 2023

Choose a reason for hiding this comment

andrews05 Oct 10, 2023

Choose a reason for hiding this comment

AlexTMjugador commented Oct 9, 2023

andrews05 commented Oct 9, 2023

AlexTMjugador commented Oct 9, 2023

andrews05 commented Oct 9, 2023

AlexTMjugador commented Oct 9, 2023

andrews05 commented Oct 9, 2023

AlexTMjugador commented Oct 10, 2023

ace-dent commented Oct 10, 2023 • edited Loading

AlexTMjugador commented Oct 10, 2023

andrews05 commented Oct 10, 2023

andrews05 commented Sep 27, 2023 •

edited

Loading

andrews05 commented Sep 28, 2023 •

edited

Loading

andrews05 Sep 30, 2023 •

edited

Loading

AlexTMjugador commented Sep 29, 2023 •

edited

Loading

ace-dent commented Oct 10, 2023 •

edited

Loading