[FFmpeg-devel] [PATCH 2/2] zlib fallbacks for pngdec and zmbv.

[Reimar Döffinger]

On Thu, Mar 24, 2016 at 05:14:49PM +0100, Hendrik Leppkes wrote:
> On Thu, Mar 24, 2016 at 4:33 PM, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> > This should demonstrate how well or badly the
> > minimal inflate fits into code designed around zlib.
> > I am not clear on whether the pngdec implementation
> > was done explicitly to save memory, some other reason,
> > or possibly for no good reason at all.
> >
> 
> This seems like a whole lot of (ugly) code. If multiple zlib variants
> should be supported (and I personally question that as well, and my
> main platform is one without native zlib), then ifdef'ing every single
> usage of it in the code is not the way to go, IMHO.

As to the need for multiple zlib implementation:
I have a strong impression that having only one
zlib implementation has lead us to rely on implementation/API
details of zlib.
Even if it is zlib, I find it very problematic
to see loads of code where replacing one implementation
of the algorithm with another (because it might be
faster, simpler, smaller, more secure) is quite hard.
It basically means that code will be stuck without
much chance of progress.
So doing this has convinced me even more that I want
to push for it.

Avoiding the ifdefs would definitely be a goal,
but it's not possible to design an API without
looking at the options and the consequences.
A state/context based API like zlib's results in
quite a lot of boilerplate code (though that might
be in large part zlib's fault, and not in general
for that kind of API), while a stateless one
can't really wrap zlib API in an efficient manner.
The ifdefs are also worse due to a fact that we
have been duplicating a lot of zlib related code all
over the place, inconsistently.
For example why does PNG use ff_png_zalloc to make
zlib use FFmpeg's allocations functions but zmbv
does not? Surely that means either that's useless
code in PNG or zmbv is buggy?
I think some code also still has the const casts
we fixed by adding the magic -DZLIB_CONST.

I may for them moment go over to clean up the zlib
related code first, but I'd appreciate anyone who
has the time to make constructive proposals.
While you are quite right with your complaint,
it doesn't exactly help me find an alternative
to "leave the zlib stuff be the crap it is now" :)