Use piston_image instead of stb_image for decoding JPEGs

[kaksmet]

The main reason stb_image was used for decoding JPEGs was the lack of progressive support in piston_image.
With version 0.7, piston_image gained support for decoding progressive JPEGs through use of the jpeg-decoder crate.

This PR removes the dependency on stb_image and instead uses piston_image 0.7 for decoding JPEGs.

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @SimonSapin (or someone else) soon.

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[kaksmet]

Here are some performance numbers from this tool.
The images being tested are the included 4043x2641 4:2:0 baseline and progressive JPEGs.

$ ./bench.py deer.jpg
libjpeg-turbo (1.4.2)          48.55ms
stb_image (2.10)               110.04ms
jpeg-decoder (0.1.0)           149.80ms
piston-image (0.6.1)           247.39ms
go (1.6)                       193.93ms

$ ./bench.py deer_progressive.jpg
libjpeg-turbo (1.4.2)          111.15ms
stb_image (2.10)               193.93ms
jpeg-decoder (0.1.0)           301.17ms
piston-image (0.6.1)           failed
go (1.6)                       323.94ms

[frewsxcv]

Don't those benchmarks suggest we should stick with stb_image?

[kaksmet]

@frewsxcv while it currently will be a performance regression, it will replace a C dependency with one written in Rust.

jpeg-decoder is also quite new and most work this far has gone into getting it spec-compliant, so there's still some optimization left to be done.

[frewsxcv]

@frewsxcv while it currently will be a performance regression, it will replace a C dependency with one written in Rust.

SGTM. I'll let someone else review though.

[kaksmet]

With version 0.1.1 of jpeg-decoder, baseline decoding went from 149.80ms to 124.82ms.

[bors-servo]

☔ The latest upstream changes (presumably #9814) made this pull request unmergeable. Please resolve the merge conflicts.

[SimonSapin]

$ ./bench.py deer_progressive.jpg
piston-image (0.6.1)           failed

This comment in the removed code doesn’t seem to be addressed:

For JPEG images, we use stb_image because piston_image does not yet support progressive JPEG.

So this is regressing not only on performance, but also on functionality.

Beyond that, I don’t feel I can make a decision about this. r? @metajack, you’ve been working on a JPEG decoder lately, haven’t you?

[kaksmet]

@SimonSapin that comment is no longer relevant as piston-image 0.7 replaced it's in-tree JPEG decoder (which did not support progressive) with the jpeg-decoder crate (which does support progressive).

So the piston-image (0.6.1) in the benchmarks is pison-image's old decoder. The one that this PR would land in servo is jpeg-decoder (through piston-image 0.7).

Sorry for the confusion, the numbers you should be looking at are these:

$ ./bench.py deer.jpg
stb_image (2.10)               110.04ms
jpeg-decoder (0.1.1)           124.82ms

$ ./bench.py deer_progressive.jpg
stb_image (2.10)               193.93ms
jpeg-decoder (0.1.1)           307.42ms

[metajack]

Please squash and this is ready to merge.

I tihnk regressing performance is ok, since we expect performance to improve and it wasn't "production quality" anyway. I am working on a new GPU assisted JPEG decoder which will hopefully be faster than state of the art. If down the line we can't make Piston's decoder fast and the GPU ideas don't pan out, we can just switch to a binding of libjpeg-turbo.

+S-needs-squash

---

Reviewed 12 of 12 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

components/net_traits/image/base.rs, line 50 [r1] (raw file):
This seems slightly wrong. We don't want to allow arbitrary formats through even if Piston support them. We want to restrict to web supported formats. Adding new formats to the web accidentally is not good.

I don't think your code did this, but could you flie a bug to fix this behavior?

---

Comments from the review on Reviewable.io

[kaksmet]

There is already #8406 regarding your concern about restricting supported file formats. It also seems to be fixed and should be closed?

[bors-servo]

☔ The latest upstream changes (presumably #9827) made this pull request unmergeable. Please resolve the merge conflicts.

[metajack]

You're right. This was fixed. You can ignore my comment :)

As soon as you rebase I'll r+.

[kaksmet]

Rebased

[Ms2ger]

@bors-servo r=metajack

[bors-servo]

📌 Commit 8c07a1a has been approved by metajack

[bors-servo]

⌛ Testing commit 8c07a1a with merge a8c321a...

[bors-servo]

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt