[Spice-devel] [spice-common] Some steps toward quic_tmpl.c and quic_rgb_tmpl.c 'unification'

[Frediano Ziglio]

> 
> Any more thoughts/reviews on this series?
> 
> Christophe
> 

I asked to have a test and you said is not worth.
I asked to split the series in different ones and you said is not worth.
For me a single "is not worth" is a self nack.
So... why are you asking for though?

Frediano

> On Wed, Jul 05, 2017 at 10:27:18AM -0400, Frediano Ziglio wrote:
> > > 
> > > Hey,
> > > 
> > > This is a v2 of the initial 2 patches, but looking a bit closer at the
> > > quic
> > > code led me down a rabbit hole ;) We currently have 2 different quic
> > > implementations using the preprocessor as a template engine. These 2
> > > implementations are quic_tmpl.c which handles 1 byte and 4 bytes images,
> > > and the quic_rgb_tmpl.c which handles 24bpp/32bpp/16bpp images.
> > > 
> > 
> > Looks like this more than a series is a set of chained series dealing with
> > 1- compiler problems
> > 2- typos
> > 3- removal obsolete stuff
> > 4- code reuse/unification
> > 
> > I think 1) and 2) could be integrated straight forward. 3) requires
> > mostly a team agreement that I don't think is hard to have, Quic is used
> > only by Spice, already superseded by other encoding so I don't see any
> > point keeping old dead code branches dealing with different settings.
> > 
> > > The main difference is that the former implementation handles a single
> > > channel,
> > > while the rgb implementation handles each color component separately.
> > > Besides
> > > that,
> > > the implementation is the same, but this is not obvious at all by just
> > > comparing the
> > > 2 files. This patch series makes the 2 files closer, but not easily
> > > mergeable
> > > yet ;)
> > > 
> > > The biggest pending difference is that quic_tmpl.c carries around a
> > > 'channel'
> > > with the data it needs, while quic_rgb_tmpl.c iterates over
> > > Encoder::channels
> > > when needed.
> > > 
> > > This imo does not justify duplicating that much code, but I don't want to
> > > spend
> > > too much time on quic either, so I'll stop there for now :)
> > > 
> > > I've only tested the RGB32 case, as I'm not sure how to trigger the 1
> > > byte or
> > > 4
> > > byte code paths (I've tried win7 and fedora 25 guests so far).
> > > 
> > > Christophe
> > 
> > The problem of point 4) is that is worth but if we are not sure we are
> > not breaking stuff is not that great. We should have a test that test only
> > image compression/decompression. I was trying to write a similar test
> > for Windows agent for png using a mix of shell/ImageMagick/C code. Lost
> > the commands I was using :-( but mainly in imagemagick there are some test
> > images, I was taking these images with different formats (grey/palette/rgb/
> > rgba and so on) and trying to compress -> uncompress checking that input
> > and output was the same (as PNG is lossless).
> >