Switch to Piston's image library

[jdm]

rust-image appears to support everything we require from an image library right now, and it's native rust code. It would be nice to scrap stb-image and rust-png if possible, but we should definitely take some measurements to sure what the performance is like before and after switching.

[jdm]

Note: NCSU students are working on this project, so please contact me before commencing any work on this to avoid overlap.

[kmcallister]

Is there any update on this? @pcwalton says stb-image is the biggest blocker for Facebook timeline -- it can't decode any of their images. That's aside from the security issues that block dogfooding or a tech demo release.

I'm not too worried about image decoding performance at the moment. It's tiny compared to some of the current inefficiencies in resource loading etc. If it shows up in profiles then we can investigate.
Right now Servo's interaction with image decoders is very basic: send some bytes, get (width, height) and a vector of pixels. It's not hard to switch decoders, and Piston's library seems like a good medium-term solution.

[jdm]

#4215 worked, but the students never addressed the review comments and it needs a new owner.

[kmcallister]

I'll try to revive that PR.

[kmcallister]

Working on this again now that the rustup has landed.

[kmcallister]

Now waiting on image-rs/image#401.

[kmcallister]

And image-rs/image#403.

[kmcallister]

Those are fixed; now it's image-rs/image#409.

[waddlesplash]

That was fixed in image-rs/image#412, so there are no more blockers, correct?

[kmcallister]

There are some other issues I didn't file yet :/

I'm going to spend some time looking into piston-image compatibility issues.

[kmcallister]

Now tracking these as image-rs/image#413. See also the testing report.

[frewsxcv]

@notriddle Did you make progress on this? Run into any issues? I noticed you have a commit on your forked Servo

[notriddle]

I had it mostly working (WebP wouldn't work, but I think that might be a
problem in Piston-Image). But then they made images serializable, and that
broke it.

On Tue, Jul 28, 2015 at 8:16 AM Corey Farwell notifications@github.com
wrote:

@notriddle https://github.com/notriddle Did you make progress on this?
Run into any issues? I noticed you have a commit on your forked Servo

—
Reply to this email directly or view it on GitHub
#3368 (comment).

[jdm]

@notriddle Other libraries have accepted configurable serialization PRs, such as servo/rust-url#118 and hyperium/hyper#603 . Might be worth a try!

[paulrouget]

We need this to decode favicons in browser.html. Do we still want to use this library?

[metajack]

We would need to use some favicon library. stb-image is not usable long term (if it supports favicons and that is your concern). Maybe we need to add favicon support to rust-image?

[mbrubeck]

Now that images use shared memory (#6705), just adding a serde feature to the image crate is not enough; we'd need to make it use the ipc-channel crate too and change how it allocates memory for image data.

Rather than push all this stuff directly into image, I think in the short term we can should use image similarly to how we currently use png and stb_image: Just use it to decode the data, then copy the results into our shared-memory image struct.

Compared to the code in #4215 this will mean keeping more of Servo's current code around, at least for now. I think I'll start a new PR from scratch for this short-term solution, unless there are any objections.

(In the long term when Rust's "placement new" equivalent is fully implemented and stabilized, maybe we can make image work with generic custom "placers" including one for IpcSharedMemory, and then revive the changes from #4215 that uses image types more directly.)

[frewsxcv]

For what it's worth, this PR caused a regression and Google News now panics: #8093

EDIT: meant to comment on the PR