77210 - Images don't show up in documents printed via Xprint

Status: VERIFIED FIXED

(Core Graveyard :: Printing: Xprint, defect)

Description

[Roland Mainz]

Per Pavlov's request in bug 76993 - opening a seperate bug for this.

Xprint does not print any images - and I do not have any idea _why_...

Assigning this to pavlov as I hope he may be able to find this bug faster than
me...

Comment 1

[Roland Mainz]

[adopted&updated copy from bug 72087]
Build instructions:
- Download&unpack mozilla source
- Compile tree:
% configure --with-xprint
% make

Usage:
- Start Xprt server (as "root" or normal user)
% Xprt -audit 4 :6 &
- tell mozilla where to find Xprt server(s - XPSERVERLIST can contain multiple
servers seperated via blanks)
% export XPSERVERLIST=":6"
- Run Mozilla
% ./mozilla

Running the Xprt server and setting the XPSERVERLIST should be done globally per
machine or network. Be sure to use "xhost" to grant other machines (xhost
+machine) the access to Xprt on demand...

More details on demand...

Comment 2

[Roland Mainz]

Usage (part 2):
- "print command" should contain the printer name
- print-to-file works but requires to give both filename _and_ valid printer
name as Xprint prints to this printer (e.g. uses the same DDX+config for this
printer) and redirects output back to the application (which copies the stuff
into the given file...)
* set left/top/right/bottom borders to "0.0" to avoid that the header/footer
stuff get's a chanche to add his monster-size text
** wrong arguments are _fatal_... Xprint module returns error - but calling code
doesn't care and continues as if nothing has happened... ;-((

Required Solaris 2.7 patches (both are in the recommended patch cluster; 2.8
does not need patches - but the recommended patch cluster is... uhm...
recommeded... :-):
- Xsun/Xprt patch:
http://sunsolve.sun.com/pub-cgi/patchRedir.pl?type=readme&item=108376
- Xprint extension patch:
http://sunsolve.sun.com/pub-cgi/patchRedir.pl?type=readme&item=107650

Comment 3

[Roland Mainz]

Pavlov... why not milestone 0.9.1 or milestone 0.9.2 ?

Comment 4

[Roland Mainz]

It seems that nsRenderingContext::DrawImage(*) methods nor
nsXPrintContext::DrawImage(*) methods are ever called...

I'll attach build logs with some nice warnings...

Comment 5

[Roland Mainz]

Attachment: gmake log building mozilla/gfx/src/xprint/

Comment 6

[Roland Mainz]

pavlov: It seems that nsRenderingContextXp::GetDrawingSurface() doesn't return a
drawingsurface causes this problem..
...but nsRenderingContextPS::GetDrawingSurface() doesn't do that, too.

The big question for me is then: How can PostScript module print images ?

Comment 7

[Roland Mainz]

Ohhh.... ohhhh... the fix is sooo simple (and doesn't need nsDrawingSurface
stuff :-)... .-)))
I am going to include fix for this in todays patches for bug 78548...

Comment 8

[Stuart Parmenter]

don't retarget my bugs.  if you post a patch i'll review it, but don't change 
the target milestone of bugs that you don't own.

Comment 9

[Roland Mainz]

Overtaking.
Patches in bug 78548 will address most image-related issues in Xprint.

Pavlov... wanna play QA for this bug, please ?

Comment 10

[Roland Mainz]

Accepting bug...

Comment 11

[Stuart Parmenter]

no, i don't want to play qa for this.

Comment 12

[Roland Mainz]

Setting QA=[e-mail]

Filing patch in a few mins...

Comment 13

[Roland Mainz]

Attachment: Patch for 2001-05-20-08-trunk

Comment 14

[Roland Mainz]

Filed patch.

Known issues:
- Xprt-side image scaling with XpSetImageResolution() does not work for some
reason. This is _BAD_ - as the workaround (scaling on Mozilla's side) makes
images in print job four times larger than required and increases printing time
a lot. Scaling via XpSetImageResolution() can be enabled via setting the
MOZILLA_XPRINT_EXPERIMENTAL_ENABLE_XPRT_SCALING env var. Question to Jay Hobson:
Is there a bug in Xprt or is this a bug in my code ?
- Images with transparent parts (nsIImage objects with 1bit "alpha" bitmaps)
print the transparent parts as "black"... uhm... is this my fault or Xprt's
fault ?
- Images with 8bit alpha bitmaps are not implemented yet (uhm... AFAIK not used
by Mozilla yet)
- Error checking is lame... ;-(

This patch contains partial (or full :-) fixes for...
- bug 80562 "Xprint does not support any other visuals than Xprt's default
visual"
- big 57820 "Xprint does not scale images to suit the higher printer resolution"
- bug 54092 "Xprint uses one bitmap in place of all bitmaps on page "

Need r=, sr= and checkin ASAP, please!! Tree for 0.9.1 freezes tomorrow
(23.05.2001)...

Comment 15

[Roland Mainz]

Attachment: Patch for 2001-05-20-08-trunk (same as previous patch but with gdiff -u -w # REVIEW ONLY!!)

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsDeviceContextXP.cpp:

  Perhaps you could move the initialization of members in the
  constructor into member initializers?

  Why comment out the body of |SetDrawingSurface| when you could just
  remove it?

  In |GetDrawingSurface|, you should probably set aDrawingSurface to
  null and then return NS_ERROR_NOT_AVAILABLE or something.  Or is it OK
  to return null and NS_OK?  You should definitely *not* return NS_OK
  without initializing the out param.

  But actually, shouldn't you be implementing GetDrawingSurface
  according to the comments in nsIDeviceContext.h?  Or does printing
  really not need a drawing surface?  (If that's the case, perhaps the
  comment in nsIDeviceContext.h should reflect that?)

nsXPrintContext.cpp:

  You should just initialize |xattributes_mask| to the correct value,
  rather than initializing to 0 and then using |= to assign the correct
  value.  It's confusing the way it is.

  In |SetupWindow|, you're mixing C-like code where you declare all the
  variables at the beginning of the function with more typical C++ code
  where you declare the variables before use.  It would probably be good
  to stick to one or the other.  In SetupWindow it *looks* like all the
  variables are declared at the top but you're adding one that isn't.

  Does the initialization of XSetWindowAttributes to |{0}| really
  zero-initialize the whole thing?  There are a lot of fields that
  you aren't setting.

  In GetScaledXImage, you use malloc and XInitImage when you should
  probably use XCreateImage (which does both for you).

  Maybe DrawImageXImageScaled and DrawImageBits shouldn't be virtual and
  should be private?  Do you really expect anything to inherit from
  nsXPrintContext without major modifications?

  In your use of the env. var.
  MOZILLA_XPRINT_EXPERIMENTAL_ENABLE_XPRT_SCALING it looks like all the
  code you wrote isn't the default and you need the environment variable
  to make it work.  Shouldn't it be the other way around?  And could you
  pick a shorter name for the environment variable?

+  Bool res = XpSetImageResolution(mPDisplay, mPContext, imageResolution,
&prev_res);
+
+  if( res == True )

  Could you just use |if (XpSetImageResolution(...))| (if only to avoid
  the explicit comparison to True).

  Have both branches after |if( res == True)| been tested?  Or is only
  one the usual case?

  Could |DrawImageXImageScaled| have a nicer name?  (DrawXImageScaled?
  DrawImageScaled?)

+  XImage *srcImg        = (XImage *)nsnull;
+  XImage *srcAlphaImg   = (XImage *)nsnull;
+  XImage *subImg        = (XImage *)nsnull;
+  XImage *subAlphaImg   = (XImage *)nsnull;
+  XImage *dstImg        = (XImage *)nsnull;
+  XImage *dstAlphaImg   = (XImage *)nsnull;

  No need for the casts.  Limit your use of casts to when you really
  need them, since casts tend to be one of the first places to look when
  code is buggy.

+  srcImg = (XImage *)malloc(sizeof(XImage));
+  memset(srcImg, 0, sizeof(XImage));
   ...

  Shouldn't you just use XCreateImage here?  (Then you don't need
  XInitImage, which is to be used for an XImage on the stack, I think.)

  In DrawImageXImageScaled, I think you leak dstImg and dstAlphaImg.

  I think you also leak the memory returned from XSubImage in the case
  where you use XSubImage to reallocate srcImg (and the same thing for
  subAlphaImg).  Can't you fix this by making the condition for nulling
  the |data| member be that subXImage is non-null, and removing the
  requirement that it be null outside the call to XDestroyImage?

  What's the difference between XYPixmap and ZPixmap?


General questions / comments (don't need to fix these now):
  Why does nsIDeviceContextXp inherit from an implementation?  Interfaces
  should not have an implementation attached.

  I think 8-bit alpha is used by PNG images in Mozilla.

Comment 17

[Roland Mainz]

> nsDeviceContextXP.cpp:
> 
>   Perhaps you could move the initialization of members in the
>   constructor into member initializers?

In theory yes. But sometimes this way is confusing for people who simply look
into the constructor and think that this is the only place where vars are
initalized...

>   Why comment out the body of |SetDrawingSurface| when you could just
>   remove it?

A reminder for me to check when&how |SetDrawingSurface| is used - and why that
code isn't required anymore...

>   In |GetDrawingSurface|, you should probably set aDrawingSurface to
>   null and then return NS_ERROR_NOT_AVAILABLE or something.  Or is it OK
>   to return null and NS_OK?  You should definitely *not* return NS_OK
>   without initializing the out param.
> 
>   But actually, shouldn't you be implementing GetDrawingSurface
>   according to the comments in nsIDeviceContext.h?  Or does printing
>   really not need a drawing surface?  (If that's the case, perhaps the
>   comment in nsIDeviceContext.h should reflect that?)

I only duplicated the code from PS module here - do _nothing_. if I do anything
here (e.g. aDrawingSurface=nsnull;) layout gets insane... and I do not know yet
_why_ this happens. I need to crawl much deeper into the code and learn how it
works...

> nsXPrintContext.cpp:
> 
>   You should just initialize |xattributes_mask| to the correct value,
>   rather than initializing to 0 and then using |= to assign the correct
>   value.  It's confusing the way it is.

Fixed.

>   In |SetupWindow|, you're mixing C-like code where you declare all the
>   variables at the beginning of the function with more typical C++ code
>   where you declare the variables before use.  It would probably be good
>   to stick to one or the other.  In SetupWindow it *looks* like all the
>   variables are declared at the top but you're adding one that isn't.

I moved the two declarations below the other ones. I usually prefer the C style
- but sometimes it's nice that C++ doesn't force me to do that always...
Fixed.

>   Does the initialization of XSetWindowAttributes to |{0}| really
>   zero-initialize the whole thing?  There are a lot of fields that
>   you aren't setting.

Uhm... good question. my "the annotated ANSI C standard"-book here in the
office... well... AFAIK the SAS/C compiler did "zero" all field this way... but
I never verified this with gcc or Sun Workshop.
I removed that initalisation... xattributes_mask will select the field which
should be used - and both are initalized.
Fixed.

>   In GetScaledXImage, you use malloc and XInitImage when you should
>   probably use XCreateImage (which does both for you).

XCreateImage() requires a |Display *| and some other stuff to work properly -
and it does not (re-)calculate fields after I modify them like XInitImage()
does... see X11R6.6 sources and manual pages... :-)

>   Maybe DrawImageXImageScaled and DrawImageBits shouldn't be virtual and
>   should be private?  Do you really expect anything to inherit from
>   nsXPrintContext without major modifications?

No... and even if that happens he/she may change the code again... :-)
Fixed - both methods are now private and non-virtual...

>   In your use of the env. var.
>   MOZILLA_XPRINT_EXPERIMENTAL_ENABLE_XPRT_SCALING it looks like all the
>   code you wrote isn't the default and you need the environment variable
>   to make it work.  Shouldn't it be the other way around?  And could you
>   pick a shorter name for the environment variable?

I don't know why that code doesn't work - it may be my fault or Xprt's fault.
For now I disabled this code by default until I get a response from Sun. I named
the magic switch "MOZILLA_XPRINT_EXPERIMENTAL_ENABLE_XPRT_SCALING" to avoid any
flames like "that env var is too short" or "the name is misleading" etc. It is
_long_ and describes itself and is going to be removed _very_soon_.

> +  Bool res = XpSetImageResolution(mPDisplay, mPContext, imageResolution,
> &prev_res);
> +
> +  if( res == True )
> 
>   Could you just use |if (XpSetImageResolution(...))| (if only to avoid
>   the explicit comparison to True).

Fixed.

>   Have both branches after |if( res == True)| been tested? 

Yes... both branched have been tested.

> Or is only
> one the usual case?

Per spec: XpSetImageResolution() only works if the DDX (driver) in Xprt supports
scaling of images. The PS and PCL DDX support scaling - but I wouldn't expect
that the XWD DDX supports scaling... =:-)

>   Could |DrawImageXImageScaled| have a nicer name?  (DrawXImageScaled?
>   DrawImageScaled?)

Good question... uhm... it's own a private method... :-)
I renamed it to DrawImageBitsScaled() - I assume that's better... :-)

> +  XImage *srcImg        = (XImage *)nsnull;
> +  XImage *srcAlphaImg   = (XImage *)nsnull;
> +  XImage *subImg        = (XImage *)nsnull;
> +  XImage *subAlphaImg   = (XImage *)nsnull;
> +  XImage *dstImg        = (XImage *)nsnull;
> +  XImage *dstAlphaImg   = (XImage *)nsnull;
> 
>   No need for the casts.  Limit your use of casts to when you really
>   need them, since casts tend to be one of the first places to look when
>   code is buggy.

Ohhh... yes...
Fixed.

> +  srcImg = (XImage *)malloc(sizeof(XImage));
> +  memset(srcImg, 0, sizeof(XImage));
>    ...
> 
>   Shouldn't you just use XCreateImage here?  (Then you don't need
>   XInitImage, which is to be used for an XImage on the stack, I think.)

See above. XCreateImage() requires a |Display *| - and I malloc() them because I
can use XDestroyImage() to get rid of the whole thing incl. all memory/stuff
attached to it...

>   In DrawImageXImageScaled, I think you leak dstImg and dstAlphaImg.

??? Oups... ;-(
Fixed.

>   I think you also leak the memory returned from XSubImage in the case
>   where you use XSubImage to reallocate srcImg (and the same thing for
>   subAlphaImg).  Can't you fix this by making the condition for nulling
>   the |data| member be that subXImage is non-null, and removing the
>   requirement that it be null outside the call to XDestroyImage?

Fixed.

>   What's the difference between XYPixmap and ZPixmap?

http://www.multimania.com/babyloon/xlib.html has a small description and
http://www.linux.ie/pipermail/ilug/2000-September/023608.html for the longer,
detailed one.

> General questions / comments (don't need to fix these now):
>   Why does nsIDeviceContextXp inherit from an implementation?  Interfaces
>   should not have an implementation attached.

Good question. Added to my ToDo list for investigation...

>   I think 8-bit alpha is used by PNG images in Mozilla.

Do you have any example URLs ?

Comment 18

[Roland Mainz]

Attachment: New patch for 2001-05-20-08-trunk

Comment 19

[Roland Mainz]

Uhm... after looking twice on that code...
What about creating a XImage from incoming nsIImage, run XSubImage() (and care
about alpha stuff if required) and pass contents of resulting XImage back to
destination...

blizzard.... do you see any problems with that idea ?

Comment 20

[Roland Mainz]

Ouch ouch... sorry... last comment was for bug 76820... SORRY... wrong browser
window... ;-(((

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> > I think 8-bit alpha is used by PNG images in Mozilla.
> 
> Do you have any example URLs ?

See the bottom of http://www.w3.org/Graphics/PNG/ .

Comment 22

[timeless]

remove:
+  //scaler = 0.5;
typo:
+  /* set image rsolution */
+    /* XpSetImageResolution(), but reset image rsolution to previous value to 
be sure... */
you lost the e in r_e_solution ...

logic question
if( XpSetImageResolution(mPDisplay, mPContext, imageResolution, &prev_res) )
{
Draw(...);
SetImageRes();
} else {
SetImageRes();
Draw(...);
}

other than that, r=timeless

Comment 23

[Roland Mainz]

Attachment: Patch for 2001-05-20-08-trunk to match r=timeless

Comment 24

[Roland Mainz]

I filed a new patch to match r=timeless.

> logic question
> if( XpSetImageResolution(mPDisplay, mPContext, imageResolution, &prev_res) )
> {
> Draw(...);
> SetImageRes();
> } else {
> SetImageRes();
> Draw(...);
> }

The SetImageRes() in the "else" part is only a
reset-because-I-do-not-know-what-happens-if-the-1st-SetImageRes()-fails.
it may be unneccesay... or not.

To Jay Hobson:
Scaling via XpSetImageResolution() may not work with
nsXPrintContext::DrawImageBits() because it does a PutImage() (in
xlib_draw_rgb_image()) _and_ a XCopyArea() (which leads to the question if
images copied XCopyArea() will be scaled by XpSetImageResolution() or not).
But this does not explain why a plain "XpSetImageResolution(); PutImage(); /* by
xlib_draw_rgb_image() */" fails, too (nsXPrintContext::DrawImageBitsScaled() use
this way except for alpha images...).

(And it would be nice to have a XpSetXYImageResolution() where X/Y can have a
different resolution (and where resolution is a float var)...)

Comment 25

[Roland Mainz]

blizzard/pavlov:
can anyone sr= this patch, please ?

Comment 26

[chris hofmann]

let's land this in 0.9.2.  let drivers know if this creates problems. thanks.

Comment 27

[Roland Mainz]

> let's land this in 0.9.2.  let drivers know if this creates problems.

Fired email to [e-mail],[e-mail],blizzard,mkaply to get
this "in" 0.9.1...

Comment 28

[Roland Mainz]

chofmann wrote:
> sounds fine to me if you can get the sr= let us know when that happens.

putting bug back to 0.9.1 radar...

Comment 29

[Michael Hore]

XDestroyImage is only supposed to be used on xlib-created images (XGetImage),
since it Xfree's it's data. I think Xfree is #defined to free most places though
so this might not be a problem.

Comment 30

[Roland Mainz]

To Asa:
Please correct me... the "status whiteboard" is slightly _wrong_. 
AFAIK the patch here got r=timeless - I only need a sr= from someone...

Comment 31

[Roland Mainz]

After some testing... it looks I found the issue why scaling via
XpSetImageResolution() didn't work in my first attempt:
-- snip --
{
  double scale = 29.0,
         int_scale; 
  int    prev_res,
         dummy;
  int    dest_x = 100,
         dest_y = 100;

  /* boooooohhhh... we have to pass the scaling factor as integer... _bad_ */
  int_scale = dpi / (int)(((double)dpi/scale)+0.9999); /* round scaling factor
                                                        * to a value which does
                                                        * not get cut by 
                                                        * integer math... */    
  
  printf("scale=%f, using scale=%f (scale>=int_scale)==%d\n", 
         scale, int_scale, (scale >= int_scale));

  puts("start page.");   
  XPU_TRACE(XpStartPage(pdpy, pwin));
  XPU_TRACE(XpuWaitForPrintNotify(pdpy, XPStartPageNotify));
          
  (void)XpSetImageResolution(pdpy, pcontext, (int)((double)dpi/int_scale), 
                             &prev_res);

  /* in theory we have to scale our image on application-side,
   * (factor=scale/int_scale) to get rid of the integer math rounding
   */
  XPutImage(pdpy, pwin, pgc, IconXImage(), 0, 0, 
            dest_x, dest_y, IconXImage()->width, IconXImage()->height);

  (void)XpSetImageResolution(pdpy, pcontext, prev_res, &dummy);

  XDrawRectangle(pdpy, pwin, pgc, 
                 dest_x-2, dest_y-2, 
                 ((double)IconXImage()->width*int_scale)+4, 
                 ((double)IconXImage()->height)*int_scale+4);

  XPU_TRACE(XpEndPage(pdpy));
  puts("end page.");
  XPU_TRACE(XpuWaitForPrintNotify(pdpy, XPEndPageNotify));      
}
-- snip --

x!!@@xx!!xx - who invented this integer math nightmare ?

At least... there is a way around this: Implement scaling via XIE and modify XIE
implementation on Xprt to use a server-side equivalent for
XpSetXYImageResolution(Display*, XPContext *, double x_res, double y_res, double
*prev_int_res, double *prev_y_res); /* yes, I know... passing double/float via
X11 protocol doesn't work well - but passing two CARD32 values (long x1 = 1, x2
= 2; double x=x1/x2;) should do the trick... */

And a Xprintv2 protcol should implemement XpSetXYImageResolution() and
XpSetXYImageScalers() /* XpSetXYImageScales() is a client-side wrapper for
XpSetXYImageResolution() which accepts x,y-scaling factors instead of "image
resolutions" */ ...

Comment 32

[tor]

Some comments:

  * rename XLIB_RGB_DITHER_XPRINT to NS_XPRINT_DITHER (or something along those
    lines) to keep the XLIB_RGB namespace clean

  * GetScaledXImage() is pretty gross.  Input and output are XImages, even when
    you have access to the raw original data and only use the data field of the
    output image.  Remove the overhead.  You need to scale three things:
    888-rgb, 8-alpha, and 1-alpha data.  The first two are trivial to do by 
    hand and will result in simpler, faster code.  The later will require some
    bit twiddling, but shouldn't be that bad.

  * GetDrawingSurface() should stick null into aSurface in case the caller
    didn't initialize it.

  * DrawImage statics should be PRPackedBool with PR_TRUE/PR_FALSE

  * image scaling is assuming that alpha data is a mask (1-bit)
    (ie: "srcAlphaImg->bits_per_pixel   = 1;")

Comment 33

[Roland Mainz]

> * rename XLIB_RGB_DITHER_XPRINT to NS_XPRINT_DITHER (or something along those
>   lines) to keep the XLIB_RGB namespace clean

Will be fixed.

> * GetScaledXImage() is pretty gross.  Input and output are XImages, even when
>   you have access to the raw original data and only use the data field of the
>   output image.  Remove the overhead.  You need to scale three things:
>   888-rgb, 8-alpha, and 1-alpha data.  The first two are trivial to do by 
>   hand and will result in simpler, faster code.  The later will require some
>   bit twiddling, but shouldn't be that bad.

This code should never ever be called in the final version - it is only planned
as a fallback solution until the _real_ scaling code will land (this is bug
57820) - and for cases where the Xprint-scaling code fails (should not happen,
too - but I'd like to be prepared for the theoretical case... :-) ...
I agree with you... the code is slow&braindead - but it is simple&small, too.
I perfer simple code in such cases - all other code will be more code than
neccesary...

> * GetDrawingSurface() should stick null into aSurface in case the caller
>   didn't initialize it.

Valid argument. But PostScript code does the same stupid thing... I try to fix
that - but it may break something else...

> * DrawImage statics should be PRPackedBool with PR_TRUE/PR_FALSE

This static member will be removed when code in bug 57820 lands... the whole
thing with the env var is a temporary solution...

> * image scaling is assuming that alpha data is a mask (1-bit)
>   (ie: "srcAlphaImg->bits_per_pixel   = 1;")

Known bug. Alpha support doesn't work anyway... I try to fix this in bug 57820,
too...

Comment 34

[Roland Mainz]

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

Comment 35

[Roland Mainz]

I've attached a new patch top get sr= 
Changes:
- Renamed XLIB_RGB_DITHER_XPRINT to MY_XLIB_RGB_DITHER_XPRINT
- GetDrawingSurface() now sets the results explicitly to nsnull
- static bools are now using PRPackedBool

Additional changes (new):
- I fixed mCurrentColor initalisation in nsRenderingContextXP.cpp, default was
"mCurrentColor = NS_RGB(255, 255, 255);" - now it's "mCurrentColor = NS_RGB(0,
0, 0);" - this bug caused that some pages had back boxes with black text...
(this bug was driving me nuts...)

Not fixed:
- GetScaledXImage() - only a temporary solution for now. I don't think it's
worth to think about a smarter,faster,prettier way of scaling XImages when bug
57820 implements a far better, faster and smarter solution (=scaling on
Xserver/printer side)... :-)

Comment 36

[Roland Mainz]

Attachment: Patch for 2001-05-28-08-trunk (same as previous patch + nsFontMetricsXP.cpp patch to fix CJK fonts (fix from bug Bug 66082))

Comment 37

[Roland Mainz]

I've attached a new patch which is identical to the previous one except that it
fixes CJK fonts (solution from bug 66082, a minor change to
nsFontMetricsXP.cpp). The fix is so easy - it screams for integration... :-)

Comment 38

[tor]

The patch from 66082 was unreviewed.  Do not pass go, do not collect $200.

Comment 39

[pocemit]

The only difference in the CJK patch, is addition of "len / 2" in certain
places, which could be gleamed by reading "man XTextWidth16" and noticing that
the length parameter for the checked string must be half the length of the
actual buffer since double-byte characters are used.

Comment 40

[Roland Mainz]

tor:
OK... point for you... but then
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36385 will make the
"race", does it (e.g. same patch but without patch for nsFontMetricsXP.cpp) ?

I'll fire-up a new bug for that issue...

Comment 41

[Roland Mainz]

timecop:
Wanna r= the fix for nsFontMetricsXP.cpp in patch
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36421 ?

Comment 42

[Roland Mainz]

Attachment: Patch for 2001-05-28-08-trunk (same as previous one + one line in nsFontMetricsXP.cpp changed per timecop's comment)...

Comment 43

[pocemit]

r=timecop (for the nsFontMetricsXP.cpp).
Looks good there.

Comment 44

[tor]

Personally I don't like the name MY_XLIB_RGB_DITHER_XPRINT, but that's
a minor matter.

How far away is the code for doing images properly (bug 57820)?

Comment 45

[Christopher Blizzard (:blizzard)]

-  void               SetDrawingSurface(nsDrawingSurface  aSurface) { mSurface =
aSurface; }
+  void               SetDrawingSurface(nsDrawingSurface  aSurface);

Is that right?  You aren't passing by reference?

+  srcImg = (XImage *)malloc(sizeof(XImage));
+  memset(srcImg, 0, sizeof(XImage));

You don't check the result of srcImg there and you should be using
nsMemory::Alloc() instead of malloc().

+  if( srcImg != nsnull )
+  {
+    if( subImg == nsnull )
+    {
+      srcImg->data = nsnull; /* we do not own this piece of memory */
+      XDestroyImage(srcImg);
+    }
+    else
+    {
+      XDestroyImage(subImg);
+    }  
+  }      

Don't you want to remove that else {} around teh XDestroyImage?  I suspect that
srcImg is leaked if subImg is not null.

Fix that and you have an sr=blizzard

Comment 46

[Roland Mainz]

tor:
> Personally I don't like the name MY_XLIB_RGB_DITHER_XPRINT, but that's
> a minor matter.

Gnnn... I know... I'll replace it with a var or remove it in the future. This
was only a small hack to test code for bug 57820 (turn off dithering to see
possible "pixel blocks" in scaled images)...

> How far away is the code for doing images properly (bug 57820)?

One or two days, excluding weekend. Biggest problem is that I have to patch
xlibrgb.c to get rid of the mad tiling it does (which causes various issues with
X11 stuff - incl. Xprint... ;-( )...
And alpha images are problematic... I wish I could simply create a nsImageXlib()
from incoming nsIImage objects (using those objects directly does not work with
nsImageGTK/nsImageQT objects) and use that for drawing (well, possible... but 
that may cause problems with turning Xprint "on" per default... - reviewers will
love me... =:-) But I would prefer this way as more code get's reused, adds a
better client-side scaling code... but that is not possible for bug 57820
because first the GC-cache (which includes many fixes to nsImageXlib) in
Xlib-toolkit must land - and then I have to port the GC-cache to Xprint...

----

blizzard:
> -  void               SetDrawingSurface(nsDrawingSurface  aSurface) { mSurface
=
> aSurface; }
> +  void               SetDrawingSurface(nsDrawingSurface  aSurface);
> 
> Is that right?  You aren't passing by reference?

-- snip --
mozilla/gfx/src% tgrep SetDrawingSurface | fgrep aSurface | fgrep -v xprint
./ps/nsDeviceContextPS.h :      void                           
SetDrawingSurface(nsDrawingSurface  aSurface) { mSurface = aSurface; }
./motif/nsDeviceContextMotif.h :  void SetDrawingSurface(nsDrawingSurfaceMotif *
aSurface) { mSurface = aSurface; }
./mac/nsDeviceContextMac.h :    void                           
SetDrawingSurface(nsDrawingSurface  aSurface) { mSurface = aSurface; }
-- snip --

Other toolkits do the same... 
I assume that code isn't used anymore...

> +  srcImg = (XImage *)malloc(sizeof(XImage));
> +  memset(srcImg, 0, sizeof(XImage));
> 
> You don't check the result of srcImg there 

Fixed.

> and you should be using
> nsMemory::Alloc() instead of malloc().

I am malloc()'ing that XImage because I feed it to XDestroyImage() later. Using
nsMemory::Alloc() doesn't work here...

> +  if( srcImg != nsnull )
> +  {
> +    if( subImg == nsnull )
> +    {
> +      srcImg->data = nsnull; /* we do not own this piece of memory */
> +      XDestroyImage(srcImg);
> +    }
> +    else
> +    {
> +      XDestroyImage(subImg);
> +    }  
> +  }
> Don't you want to remove that else {} around teh XDestroyImage?  I suspect 
> that srcImg is leaked if subImg is not null.

If (subImg != NULL) then (subImg == srcImg) is TRUE, too - because the original
src was free'ed previously...

> Fix that and you have an sr=blizzard

Thanks, I'll file a new patch...

Comment 47

[Roland Mainz]

Attachment: Patch for 2001-05-28-08-trunk to match sr=blizzard

Comment 48

[Roland Mainz]

Filed new patch to match sr=blizzard ...

Asa:
I need a=drivers/a=asa/etc. that mkaply can check it in..., please...

Comment 49

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to 0.9.1
(on behalf of drivers)

Comment 50

[Roland Mainz]

mkaply:
Could you please checkin the lastest patch (both "0.9.1"+"main" branch) and mark
the bug as "fixed" after checkin, please ?
Thanks !

Comment 51

[Roland Mainz]

timeless:
Could you please checkin the lastest patch (both "0.9.1"+"main" branch) and mark
the bug as "fixed" after checkin, please ?
Thanks !

Comment 52

[timeless]

ok, mkaply committed to branch and I committed to trunk.

Comment 53

[Masaki Katakai]

I'm now verifying the Xprt bugs, however, I'm not sure how to
verify this bug.

Roland,

could you give the exact instruction how to reproduce
this problem? Is it OK when I can print images?

Comment 54

[Roland Mainz]

Verifying this one is easy:
If you see images (correctly scaled) in the print job then everything is OK...
:)

Comment 55

[Masaki Katakai]

Thanks, mark this as VERIFIED.