gnuplot-beta

Re: palette test? [Re: patchset #1448674]

[Ethan A M. <merritt@u.washington.edu>]

On Tuesday 14 March 2006 08:34 am, Daniel J Sebald wrote:
> 
> 1) Is this test from color.c complete?
> 
>     if (save_pal.colorFormulae < 0
> 	|| sm_palette.colorFormulae != save_pal.colorFormulae
> 	|| sm_palette.colorMode != save_pal.colorMode
> 	|| sm_palette.formulaR != save_pal.formulaR
> 	|| sm_palette.formulaG != save_pal.formulaG
> 	|| sm_palette.formulaB != save_pal.formulaB
> 	|| sm_palette.positive != save_pal.positive
> 	|| sm_palette.colors != save_pal.colors) {

I think it is not complete.
It also has some strange tests (e.g. so far as I can see,
colorFormulae can never be anything but 37).


> 2) Would it make sense to replace the above lines of code in color.c with the function
> 
> palettes_differ(t_sm_palette *p1, t_sm_palette *p2)

You mean, replace it for the purpose of printing a message?  I don't know.

But I think this code has some problems in it right now:

>    save_pal = sm_palette;

copies current palette to save_pal, including pointers

>   if (sm_palette.color != NULL) {
>        free(sm_palette.color);
>        sm_palette.color = NULL;
>    }

Bad!  Now save_pal contains a pointer to an color array
that has just been freed.

>    sm_palette.color = gp_alloc( sm_palette.colors * sizeof(rgb_color),
>                                 "pm3d palette color");
>    /*  fill sm_palette.color[]  */
>    for (i = 0; i < sm_palette.colors; i++) {
>        gray = (double) i / (sm_palette.colors - 1);    /* rescale to [0;1] */
>        rgb1_from_gray( gray, &(sm_palette.color[i]) );
>    }

OK. We allocate and fill in a new color array.
But we cannot compare it to the old one, because that one
was freed prematurely.

>    /* let the terminal make the palette from the supplied RGB triplets */
>    term->make_palette(&sm_palette);

This is where we would like to implement Daniel's proposed
non-redundancy test:

    if (palettes_differ(&sm_palette, &sav_pal))
        term->make_palette(&sm_palette);

But in order to do that, the memory allocate/free needs to be cleaned up.
Also as we discussed, there needs to be a separate test for whether this
is the same terminal as we sent the previous palette to.

Further, Daniel has worried that some terminals might require reloading
the palette every time, even if it *is* the same.  I don't think that is
the case for any real terminal. Does anyone know differently?

What do you think?  It seems to me that this would be a much smaller
and cleaner change than Daniel's patch #1448674, and it fixes the use
for all terminals, not just x11.


-- 
Ethan A Merritt
Biomolecular Structure Center
University of Washington, Seattle 98195-7742

Re: palette test? [Re: patchset #1448674]

[Ethan A M. <merritt@u.washington.edu>]

On Tuesday 14 March 2006 09:42 am, Ethan A Merritt wrote:
> >    /* let the terminal make the palette from the supplied RGB triplets */
> >    term->make_palette(&sm_palette);
> 
> This is where we would like to implement Daniel's proposed
> non-redundancy test

Doh!  Actually, it isn't.  x11 does its own palette maintenance,
and gets called at the earlier call site.  

That makes it all even easier.
Please test the following trivial patch and see if it solves whatever
problems you were having.
Also test for bad interactions with other terminal types.

--- gnuplot/src/color.c	2006-02-19 21:09:15.000000000 -0800
+++ gnuplot-cvs/src/color.c	2006-03-14 11:25:47.000000000 -0800
@@ -115,7 +115,13 @@
 	   It will not change palette passed below, but non-NULL has to be
 	   passed there to create the header or force its initialization
 	 */
-	term->make_palette(&sm_palette);
+
+	if (memcmp(&save_pal, &sm_palette, sizeof(t_sm_palette))) {
+	    term->make_palette(&sm_palette);
+	    save_pal = sm_palette;
+	    FPRINTF((stderr,"make_palette: calling term->make_palette for term with ncolors == 0\n"));
+	} else
+	    FPRINTF((stderr,"make_palette: skipping duplicate palette for term with ncolors == 0\n"));
 	return 0;
     }
 


-- 
Ethan A Merritt
Biomolecular Structure Center
University of Washington, Seattle 98195-7742

Re: palette test? [Re: patchset #1448674]

[Daniel J S. <dan...@ie...>]

Ethan A Merritt wrote:

> That makes it all even easier.
> Please test the following trivial patch and see if it solves whatever
> problems you were having.
> Also test for bad interactions with other terminal types.

OK, I've tried it.  Now, this works very well for x11.trm and the redrawing problem.  I think its performance is better than the patch that I created because, not only is there reduced code, there isn't the step of creating the palette and doing an inefficient test.  The memcmp is a much faster routine.  [I would consider verifying that the memcmp alone catches everything because there is a pointer in that structure.  Couldn't it be the case that if a new palette is built its pointer address could somehow be the same as an old palette if somehow the first palette is deleted before the second one is defined?  The contents of the memory could be different but the starting address the same.]

In any case, what I suspected for other terminals with the "further back test" has shown to be the case.  Try the following commands for the PostScript terminal:

  set term postscript color solid
  set output 'test.ps'
  set pm3d
  splot x
  set output

[now look at the file test.ps in a postscript viewer... looks fine]

  set output 'test.ps'
  replot
  set output

[now look at the file test.ps in a postscirpt viewer... doesn't work]

The problem is that the second plot isn't getting the palette information it needs in the file.

I've tried the above with the patch I created and PostScript still works.

I'm not advocating my patch anymore.  Since we've gone this far why don't we consider solving it the appropriate way.  Going back to my email of a few months back:

> On the _terminal driver side_ of the pipe is the following test:
> 
> /* Only send the palette if it is different from the last palette,
> * one hasn't been sent yet, or if the plot number is different from
> * the plot number the last time the palette was set.
> */
> 
> If one thinks through the logic for that, you'll find it avoids flaky 
> behavior on part of the palette, even in multiplot mode.
> 
> That keeps gplt_x11.c from having to reconstruct the color tables unless 
> necessary.  The refresh speedup is clearly back to what it once was.
> 
> I would add that another part of this equation is that the gnuplot core 
> doesn't need to send the palette so often.  If it followed the formula 
> that it only send the palette when the _terminal_ changes or the palette 
> commands are entered, it would reduce more wasted CPU.  (If some devices 
> need a copy of the palette for every plot, the driver should keep a copy 
> internally.)  Don't want to get into that, however.  (Note the rule 
> would change if developers went the path of the core used plots as 
> objects, Hans' desire.)

Ethan has also proposed sending a variable along with the palette

 	TBOOLEAN same_palette_as_before

which we agreed wasn't the most elegant of solutions.

I'd prefer following the above rule to cut down on extra CPU usage.  I know, I know, CPUs today are fast-fast-fast.  But I always prefer good code... my original attempt at this fix was just a patch to avoid hurting anything else.

Dan