Hi, the regtesting seems to have finished correctly, in 10 hours or so i'll give it a final look and probably commit it, thanks for the patch.

I've noticed that you use out->restoreState(state); several times in Gfx.cc, why not using Gfx::restoreState, and it doesn't seem to be a previous saveState() for any of them. Is it really needed? output devs already call save/restore in beginTextObject/endTextObject

(In reply to comment #2)
> I've noticed that you use out->restoreState(state); several times in Gfx.cc,
> why not using Gfx::restoreState, and it doesn't seem to be a previous
> saveState() for any of them. Is it really needed? output devs already call
> save/restore in beginTextObject/endTextObject

Hi Carlos!

Three questions in one :-) 
a) why using out->restoreState? 
The implementation in SplashOutputDev and PSOutputDev is in the way, that image masks or text is set as clipping path, therefore in starting text or image mask colorizing need to save the state (in the output device) for later remove the clipping path again correctly. Of cource I could use there the Gfx::saveState, but I need only to save the state in the ouptut device.
After filling the clipping path with doPatternFill, we have to remove the clipping path, and that is done by out->restoreState.
b) where is out->saveState?
It is done, when recognized that text or an image mask has to be set in pattern colorspace, Gfx remembers this in setting either textHaveCSPattern or maskHaveCSPattern to gTrue only if the output device supports it, colorspace is pattern colorspace AND output device calls it own saveState.
c) why not using Gfx::restoreState
I thought it was clearer, when calling out->saveState in the implementation, just calling out->restoreState to remove clipping path again.

Best regards,
Thomas

(In reply to comment #3)
> (In reply to comment #2)
> > I've noticed that you use out->restoreState(state); several times in Gfx.cc,
> > why not using Gfx::restoreState, and it doesn't seem to be a previous
> > saveState() for any of them. Is it really needed? output devs already call
> > save/restore in beginTextObject/endTextObject
> 
> Hi Carlos!
> 
> Three questions in one :-) 
> a) why using out->restoreState? 
> The implementation in SplashOutputDev and PSOutputDev is in the way, that image
> masks or text is set as clipping path, therefore in starting text or image mask
> colorizing need to save the state (in the output device) for later remove the
> clipping path again correctly. Of cource I could use there the Gfx::saveState,
> but I need only to save the state in the ouptut device.

then I think it should be called from the outputdice rather than from Gfx.cc

> After filling the clipping path with doPatternFill, we have to remove the
> clipping path, and that is done by out->restoreState.

but doPatternFill already uses save/restore

> b) where is out->saveState?
> It is done, when recognized that text or an image mask has to be set in pattern
> colorspace, Gfx remembers this in setting either textHaveCSPattern or
> maskHaveCSPattern to gTrue only if the output device supports it, colorspace is
> pattern colorspace AND output device calls it own saveState.

so we are calling save from the output device and restore from Gfx?

> c) why not using Gfx::restoreState
> I thought it was clearer, when calling out->saveState in the implementation,
> just calling out->restoreState to remove clipping path again.

do you mean you only need to save/restore the clipping path?

> Best regards,
> Thomas

By the way, I've noticed that adrianj patch to implement it in cairo output device doesn't change drawImageMask, so we are still drawing the image mask instead of setting a new softmask in that case. Do you have a test case where that makes a difference?

Thanks!

(In reply to comment #4)

I think I need to clearify how colorizing text with pattern colorspace is working:

1. If I encounter when starting text (BT command) or changing colorspace inside a BT/ET bracket, I stop painting text itself but set rendering mode to use text as clipping path in the output device. To restore the former state when ending this behaviour I call saveState i.e. in SplashOutputDev::beginTextObject
2. From this time on, all text displaying in the output device is suspended, instead of it text clipping path is extended.
3. In this state, when coming to an ET or when changing colorspace back to a "normal" colorspace i.e. with opSetFillColorSpace or opSetFillCMYKColor or simular, the actual text clipping path need first to be filled with private Gfx::doPatternFill before restoring the state saved in 1. If we omit saveState in 1, the text clipping path will remain active, because saveState/restoreState in doPatternFill of course restore the text clipping path, isn't it?

To hide the restoreState from Gfx.cc and do it i.e. in the output device (keep in mind that we stll need the text clipping path) in´the endTextObject in the output device, we would need the doPatternFill available in endTextObject. This would make it clearer, but would need much more changes in code. And please keep in mind, that this behaviour (calling out->restoreState without matching out->saveState in Gfx.cc but with matching saveState in SplashOutputDev::beginTextObject, I don't mean that this makes it more understandable, but at least it seems to work) was already introduced in the patch of bug 19670, this patch just complete that behaviour where it is missing.
If You have any better idea to make it more understandable, I would have no pain to redesign and test it once again :-)

What I redesigned in this patch, was colorizing image masks with pattern colorspace in SplashOutputDev.cc, because the old using softmasks doesn't work with http://www.alfa.de/2285316_1_4.pdf.

Regarding cairo output: I never compiled it and therefore never tested it 'cause I have no test case for it, but in my opinion simply drawing the image mask will loose the black shadow of the white text in http://www.alfa.de/2285316_1_4.pdf (because the image mask will just paint white color, the black shadow comes from the pattern colorspace), and it should give incredibal results with http://www.alfa.de/de/admin/content/document/PDF/cafebarspielsalon_26022009_01.pdf from bug 19994 where I introduced colorizing image masks with pattern colorspace.

Hoply this comments make the things clearer.

(In reply to comment #5)
> (In reply to comment #4)
> 

> To hide the restoreState from Gfx.cc and do it i.e. in the output device (keep
> in mind that we stll need the text clipping path) in´the endTextObject in the
> output device, we would need the doPatternFill available in endTextObject. This
> would make it clearer, but would need much more changes in code. And please
> keep in mind, that this behaviour (calling out->restoreState without matching
> out->saveState in Gfx.cc but with matching saveState in
> SplashOutputDev::beginTextObject,

what I look confusing is that endTextObject already calls restoreState(). 

>  I don't mean that this makes it more
> understandable, but at least it seems to work) was already introduced in the
> patch of bug 19670, this patch just complete that behaviour where it is
> missing.

I know, but I didn't see your patches at that time, we should probably discuss this in the original bug report, but it's closed now so better use this one which is mostly the same topic and it's open :-)

> Regarding cairo output: I never compiled it and therefore never tested it
> 'cause I have no test case for it, but in my opinion simply drawing the image
> mask will loose the black shadow of the white text in
> http://www.alfa.de/2285316_1_4.pdf (because the image mask will just paint
> white color, the black shadow comes from the pattern colorspace), and it should
> give incredibal results with
> http://www.alfa.de/de/admin/content/document/PDF/cafebarspielsalon_26022009_01.pdf
> from bug 19994 where I introduced colorizing image masks with pattern
> colorspace.

I'll try with those to fix cairo output device if it's broken, thanks!

> Hoply this comments make the things clearer.

Yes, sure, thank you very much.

(In reply to comment #6)
: : :
> what I look confusing is that endTextObject already calls restoreState(). 
> 

Sure? If I didn't make a mistake, restoreState is only called if and only if actual colorspace is no more a pattern color space but the marker haveCSPattern is still gTrue. So this is just a safety line.

I cannot reproduce any uninitialized memory access without the Function.cc patch, moreover you are looping over "c" but not using it at all. Can you please provide some backtrace or more info explaining how that happens?

(In reply to comment #8)
> I cannot reproduce any uninitialized memory access without the Function.cc
> patch, moreover you are looping over "c" but not using it at all. Can you
> please provide some backtrace or more info explaining how that happens?

You're true, I cannot reproduce it either. I don't know, if it comes from other code changes, that it is no more reproducable, and okay, the correct code would be

Function::Function() {
	int c;
	for (c = 0; c < funcMaxInputs; c++) {
		domain[c][0] = 0.0;
		domain[c][1] = 1.0;
		range[c][0] = 0.0;
		range[c][1] = 1.0;
	}
}

And even if I think that it is a good idea to initialize all class variable in a constructor and don't let it up to the compiler to do it correctly, it's up to You if You commit this part of code or not.

Created attachment 35100 [details]
Sample PDF where this new patch will cause new regression

(In reply to comment #10)
> Created an attachment (id=35100) [details]
> Sample PDF where this new patch will cause new regression

Unfortunately, even running through the regression test, a part of the patch will produce new regressions, see attached PDF. But I found the problem in Gfx::opSetFillColorSpace, and implement it now in the same way I did it in the other fill routines, so I think, it's a little bit more understandable. The only difference to the other fill routines are coming from the different commands. Here the new implementation:

void Gfx::opSetFillColorSpace(Object args[], int numArgs) {
  Object obj;
  GfxColorSpace *colorSpace;
  GfxColor color;

  res->lookupColorSpace(args[0].getName(), &obj);
  if (obj.isNull()) {
    colorSpace = GfxColorSpace::parse(&args[0]);
  } else {
    colorSpace = GfxColorSpace::parse(&obj);
  }
  obj.free();
  if (colorSpace) {
	  if (textHaveCSPattern) {
		  GBool needFill = out->deviceHasTextClip(state);
		  out->endTextObject(state);
		  if (needFill) {
			  doPatternFill(gTrue);
		  }
		  out->restoreState(state);
	  }
	  state->setFillPattern(NULL);
	  state->setFillColorSpace(colorSpace);
	  out->updateFillColorSpace(state);
	  colorSpace->getDefaultColor(&color);
	  state->setFillColor(&color);
	  out->updateFillColor(state);
	  if (textHaveCSPattern) {
		  out->beginTextObject(state);
		  out->updateRender(state);
		  out->updateTextMat(state);
		  out->updateTextPos(state);
		  textHaveCSPattern = colorSpace->getMode() == csPattern;
	  } else if (drawText && out->supportTextCSPattern(state)) {
		  out->beginTextObject(state);
		  textHaveCSPattern = gTrue;
	  }
  } else {
    error(getPos(), "Bad color space (fill)");
  }
}

With this change now all my samples are working, and the new one, too.

@Albert: Do You think this piece of code is sufficient, or should I make once again a new patch?

Sorry again for changes and changes, but as I discussed it already with Carlos: The xpdf and therefore also poppler was not made for colorizing text in pattern colorspace, and only the trick with text clipping and filling it later seems to work, but that means that we must be very careful with former states and changing a state.
But hopefully (as I thought a lot of times in the past :-) ), this is the really last change for this feature!

No need for a new patch.

That works on the splash outputdev, but unfortunately creates a regression in the cairo outputdev in the file from bug 19670 due to store/restore.

Do you think is there any possibility of making all the store/restore in Gfx?

If not, Carlos do you think there is any chance of you fixing that regression?

I'd really like to add this since it fixes some nasty pdf files.

(In reply to comment #12)
> No need for a new patch.
> 
> That works on the splash outputdev, but unfortunately creates a regression in
> the cairo outputdev in the file from bug 19670 due to store/restore.
> 
> Do you think is there any possibility of making all the store/restore in Gfx?
> 
> If not, Carlos do you think there is any chance of you fixing that regression?
> 
> I'd really like to add this since it fixes some nasty pdf files.

I think, I could call out->saveState and out->restoreState in Gfx in case of recognizing pattern colorspace. But I had a look in CairoOutputDev trying to see the problem, but with saveState & restoreState it behaves like SplashOutputDev and PSOutputDev, so I should remove it there, too, but can't really test it. So I would prefer a solution for the regression in cairo first, and afterwards to move the store/restore in Gfx if still necessary (or to make the things clearer)

(In reply to comment #12)
> No need for a new patch.
> 
> That works on the splash outputdev, but unfortunately creates a regression in
> the cairo outputdev in the file from bug 19670 due to store/restore.

I'm not sure the regression is due to save/restore, since splash returns splashErrNoSave several times, but the error is silently ignored.

> Do you think is there any possibility of making all the store/restore in Gfx?
> 
> If not, Carlos do you think there is any chance of you fixing that regression?

I'll try it.

> I'd really like to add this since it fixes some nasty pdf files.

Patch commited, keep them coming!