Re: [gnome-print] Filtering code

[Scott Gifford <sgifford suspectclass com>]

Chema Celorio <chema@ximian.com> writes:

> Your work for the filtering stuff is great, excellent work. 

Thanks!

> I started integrating your filter work into gnome-print and did some
> modifications to it to GNOMEfy it. As such the code worked fine and
> there isn't anything wrong about not beeing GNOMEfied from a code
> perspective it works fine, but having a consistent way of writing code
> makes GNOME a lot easier to understand. I got a bit carried away but
> decided tonight to give you what I currently have so you can continue
> working on it.

Hrm...I have mixed feelings on this.  Once everything's working, I'm
hoping I can share this code with the folks at Mozilla and KDE
(neither of whom had decent postscript image encoding when I checked
about a year ago), so I'm reluctant to add a dependency on glib.  On
the other hand, as part of gnome-print it makes sense to be consistent
with everything else...I'll have to contemplate a bit...

> This are some of the GNOMEisms I changed.

[...]

All of those look fine (modulo the above comment about GNOMEfying...)
It's going to take me awhile to be able to understand the GNOMEfied
code, since I don't really know anything about GNOME/GTK's object
system beyond the very basics.  Is there a good tutorial on this
somewhere?

> I didn't finished polishing the rle filter, but added some comments in
> the code. I didn't tested the filter code for hex & rle, it was in
> "compiles" state ;-). I don't think that i screwed anything up but I
> can't bet my life on it. You'll find the patch unfinished and with some
> leftover code too which we need to clean before getting comiting it.

The test suite should be able to confirm whether anything broke.  cd
into tests/filter, and run ./test.  The test suite stuff was included
in my original patch and tarfile.

When I manage to get this to compile, I'll run through the test and
check it out, too.

> On the short term this is what I'd like to do:
> 
> - Only get 3 filters working: hex, rle, & flate. flate is the most
> important one because the objects and classes might need tweaking to get
> this filter to work. Forget about the other filters for now lets get the
> structs and API nailed first.

I don't know enough about compression and zlib to do flate without
some help, which is why I didn't do it originally.  Any pointers?  Do
we already have a dependency on zlib?

Also, is there a reason to use hex instead of a85?  a85 is
significantly smaller.

> - We need to make the filters work both ways, encode and decode to make
> the test suite reliable. Plus we already had code for this in the 1.4
> branch.

In the test suite in tests/filters, I use ghostscript to do the
decoding.  What do you think of that idea?

> - Forget about the _set_pointer and _set_string functions for now,
> hardcode those values. We will use GPANodes later for it. For now, only
> make it so that one filter works and it is passed as an id by the rest
> of gnome-print

Any particular reason you'd like to see this code go?  It's currently
used by the test suite for testing the various combinations of
filters, and was designed with the idea that it would be easy to use
for customizing printing options from a printer properties dialog box
in the long-term, and from a simple environment variable in the
short-term...Something like:

   GNOME_PRINT_IMAGE_ENCODE="rle a85"

for good compression, or

   GNOME_PRINT_IMAGE_ENCODE="hexenc"

for the old behavior.  This could be especially useful as a workaround
for problems with this encoding, or to enable extra compression where
needed if you know your printer supports it.

> - In the testsuite code, I'd like to decode files and decode them rather
> than having known stuff. This makes our testing code scale, we don't
> need to add more files when we port more filters.

In the test code in tests/filters, it encodes files, has ghostscript
decode them, and makes sure they are identical.  Are you talking about
something different than that?

[...]

> Why did you used size_t for needed bytes instead of gint? I changed it
> to gint but have the feeling it should really be size_t.

It's passed to malloc, which takes a size_t.


Some comments on your inline comments:

In gnome_print_filter_rle_finish:

      /* FIXME, we shold have an enum GnomePrintFilterRleRunType rather than using 1 & -1

OK, although that seems to me to add complexity without any real gains...

       * also why is 257 hardcoded below? <chema>
       */

257 is a constant from Adobe's "Filters and Reusable Streams", Sec. 4
Paragaph 1.  It could be a constant with a name like
GNOME_PRINT_FILTER_RLE_LENGTH_SUBTRACTFROM_CONSTANT, but I'm not sure
that's much more illuminating... :-)

      /* If run_type and run_lenght are not beeing modified, why do we pass pointers to int
       * rather than ints? <chema>
       */

Yeah, there doesn't seem to be any reason for this.  IIRC, previous
versions did the setting of *run_length and *run_type back to 0, but
the caller is now responsible for that.

      /* what if we pass only to finish buff a GnomePrintFilterRle pointer? <chema>*/

That would require us to update the structure before calling it, so
would probably have about the same overhead, but other than that I
don't see any problem with it.  finishbuf() started out as a simple
factoring out of repetitive code in gnome_print_rle_filter, which is
why it's written that way.

I'll work on getting this to compile (my environment seems messed up),
and will probably have more comments then.

Thanks,

----ScottG.