412165 - [BeOS] Postscript printing is ignorant about GFXFORMAT in images

Status: RESOLVED WONTFIX

(Core :: Printing: Output, defect)

Description

[Sergei Dolgov]

In non-Cairo version of Postscript-printing nsPostScriptObj::draw_image
makes two too free assumptions:
1)"transparent" pixel-bytes with alphadepth = 1 were already whitened somewhere else (and pointing there to GTK code in nsImageGTK, while that whitening don't happen in LockImageBits in other toolkits!
http://lxr.mozilla.org/mozilla1.8/source/gfx/src/ps/nsPostScriptObj.cpp#2425

2) Byte-order in Mozilla device-indepent image format is expected to always be RGB.
http://lxr.mozilla.org/mozilla1.8/source/gfx/src/ps/nsPostScriptObj.cpp#2444

first assumption can be workarounded with hacks,

second is really bad - because BGR-order is set at libpr0n level for WIN, OS_2 and BeOS for example.


As expected, this lazy approach results in swapped R and B in BeOS postscript output.

Comment 1

[Sergei Dolgov]

Attachment: patch, swaps R and B for BeOS

will do fix for whitening in nsImageBeOS - that's quite safe to do it there.
So, only correct RED and BLUE order fix here.
As no complaints from WIN and OS_2 - for BeOS-only.

Actually that's a problem that every decoder in libpr0n determines and defines RGB versus BGR in its' own way, instead using global define or API. But nothing that I can do with that.

first review request - from BeOS tester.

Comment 2

[Doug Shelton]

Comment on attachment 296826 [details] [diff] [review]
patch, swaps R and B for BeOS

Patch is malformed: 
+#endif      if (outputCount >= 72) {
needs new line between "#endif" and "if".  Won't build as submitted.  Logic itself is fine.  I fixed this patch problem, built and tested.  Red and Blue are now displayed correctly in BeOS Postscript output.

Comment 3

[Sergei Dolgov]

Attachment: patch

same as previous, but with proper newline
r=?

Comment 4

[Doug Shelton]

Comment on attachment 296934 [details] [diff] [review]
patch

sergei, are other approvals required before landing on branch?  If not, requesting permission.

Comment 5

[Sergei Dolgov]

Doug, I'm unsure in this case. Inspite patch code changes program behaviour for BeOS only, it is in common (for several platforms) file. And also is related general issue with dependency of "independent" format on  platform.
So it will be good if some printing and postscript guru/peer has look on this code as second review.


From other side, this is irrelevant for current trunk, AFAIK - no such code there, and postscript printing is generated from Cairo. 

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 296934 [details] [diff] [review]
patch

In the non-BEOS code this changes the mPrintSetup->color case:
 - outputCount is no longer incremented
 - uses pixel[] instead of p[]

Is there a non-BEOS bug describing what you're fixing here? The latter worries me in particular since it appears to be backing out bug 368754. Please get r=tor and/or kherron on this change before requesting branch approval. I have no idea what difference the outputCount change makes.

Comment 7

[Sergei Dolgov]

(In reply to comment #6)
> (From update of attachment 296934 [details] [diff] [review])
> In the non-BEOS code this changes the mPrintSetup->color case:
>  - outputCount is no longer incremented
>  - uses pixel[] instead of p[]
> 
> Is there a non-BEOS bug describing what you're fixing here? The latter worries
> me in particular since it appears to be backing out bug 368754. Please get
> r=tor and/or kherron on this change before requesting branch approval. I have
> no idea what difference the outputCount change makes.
> 
This should not change anything (by idea at least) in non-BeOS case. Code here in local sources before patch:
       if (mPrintSetup->color)
         outputCount += fprintf(mScriptFP, "%02x%02x%02x", p[0], p[1], ]);
       else
         outputCount +=
           fprintf(mScriptFP, "%02x", NS_RGB_TO_GRAY(p[0], p[1], p[2]));
       if (outputCount >= 72) {
         fputc('\n', mScriptFP);
         outputCount = 0;
       }
code after patch:
#else
      if (mPrintSetup->color)
          fprintf(mScriptFP, "%02x%02x%02x", pixel[0], pixel[1], pixel[2]);
      else
        outputCount +=
          fprintf(mScriptFP, "%02x", NS_RGB_TO_GRAY(p[0], p[1], p[2]));
#endif
      if (outputCount >= 72) {
        fputc('\n', mScriptFP);
        outputCount = 0;
      }

So probably I will retest it again - with applying given patch to get why it looks so suspicious. I made given diff from file which desn't drop outputCount incrementation and so on...

Comment 8

[Sergei Dolgov]

I wonder myself, what happens in win-compatible platforms here.
OS/2 has stubs in place of Lock/UnlockImagePixels, so if it uses Postscript printing from nsIImage pixmaps, it either do it some other place or does it its own special way.

Maybe they just don't care, as previously color postscript printers were too rare, and now trunk uses Cairo, not this code?

Comment 9

[tor]

Comment on attachment 296934 [details] [diff] [review]
patch

>Index: mozilla/gfx/src/ps/nsPostScriptObj.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/ps/Attic/nsPostScriptObj.cpp,v
>retrieving revision 1.124.2.1
>diff -u -p -4 -r1.124.2.1 nsPostScriptObj.cpp
>--- mozilla/gfx/src/ps/nsPostScriptObj.cpp	6 Apr 2007 16:24:57 -0000	1.124.2.1
>+++ mozilla/gfx/src/ps/nsPostScriptObj.cpp	14 Jan 2008 11:00:05 -0000
>@@ -2439,13 +2439,23 @@ nsPostScriptObj::draw_image(nsIImage *an
>         MOZ_BLEND(p[1], 255, pixel[1], alpha);
>         MOZ_BLEND(p[2], 255, pixel[2], alpha);
>       }
> 
>+// See Bug 412165. As no blame from Win and OS_2, swapping bytes for BeOS-only
>+// Do it at fprint stage for compatibility with 1.9 code
>+#if defined(XP_BEOS)
>       if (mPrintSetup->color)
>-        outputCount += fprintf(mScriptFP, "%02x%02x%02x", p[0], p[1], p[2]);
>+          fprintf(mScriptFP, "%02x%02x%02x", pixel[2], pixel[1], pixel[0]);

You lost the "outputCount +=".

>+      else
>+        outputCount +=
>+          fprintf(mScriptFP, "%02x", NS_RGB_TO_GRAY(p[2], p[1], p[0]));
>+#else
>+      if (mPrintSetup->color)
>+          fprintf(mScriptFP, "%02x%02x%02x", pixel[0], pixel[1], pixel[2]);

Ditto.

>       else
>         outputCount +=
>           fprintf(mScriptFP, "%02x", NS_RGB_TO_GRAY(p[0], p[1], p[2]));
>+#endif
>       if (outputCount >= 72) {
>         fputc('\n', mScriptFP);
>         outputCount = 0;
>       }

Comment 10

[Sergei Dolgov]

well, will correct that patch ASAP,
but unfortunately situation with other two OS-es which use BGR order for nsImage (Win and OS/2) is still mistery for me. Pure theoretical interest, but anyway.

Comment 11

[Doug Shelton]

good catch on the "outputCount", t_rowley.  Thanks!

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to comment #11)
> good catch on the "outputCount", t_rowley.  Thanks!

That was my first point in comment 6.

Tor: you were silent on the issue of switching from p[] back to pixel[] in the color case, my primary concern. I'd like an explicit OK on that please.

Comment 13

[tor]

(In reply to comment #12)
> Tor: you were silent on the issue of switching from p[] back to pixel[] in the
> color case, my primary concern. I'd like an explicit OK on that please.

Sorry, missed your comment when skimming the bug originally.  You are right, the switch from p[] to pixel[] is a mistake.

Comment 14

[Wayne Mery (:wsmwk)]

Sergei/Anyone  Can land/close in the next week, before Gerv's mailnews component reorg?

[this is practically the only usefully assigned mailnews: printing bug]

Comment 15

[Sergei Dolgov]

(In reply to comment #14)
> Sergei/Anyone  Can land/close in the next week, before Gerv's mailnews
> component reorg?
> 
> [this is practically the only usefully assigned mailnews: printing bug]
> 
Will try to submit new patch tomorrow.
I thought about cleaning senseless for BeOS BGR order in all libpr0n components by removing BeOS ifdefs from there (as actually we use here only 32-bit pixels, so no benefits from using this byte triplet in "proper" order) - but it seems too troublesome, so probably will workaround that inconsistency here. If not, will close that bug and fix that in BeOS-only code in gfx/src/beos

Comment 16

[Sergei Dolgov]

changing component, wondering how it happen to be just MailNews printing

Comment 17

[Sergei Dolgov]

Attachment: safe patch

I think this version is absolutely safe, though it adds a bit of code for BeOS case.

Unfortunately I have now troubles with mozilla CVS, so patch is in simplest form.

Comment 18

[Murali Nandigama [:murali]]

You have a patch on this bug that is flagged for 'review?' and not assigned to any reviewer. If you want the patch to be reviewed please assign a reviewer. Thanks

Comment 19

[Sergei Dolgov]

(In reply to comment #18)
> You have a patch on this bug that is flagged for 'review?' and not assigned to
> any reviewer. If you want the patch to be reviewed please assign a reviewer.
> Thanks

I got impression that branch is almost totally frozen except for security patches, thus didn't try from freezing moment to put any patch through.

Comment 20

[Mats Palmgren (inactive)]

Sorry, but BeOS support has been removed from mozilla-central.