Remove the zlib "inflate" implementation from the image library

[sivadeilra]

This removes the zlib "inflate" implementation from the image library, and and instead uses the external zlib implementation from https://github.com/sivadeilra/zlib. This is better for refactoring dependencies, and also quite substantially improves performance for decoding PNG images.

[eddyb]

I have safe Rust code (written more than 6 months ago IIRC) that borderline outperforms zlib. You definitely do not need goto for state machines.
I wrote that for my streaming PNG decoder (on par with libpng, faster for small files), which was meant to be the start of this image library, but I didn't have the time and still haven't gotten around to it yet.

[sivadeilra]

Well great then I've wasted a week. Where's this fast code?

-----Original Message-----
From: "Eduard Burtescu" notifications@github.com
Sent: ‎1/‎2/‎2015 7:01 PM
To: "PistonDevelopers/image" image@noreply.github.com
Cc: "sivadeilra" arlie.davis@gmail.com
Subject: Re: [image] Remove the zlib "inflate" implementation from the imagelibrary (#223)

I have safe Rust code (written more than 6 months ago IIRC) that borderline outperforms zlib. You definitely do not need goto for state machines.
I wrote that for my streaming PNG decoder (on par with libpng, faster for small files), which was meant to be the start of this image library, but I didn't have the time and still haven't gotten around to it yet.
—
Reply to this email directly or view it on GitHub.=

[nwin]

I suppose he means a newer version of this code from #99?

Never-the-less we would still need a fast deflater or do you have something fast on the shelf as well @eddyb?

Btw. that's a classic and reminds me to always file an issue when implementing something big. Funnily I also did some work in this direction. :(

@sivadeilra I skimmed over your lib. What is the performance difference between a save version of your code and the unsafe one? You didn't put cfg-guards around all of them. I also noted quite some panics, would it be possible to replace them with a try! or return? Since it's coming from the internet I expect the data to be malformed occasionally.

If it's safe and faster what we have currently I do not see any reason to not include the code until someone comes up with something even faster and safe.

[eddyb]

@nwin that's the code I am talking about, it should be failrly easy to update. But I don't have a deflater, I wouldn't know where to even start with one.
And given that Rust alpha is so near there's no way I can provide you with anything but inspiration and tips.
Maybe I'll revive all of that fast code whenever we get generators in Rust. Maybe :).

[nwin]

FWIW this version compiles with the latest rust. I can't guarantee it works because there were some nasty type-errors because the integers literals did not have any annotations and I'm not sure that I always picked the right type. The error messages were really funny sometimes…

[sivadeilra]

Hey, I'd like to resume this conversation. Got distracted by a few things.

First, about the "panic!" calls. Right now, just because my zlib port is
under development, the path that detects bad input currently calls panic!.
That can easily be removed. Otherwise, it should not be possible to cause
a panic due to reading user input.

Second, about performance. I don't mean that "goto" is necessary for
performance. What I mean is that, in the current port as it is, the
transformation from zlib's goto-based state machine to a loop based on
matching on an enum has caused inefficient code gen. This can certainly be
fixed.

One important design question is, should the Rust port of zlib provide a
non-blocking API (which is what zlib provides), or a blocking API? The
non-blocking API is partly what causes the need for the gotos in the
original code, since when you start decoding a new block of data you have
to jump to the right state in the state machine. The individual states
jump to each other, but that can be removed, if you're using a non-blocking
design.

I took a look at the inflate.rs that was posted in the gist. It's
interesting. It does provide an API that is significantly different than
what some current users of ZLIB will be expecting.

All I care about is having a great zlib implementation -- I don't care who
provides it. That, and I want an API that 1) will be very easy to port
existing C to use, and 2) fits naturally into Rust abstractions, like
borrowing and Reader.

So, what would you guys like to do? I can work in improving my zlib to
reach C's perf, possibly providing a blocking implementation of Reader if
that provides a substantial improvement in performance. If you like, I can
merge this inflate.rs into my own -- whatever works best. But it seems
clear that 1) Rust / crates.io needs a separate zlib, since so many apps
use zlib, and 2) the Image library should be using that separate zlib.

Thoughts?

On Sat, Jan 3, 2015 at 4:26 AM, nwin notifications@github.com wrote:

FWIW this version https://gist.github.com/nwin/b0b8bb23040a526b5b59
compiles with the latest rust. I can't guarantee it works because there
were some nasty type-errors regarding the integers and I'm not sure that I
always picked the right type. The error messages were really funny
sometimes…

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

[nwin]

I don't have an opinion about that. Nowadays I would guess in most use cases you don't stream images although it is of course neat if it is possible.

[bvssvni]

Closing because of outdated.