415522 - [OS/2] Update Printing Support

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[Peter Weilbacher]

Follow-up from bug 394412: printing works when printing to PDF and to low resolution printers (like the PMFax driver).

Printing to "normal" printers (with resolutions >~ 300 dpi) is slow, uses huge amounts of RAM (much more than expected for the surface sizes created), and often also crashes in PMGPI.

Solution is probably to create a new printing surface for OS/2 in the cairo library, the way it is done for Windows. Doodle said he will work on that.

Comment 1

[Peter Weilbacher]

At least I found out that the crash comes from the fallback call of GpiDrawBits() in cairo
   http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/cairo/cairo/src/cairo-os2-surface.c&rev=1.17&mark=413-419#406

But I have no idea why it crashes and what I could do about it. When I debugged this last night, it crashed only for the printer using the OMNI driver while it worked correctly for LASERJET, PSCRIPT, PSCRIPT2, and PMFax/FXPRINT drivers. So resolution doesn't seem to matter...

So the solution would really be an adapted surface type that can handle printers better and doesn't use a time-consuming 32bit call to GpiDrawBits that fails.

Comment 2

[Peter Weilbacher]

Despite lots of debugging I haven't found a fix. I tried scaling the size of the pixel surface in nsDeviceContextSpecOS2::GetSurfaceForPrinter but that then just resulted in more output pages and still led to the same crash.

The only way I currently see, is to force print output to a PDF file.

Comment 3

[Peter Weilbacher]

Attachment: [checked in] workaround

Like this. This by default creates a file like <APPNAME>_YYYYMMDD_hhmmss.pdf (including the date to make it reasonably unique) on the Desktop. If the user selects "Print to File" in the print dialog it will use that instead.

Comment 4

[Peter Weilbacher]

Comment on attachment 322181 [details] [diff] [review]
[checked in] workaround

Checked this into CVS trunk.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Peter, what about a review before checking in patches into /widget? No need for that for OS/2 patches?

Comment 6

[Peter Weilbacher]

For stuff that is about cairo-os2 I never found a reviewer who wanted to wrap his brain around Thebes _and_ OS/2, so Stuart allowed me to check that stuff in unreviewed. Stuff that is in OS/2-only files or directories is NPOTB so it doesn't hurt anyone. Stuff that is contained to widget is normally reviewed my Mike Kaply, but as this workaround is the effect of our Thebes breakage I didn't want to bother him...

If somebody wants to volunteer for future patches of this kind or has post-checkin comments, I would be happy to improve my changes!

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Ah ok, then it's fine. Thanks for your answer.

Comment 8

[Peter Weilbacher]

I'll probably not be able to work on any bugs in the near future.

Comment 9

[Rich Walsh]

(In reply to comment #8)
> I'll probably not be able to work on any bugs in the near future.

Before you disappear into the sunset, could you direct me to the relevant bugs, patches, etc, that might enable me to get printing to work properly?

Comment 10

[Peter Weilbacher]

I'll at least be reading bugmail for the foreseeable future...

I don't remember if there are other bugs about printing. The latest stage of the patches which I used to let Mozilla apps print to PS printers are in my Hg repo at <http://hg.mozilla.org/users/mozilla_weilbacher.org/m-c_Mq/file/a27a31dcb0df>:
The relevant files are probably

   possible_printing_improvements.diff
   bug415522_print_changes.diff
   bug415522_print_pscript_all_js.diff
   bug415522_print_pscript_improvements.diff
   bug415522_print_pscript_remove_other_stuff.diff
   bug415522_print_pscript_reorg.diff
   bug415522_print_pscript_via_file.diff

By now I have forgotten what each patch did. And they were not well separated anyway or should I rather say: they were a complete mess. ;-)

Comment 11

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 12

[Rich Walsh]

The patches that follow add full support for printing via CUPS and provisional support for printing directly to Postscript printers.

They rely on Cairo's ability to create Postscript output and use the IBMNULL driver to pass that output to the spooler unmodified.  While they don't use native drivers to format output, they do rely on them to identify which printer queues are capable of receiving PS output and to provide parameters needed to format each job.  For queues that don't support PS, these patches create a PDF file, as is currently done.

This update also adds a few niceties:  the ability to specify a PDF's name and path;  the ability to set the output format for a file by setting its extension in the Print to File dialog;  and the elimination of empty files that were created when using Print Preview.

Note:  In Phase I of this project, support for printing directly to PS printers is deemed "provisional" because some printers appear incapable of interpreting Cairo's PS output while others have no problem with it.  Phase II, which will implement native driver support, should eliminate this problem.

Comment 13

[Rich Walsh]

Attachment: print-gfx - update configure.in & files in gfx tree

Comment 14

[Rich Walsh]

Attachment: print-wdgt - update files in widget/src/os2 except nsDeviceContextSpec

Comment 15

[Rich Walsh]

Attachment: print-spec - update nsDeviceContextSpecOS2.*

Comment 16

[Rich Walsh]

Attachment: print-debug - debugging printf's - not for checkin

I'd suggest OS/2 builders include this in their unofficial distros until we're quite sure everything works as expected.

Comment 17

[Peter Weilbacher]

Comment on attachment 474474 [details] [diff] [review]
print-wdgt - update files in widget/src/os2 except nsDeviceContextSpec

This looks good, too. (nsPrintOS2.cpp needs to be reformatted to the standard format, but you shouldn't really waste time to do that beyond what you already did.)

Comment 18

[Peter Weilbacher]

Comment on attachment 474476 [details] [diff] [review]
print-spec - update nsDeviceContextSpecOS2.*

Very nice, _much_ cleaner and more capable than my draft implementation from a year ago. Unfortunately, I don't have the end result of my old set of patches handy to cross-check some details nor am I near an OS/2 machine to test any of this, but what I understand is great.

Some small details:

>+  // Determine if this is a raster or Postscript printer.
>+  long driverType;
>+  if (!DevQueryCaps(hdc, CAPS_TECHNOLOGY, 1, &driverType))
>+    driverType = 0;

IIRC my testing showed this to not always be reliable. That's why I added the print.os2.supported.postscript pref. But maybe I just misremember or did something wrong back then.

>+  }
>+  else
>+  if (StringEndsWith(aFileName, NS_LITERAL_STRING(".ps"),

Mozilla coding style is
    } else if (...) {

>+//---------------------------------------------------------------------------
>+// Helper function to convert the string to the native codepage,
>+// similar to UnicodeToCodepage() in nsDragService.cpp.
>+
>+static
>+char* GetACPString(const PRUnichar* aStr)
>+{
>+   nsString str(aStr);
>+   if (str.Length() == 0) {
>+      return nsnull;
>+   }
>+
>+   nsAutoCharBuffer buffer;
>+   PRInt32 bufLength;
>+   WideCharToMultiByte(0, PromiseFlatString(str).get(), str.Length(),
>+                       buffer, bufLength);
>+   return ToNewCString(nsDependentCString(buffer.Elements()));
>+}

I haven't really found out why this appears in the patch at all. This isn't new code, right?

>+nsresult nsDeviceContextSpecOS2::SetPrintSettingsFromDevMode(nsIPrintSettings* aPrintSettings, ULONG printer)

Break this long line, please.

>+  int bufferSize = 3 * sizeof(DJP_ITEM);
>+  PBYTE pDJP_Buffer = new BYTE[bufferSize];
>+  memset(pDJP_Buffer, 0, bufferSize);

Is there an advantage of new[]+memset() over calloc()?

>+  //Get Number of Copies from Job Properties

Add a space after the // (similar for the following)? (Yes, I know this was not written by you, but as it and the following appears in the patch, you might as well fix it.)

>+  LONG rc = GreEscape(hdc, DEVESC_QUERYJOBPROPERTIES, bufferSize, pDJP_Buffer,
>+                      &driverSize, PBYTE(nsDeviceContextSpecOS2::PrnDlg.GetPrintDriver(printer)));

Please try to make it shorter. (I think 80 char/line are still the official number, even though it is hard to follow sometimes...)

>+        if ((pDJP->ulValue == DJP_ORI_PORTRAIT) || (pDJP->ulValue == DJP_ORI_REV_PORTRAIT))

Again length...

>-    GreEscape (hdc, DEVESC_SETJOBPROPERTIES, bufferSize, pDJP_Buffer, 
>+    GreEscape (hdc, DEVESC_SETJOBPROPERTIES, bufferSize, pDJP_Buffer,
>                &driverSize, PBYTE(nsDeviceContextSpecOS2::PrnDlg.GetPrintDriver(printer)));

While you are here remove the space before the parenthesis and try to make the second line shorter.

>-  if (NS_FAILED(rv)) {
>+  if (NS_FAILED(rv))
>     return rv;
>-  }

Against coding style. ;-)

>-   if (!*aPrinterName) 
>+   if (!*aPrinterName)
>      return NS_OK;
> 
>-  if (NS_FAILED(GlobalPrinters::GetInstance()->InitializeGlobalPrinters())) 
>+  if (NS_FAILED(GlobalPrinters::GetInstance()->InitializeGlobalPrinters()))
>     return NS_ERROR_FAILURE;

If you want you can add the { } to follow the style guide... Same goes for some other instances further down.

>   ULONG numPrinters = GlobalPrinters::GetInstance()->GetNumPrinters();
>   for(ULONG i = 0; i < numPrinters; i++) {
>-    if ((GlobalPrinters::GetInstance()->GetStringAt(i)->Equals(aPrinterName, nsCaseInsensitiveStringComparator()))) 
>+    if ((GlobalPrinters::GetInstance()->GetStringAt(i)->Equals(aPrinterName, nsCaseInsensitiveStringComparator())))
>       nsDeviceContextSpecOS2::SetPrintSettingsFromDevMode(aPrintSettings, i);

Long lines...

>+nsresult os2SpoolerStream::Init(PRTQUEUE* aQueue, const char* aTitle)
>+{
>+  ULONG   rc;
>+  ULONG   ulSize = 0;
>+
>+  // Sending Moz's Postscript output through the native PS driver, even in raw
>+  // format, may corrupt it and make it unusable.  To work around this, we send
>+  // it through IBMNULL which won't change anything.  However, IBMNULL must be
>+  // associated with the target printer, and it's unlikely that the user would
>+  // have the printer setup that way.  So... this adds IBMNULL to the printer's
>+  // list of drivers.

This idea is amazing. Wouldn't ever have thought of that! Did you discover this purpose of IBMNULL by chance or is that documented somewhere?

Comment 19

[Peter Weilbacher]

Comment on attachment 474479 [details] [diff] [review]
print-debug - debugging printf's - not for checkin

I would actually suggest to check this in, too, at least so that it can be activated easiy. It's not too intrusive while reading the code and it might come in as helpful later.

Comment 20

[Rich Walsh]

Attachment: native printing support - gfx tree

Comment 21

[Rich Walsh]

Attachment: native printing support - widget/os2

Comment 22

[Rich Walsh]

Attachment: native printing support - nsDeviceContextSpecOS2

Comment 23

[Rich Walsh]

Attachment: native printing support - debug printf's - not for checkin

Comment 24

[Rich Walsh]

These new patches provide bitmap-based printing support using native drivers.

Cairo's PS support can still be used by adding this boolean preference:
  "print.os2.postscript.use_builtin"
and setting it to "true".

These patches depend on those attached to Bug 609196 (Revise Cairo and Thebes).

Comment 25

[Rich Walsh]

Attachment: native printing support - gfx tree -v2

makes trivial changes in configure.in

Comment 26

[Rich Walsh]

Attachment: native printing support - debug printf()s v2 - not for checkin

Comment 27

[Wayne Mery (:wsmwk)]

OS/2 is no longer a supported platform.