85388 - Xprint prints transparent images with black background...

Status: VERIFIED FIXED

(Core Graveyard :: Printing: Xprint, defect)

Description

[Roland Mainz]

Xprint module (_after_ checkin of patch in bug 57820, see
nsXPrintContext::DrawImageBitsScaled() in
mozilla/gfx/src/xprint/nsXPrintContext.cpp) does not print transparent images
correctly. The background of these "alpha" images is always black.

I tested the Xprint module with a normal Xserver (Solaris 7 Xsun; set #define
HACK_PRINTONSCREEN in nsXPrintContext.cpp and use "xprint_preview" as printer
name) and the images appear correctly.

Question to Jay Hobson:
Does Xprt support transparent images ? If not, please take a look at bug 12037 -
they _may_ have an solution (see
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=23962 - but I am not
sure if this is correct)... :-)

Comment 1

[Roland Mainz]

Swapping owner<-->QA, setting target milestone to 0.9.3...

Comment 2

[Roland Mainz]

Attachment: PostScript job created with Solaris 7([phone]) Xprt+Mozilla Xprint module... take a look at the background of the "Yahoo!" image at the top of page 1

Comment 3

[Roland Mainz]

Attachment: GIF image of page 1 (bad dithering "contributed by sdtimage"... =:-)

Comment 4

[Tobias Burnus]

Is this not a dup of bug 12037? (Or at least depending on it?)
This effects all platforms and should therefore rather independend of XPrint

Comment 5

[Roland Mainz]

tobias:
Good question.... maybe I am hunting a ghost here... adding dependicy to bug
12037.

But: When the destination is a normal Xserver (e.g. not Xprt) the images appear
"normal" (but I didn't test this in detail). May be a limitation in Xprt which
should be fixed (I have a workaround for PS level 2 and a solution for for PS
Level 3). That's the reason why Jay Hobson is in the CC:-list... :-)

Comment 6

[Roland Mainz]

Retargeting to milestone 0.9.4... ;-(

Comment 7

[Marcus Münch]

This also appears with Mozilla 0.93 on Windows 2000. Background of printed
transparent GIF-Image is black.

Comment 8

[Roland Mainz]

Moving to milestone 0.9.6 - first we have to implement alpha support in Xprt ;-(

Comment 9

[Tobias Burnus]

Can we print it white as work-around/intermediate resultion? (see also
discussion in bug 12037)

Comment 10

[Roland Mainz]

Well... we can do it better... AFAIK we have the background color information
available - noone prevents use from using it (instead of using "white").

Unfortunately my last attempts to get this right failed because I cannot get the
depth for the alpha bits (thats bug 81353) - we won't get support for 8bit
transparency stuff until bug 81353 gets fixed ... or I implement my own
nsImageXp class to work around that ...

The stuff in bug 12037 is usefull for Xprt PS DDX (e.g. the PostScript driver)
and the PCL3 DDX; PCL5 supports AFAIK transparency in it's full beauty ... 

jay, wanna comment this, please ?

Comment 11

[Roland Mainz]

Attachment: Prototype patch

Comment 12

[Roland Mainz]

Filed prototype patch which supports 1bit and 8bit alpha support.
Any comments ?

Example URL looks impressive (on paper) ... :-))

Comment 13

[Roland Mainz]

Attachment: page 1 of resulting PostScript job (from example URL), dithered GIF image

Comment 14

[Roland Mainz]

http://www.yahoo.de/ does not print very well... I'll blame the inability to
query the alpha mask depth directly ... that's bug 81353 ... ;-(

Comment 15

[Roland Mainz]

Attachment: Patch for 2001-08-28-08-trunk

Comment 16

[Roland Mainz]

Filed patch.
The patch adds alpha image support to Xprint module; additionally some nsCOMPtr
cleanup and sync with PS module have been added (thanks to dbaron for his help
:-)

Requesting r=/sr=

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

nsRenderingContextXlib.cpp: why the static cast?  I think it's ok, however.

putenv((char *)"XSUNTRANSPORT=xxx");  This cast seems to be overkill...

Is this cast needed:  PL_strfree((char *)s);

Why malloc and not a PR/etc routine?  Why +16?
(PRUint8 *)malloc(aHeight * row_bytes + 16)))
(PRUint8 *)malloc((aDHeight * dstimg_bytes_per_line)+16); 

I'm not sure I like abort()'s for debugging in the mainline source (and not in
#if DEBUG)

free(composed_bits);  - again, avoid malloc/free

Modulo those nits (fix or explain), r=rjesup for whatever that's worth

Comment 18

[Roland Mainz]

> nsRenderingContextXlib.cpp: why the static cast?  I think it's ok, however.
> 
> putenv((char *)"XSUNTRANSPORT=xxx");  This cast seems to be overkill...

Per putenv(3c) manual page the supplied string should be allocated from heap.
But I am lame and passing a |const char *| here (this works on most platforms -
at least for Solaris - this putenv() is only "here" to work around a bug in
Solaris7-|XCloseDisplay()| code).
I added the cast to get rid of the Sun Workshop compiler warning...

> Is this cast needed:  PL_strfree((char *)s);

Not really. I just wanted to get rid of the anoying compiler warning produced by
Sun Workshop.

> Why malloc and not a PR/etc routine? 

Easier to purify :-) |PR_Malloc()|&co. are AFAIK only mandatory if the code
should be really crossplatform. |malloc()| is AFAIK available on all Unix/Linux
versions...
Finally... PR_Malloc() is AFAIK only a wrapper which calls another wrapper which
calls another wrapper which  finally calls malloc()...

> (PRUint8 *)malloc(aHeight * row_bytes + 16)))
> (PRUint8 *)malloc((aDHeight * dstimg_bytes_per_line)+16); 
> 
> Why +16?

Safety pad for Purify. In fact it should be
calculated_size+size_of_largest_integer_on_platform ...

> I'm not sure I like abort()'s for debugging in the mainline source (and not in
> #if DEBUG)

Those parts of code should never be called - however - I'd like to have a
coredump for analysis - regardless of debug mode or not.

> free(composed_bits);  - again, avoid malloc/free

Why ? That code should never run on Mac/Win32 ...

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

>> putenv((char *)"XSUNTRANSPORT=xxx");  This cast seems to be overkill...
>
>Per putenv(3c) manual page the supplied string should be allocated from heap.

	I don't see a mention of heap on a RH 7.1 Linux system (though it can't be an
automatic).  On BSD, the string is copied into the env so it can be anything.

>I added the cast to get rid of the Sun Workshop compiler warning...

	What's the type given?  putenv() takes either "const char *" or "char *"
arguments depending on the OS that I've seen.

>> Is this cast needed:  PL_strfree((char *)s);
>
>Not really. I just wanted to get rid of the anoying compiler warning
>produced by Sun Workshop.

	Again, what warning would that be?  's' is a char *.

>> Why malloc and not a PR/etc routine? 
>
>Easier to purify :-) |PR_Malloc()|&co. are AFAIK only mandatory if the code
>should be really crossplatform. |malloc()| is AFAIK available on all Unix/Linux
>versions...
>Finally... PR_Malloc() is AFAIK only a wrapper which calls another wrapper
which
>calls another wrapper which  finally calls malloc()...

	It can cause problems if someone wants to wrapper PR_Malloc/etc to attempt to
do stuff like track memory allocation, etc.  Use PR_Malloc (IMHO).

>> (PRUint8 *)malloc(aHeight * row_bytes + 16)))
>> (PRUint8 *)malloc((aDHeight * dstimg_bytes_per_line)+16); 
>> 
>> Why +16?
>
>Safety pad for Purify. In fact it should be
>calculated_size+size_of_largest_integer_on_platform ...

	HEH???????

	Purify requires all mallocs to be bumped up?  ICK!!!!  I don't believe it. If
so, why isn't this scattered all through the code?  And if it was, why isn't it
hidden inside malloc (or a malloc wrapper, see above), or at least in a macro?

>> I'm not sure I like abort()'s for debugging in the mainline source (and not
in
>> #if DEBUG)
>
>Those parts of code should never be called - however - I'd like to have a
>coredump for analysis - regardless of debug mode or not.

	Then make it DEBUG_yourname or some such.  BTW, coredumps without debug symbols
are pretty useless....  I still object.

>> free(composed_bits);  - again, avoid malloc/free
>
>Why ? That code should never run on Mac/Win32 ...

	I do not see "won't run on Mac/Win32" as a valid argument for avoiding NSPR.

Comment 20

[Roland Mainz]

> >> putenv((char *)"XSUNTRANSPORT=xxx");  This cast seems to be overkill...
>
> >Per putenv(3c) manual page the supplied string should be allocated from heap.
> 
> I don't see a mention of heap on a RH 7.1 Linux system (though it can't be an
> automatic).  On BSD, the string is copied into the env so it can be anything.

Oh... yes... not heap ... an automatic... looks I need more coffee... more...
;-( Sorry...

> >I added the cast to get rid of the Sun Workshop compiler warning...
> 
> What's the type given?  putenv() takes either "const char *" or "char *"
> arguments depending on the OS that I've seen.

Solaris7 |putenv| wanta a |char *| - but the string literal given is a |const
char *| ...

> >> Is this cast needed:  PL_strfree((char *)s);
> >
> >Not really. I just wanted to get rid of the anoying compiler warning
> >produced by Sun Workshop.
> 
> Again, what warning would that be?  's' is a char *.

Sun Workshop 6 Update 2 FCS emits the following warning:
-- snip --
Warning: String literal converted to char* in initialization.
-- snip --

> >> Why malloc and not a PR/etc routine?
> >
> >Easier to purify :-) |PR_Malloc()|&co. are AFAIK only mandatory if the code
> >should be really crossplatform. |malloc()| is AFAIK available on all 
> > Unix/Linux versions...
> >Finally... PR_Malloc() is AFAIK only a wrapper which calls another wrapper
> which
> >calls another wrapper which  finally calls malloc()...
> 
> It can cause problems if someone wants to wrapper PR_Malloc/etc to attempt to
> do stuff like track memory allocation, etc.  Use PR_Malloc (IMHO).

OK... good argument... I'll fix that...

> >> (PRUint8 *)malloc(aHeight * row_bytes + 16)))
> >> (PRUint8 *)malloc((aDHeight * dstimg_bytes_per_line)+16);
> >>
> >> Why +16?
> >
> >Safety pad for Purify. In fact it should be
> >calculated_size+size_of_largest_integer_on_platform ...
> 
> HEH???????
> 
> Purify requires all mallocs to be bumped up?  ICK!!!!  I don't believe it. If
> so, why isn't this scattered all through the code?  And if it was, why 
> isn't it hidden inside malloc (or a malloc wrapper, see above), 
> or at least in a macro?

No no. I mean I do not want to get problems just because my code writes one
byte/int/long "over" the allocated memory area.
Not really reccesary... I'll remove it...

> >> I'm not sure I like abort()'s for debugging in the mainline source (and not
> in
> >> #if DEBUG)
> >
> >Those parts of code should never be called - however - I'd like to have a
> >coredump for analysis - regardless of debug mode or not.
> 
> Then make it DEBUG_yourname or some such.  BTW, coredumps without debug 
> symbols
> are pretty useless....  I still object.

No. It is still possible to track the issue to it's location... I just want to
get a _clue_ where it fails. For example... this abort()-stuff helped me to
narrow-down a issue in the 0.9.2 milestone build. Unix has this abort() feature
for such cases...

BTW: This code is not called unless pavlov starts to return alpha data which are
not 1bit/8bit...

And finally... what should be the replacement for the code if I should not use
abort() ? This is more or less a fatal condition ...

> >> free(composed_bits);  - again, avoid malloc/free
> >
> >Why ? That code should never run on Mac/Win32 ...
> 
> I do not see "won't run on Mac/Win32" as a valid argument for avoiding NSPR.

See above... I am going to file a new patch...

Comment 21

[Roland Mainz]

Attachment: New patch to match r=jesup

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

>Solaris7 |putenv| wanta a |char *| - but the string literal given is a |const
char *| ...

	Ok, that's fine then.

>> >> Is this cast needed:  PL_strfree((char *)s);
>> Again, what warning would that be?  's' is a char *.
>
>Sun Workshop 6 Update 2 FCS emits the following warning:
>Warning: String literal converted to char* in initialization.

	Um.  I don't think it's for that line....  s isn't a literal; it's a char*
automatic.  Unless maybe it's complaining about "char *s=nsnull", in which case
you should put the cast there.

>No no. I mean I do not want to get problems just because my code writes one
>byte/int/long "over" the allocated memory area.

	!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
	If you write 1 byte "over" your allocation, I _want_ the system to hand your
head to you on a platter.  In fact, I demand it.  Hiding logic flaws by
allocating extra space "just in case" is grounds for crucifixion.  IMHO.

>And finally... what should be the replacement for the code if I should not use
>abort() ? This is more or less a fatal condition ..

	If it's truely fatal, fine.  If it merely means that you can't do alpha
(probably the case here), or that the operation requested can't be done (can't
display the image), then you fail the request, or turn off alpha, etc.

	Don't assume that changes to pav's code will automatically get reflected in
yours.  I'd be _really_ pissed if my browser exits because someone added 32-bit
alpha to something and didn't bother to test printing images with a 32-bit
alpha, for example, just because you wanted breadcrumbs to track it down.

I'll look at the new patch.

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

The PL_strfree() issue is because you changed s to "const char *" from "char
*".  Change it back; it's not const (certainly not if you're freeing it!)

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Remove the abort() call and error out in a friendly way (or get someone to tell
me that it's ok), and change s back to char *, and change
MyConvertUCS2ToLocalEncoding() to be "char *" (which is what it should be), and
r=rjesup (for what it's worth)

Comment 25

[Roland Mainz]

Attachment: new patch - More fixes per jesup's comments

Comment 26

[Roland Mainz]

OK... I removed the |abort()| (and return nsnull instead (which causes that the
image does not print with an alpha mask/channel)) and fixed the |const char
*|vs.|char *| stuff.

> If you write 1 byte "over" your allocation, I _want_ the system to hand your
> head to you on a platter. 

 :-))

> In fact, I demand it.  Hiding logic flaws by
> allocating extra space "just in case" is grounds for crucifixion.  IMHO.

Agreed. May be I should stop adding such safety thingies which aren't
neccesary... :-)
BTW: I removed the 16byte pad in my previous patch.

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

r=[e-mail] (for what it's worth)

Comment 28

[Roland Mainz]

jesup:
Thanks!

Requesting r=/sr= ...

Comment 29

[Roland Mainz]

Attachment: New patch for 2001-08-28-08-trunk

Comment 30

[Roland Mainz]

The new patch fixed an issue when the image has either 0 or 1 as width/height
which may cause a PR_Malloc() call with 0 size.

Comment 31

[kinmoz]

sr=[e-mail]

Comment 32

[Roland Mainz]

Retargeting to milestone 0.9.5 for now ...

Comment 33

[Roland Mainz]

Requesting a= for {0.9.5,trunk} ...

Comment 34

[Roland Mainz]

Correction:
Requesting a= for {0.9.4,trunk} ..

Comment 35

[Roland Mainz]

CC:'ing [e-mail] for checkin when tree opens after branching (trunk
only for now...) ...

Comment 36

[Roland Mainz]

Fix checked in into "trunk"
(http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=999674820&maxdate=999675629&who=bryner%25netscape.com).

Going to request a= for 0.9.4 branch once I have facts for drivers that the code
does not cause any problems... :-)

Comment 37

[Masaki Katakai]

Verified on 2002011614 build.