Bug 689559

Created attachment 3552 [details]
writing vertical text on vertical blue line

Sample file
You must setup your cidfont to provide
Ryumin-Light CIDFont by some CJK TrueType font.

Created attachment 3553 [details]
rendering result by HEAD

Created attachment 3554 [details]
part of Koji Otani patch to fix this issue

part of Koji Otani's patch submitted to bug 689304,
it fixes this issue.

Created attachment 3555 [details]
fixed result by the part of Koji Otani's patch

Now under regression test.

no regression is found.

Created attachment 3561 [details]
Revised Koji Otani's patch

3 trivial remarks are revised.

* gs_glyph_data_free() should be called with client function name.
* implicit typecast in recursive calling of gs_type42_glyph_fbbox()
* if notdef glyph is loaded, 

--

Fix: Wrong centerline of vertical text drawn by CJK TrueType fonts.

DETAILS:
In vertical writing mode, when zchar_get_metrics() returns
successfully but returned value is "metricsNone", the vertical
metric obtained by gs_type42_default_get_metrics() with a flag
gs_type42_metrics_options_WMODE1_AND_BBOX is not reliable.

The metric for vertical writing mode is synthesized from
per-glyph bounding box (pbbox) obtained by new function
gs_type42_glyph_fbbox() and horizontal metric obtained by
gs_type42_default_get_metrics() with a flag
gs_type42_metrics_options_WMODE0_AND_BBOX. Fixes bug 689559.

"pbbox" is filled by calling gs_type42_glyph_fbbox() which
parses the entry of glyf table for given glyph index and
updates both of per-glyph and per-font bounding box.
If per-glyph bounding box is unavailable, per-font bounding
box is used.

EXPECTED DIFFERENCES:
None.

Created attachment 3566 [details]
Re-Revised Koji Otani's patch

Revised to skip calling gs_type42_glyph_fbbox() when writing
mode is not vertical. This function is designed to fix the
bug of metrics in vertical writing mode, calling it always
is unnecessary.

Reviewing the patch 3566 :

[beg quote]
+ * Modified by AXE,Inc., BBR Inc. and Turbolinux Inc.
+ *   under the technical advice by suzuki toshiya (Hiroshima University)
+ * For questions, please send mail to espgs8-cjk@printing-japan.org
+ *
+ * (C) Copyright 2006 Center of the International Cooperation for
+ *     Computerization
[end quote]

This copyright is not acceptable for Artifex releases. Please either submit 
another patch or negotiate with the mentioned enterprises about licensing their 
patch to Artifex. Thanks for understannding.

From the log message :

[beg quote]
"pbbox" is filled by calling gs_type42_glyph_fbbox() which
parses the entry of glyf table for given glyph index and
updates both of per-glyph and per-font bounding box.
[end quote]

This solution must not go to production, because the glyph position depends on 
what glyphs are printed before it ("before" in the CPU time, which may be 
different than the document's text order). Same character may get different 
offsets if another character appears between 2 occurances of same character, as 
in the word "init". The effect will be stablely visible with -dNOCACHE.

This bug was introduced by SVN revision 7567.
SVN revision 7566 was the final revision that can render
the glyphs on correct centerline. I think Otani's patch
was trying to restore original correct behaviour.

>The effect will be stablely visible with -dNOCACHE.

Hmm. According to the log message, SVN revision 7567
changes the metrics handling in glyph cache:
zchar42_set_cache() is changed, although the motivation
of the changeset come not from glyph cache but from
wrong glyph metrics. Why the changeset 7567 modified
zchar42_set_cache()?

Regarding Comment #11: I afraid now I can't add anything to the rev 7567 log. 
It was done for CET complience. Sorry now I can't recall details becauase I 
worked on many other issues after that.

I still think that Otani's patch should not go. A better idea is to unite all 
glyph boxes when the font is loaded. But we don't want to scan all ideograms 
which all have a "standard" bbox, so only a small part of a character 
collection to be scanned. I think CJK specialist can define the part better 
than me. In any case we don't want a sesnible slowdown.

By another hand, likely the font is incorrect, so I'm in doubt whether we must 
handle it at all. CET conformity is preferrable.

Please also pay attention to the change to trunk/gs/doc/pscet_status.txt in rev 
7567. The removed text explains the reason.

Created attachment 3794 [details]
patch to fix vertical metrics utlization in zchar42_set_cache()

For the bug since SVN revision 7567, the problematic part
in zchar42.c::zchar42_set_cache() is following.

-----------------------------------------------------------------

When vertical metrics is found and we setup sbw[] array 

SVN 7566: sbw[] is horizontal, sbw42[] is vertical
	sbw[0] = sbw[2] / 2;
	sbw[1] = (pbfont->FontBBox.q.y + pbfont->FontBBox.p.y - sbw42[3]) / 2;
	sbw[2] = sbw42[2];
	sbw[3] = sbw42[3];
	present = metricsSideBearingAndWidth;
SVN 7567: sbw[]_bbox is vertical
	sbw[0] = sbw_bbox[2] / 2;
	sbw[1] = (pbfont->FontBBox.q.y + pbfont->FontBBox.p.y - sbw_bbox[3]) /
2;
	sbw[2] = sbw_bbox[2];
	sbw[3] = sbw_bbox[3];
	present = metricsSideBearingAndWidth;

-----------------------------------------------------------------

See the part calculating sbw[0] when vertical metrics is
available. In SVN revision 7566, sbw[0] is always calculated
from horizontal metrics. Since 7567, if vertical metrics is
found, sbw[0] is calculated from it. If vertical sbw[0] is
calculated from horizontal metrics always, the bug disappears.
Attached is the patch to do that.

In the patch (and SVN revision 7566), always horizontal metrics
is used. The utilization of sbw[] and sbw_bbox[] in SVN revision
7566 zchar42.c is too complicated, I introduced explicitly-named
variables hsbw_bbox[], vsbw_bbox[] (storage for metric/bbox infos
for each writing modes) and hcode, vcode (metrics availability for
each writing modes) for easier maintenance.

Index: src/zchar42.c
===================================================================
--- src/zchar42.c	(revision 8527)
+++ src/zchar42.c	(working copy)
@@ -40,76 +40,71 @@
	    uint glyph_index, op_proc_t cont, op_proc_t *exec_cont, bool
put_lsb)
 {   double sbw[4];
     double w[2];
-    int present;
+    int present = zchar_get_metrics(pbfont, cnref, sbw);
     gs_font_type42 *pfont42 = (gs_font_type42 *)pbfont;
-    int code = zchar_get_metrics(pbfont, cnref, sbw);
     gs_rect bbox;
     int vertical = gs_rootfont(igs)->WMode;
-    float sbw_bbox[8];
+    int hcode, vcode;
+    float hsbw_bbox[8], vsbw_bbox[8];
 
-    if (code < 0)
-	return code;
-    present = code;
+    /* horizontal metrics is always required */
+    hcode = pfont42->data.get_metrics(pfont42, glyph_index, 
+	      gs_type42_metrics_options_WMODE0_AND_BBOX, hsbw_bbox);
+    if (hcode < 0)
+	 return hcode;
+
     if (vertical) { /* for vertically-oriented metrics */
-	code = pfont42->data.get_metrics(pfont42, glyph_index, 
-		gs_type42_metrics_options_WMODE1_AND_BBOX, sbw_bbox);
-	if (code < 0) { 
+	vcode = pfont42->data.get_metrics(pfont42, glyph_index, 
+		gs_type42_metrics_options_WMODE1_AND_BBOX, vsbw_bbox);
+	if (vcode < 0) { 
	    /* No vertical metrics in the font, compose from horizontal. 
	       Still need bbox also. */
-	    code = pfont42->data.get_metrics(pfont42, glyph_index, 
-		    gs_type42_metrics_options_WMODE0_AND_BBOX, sbw_bbox);
-	    if (code < 0)
-		return code;
	    if (present == metricsNone) {
		if (pbfont->FontType == ft_CID_TrueType) {
-		    sbw[0] = sbw_bbox[2] / 2;
+		    sbw[0] = hsbw_bbox[2] / 2;
		    sbw[1] = pbfont->FontBBox.q.y;
		    sbw[2] = 0;
		    sbw[3] = pbfont->FontBBox.p.y - pbfont->FontBBox.q.y;
		} else {
-		    sbw[0] = sbw_bbox[0];
-		    sbw[1] = sbw_bbox[1];
-		    sbw[2] = sbw_bbox[2];
-		    sbw[3] = sbw_bbox[3];
+		    sbw[0] = hsbw_bbox[0];
+		    sbw[1] = hsbw_bbox[1];
+		    sbw[2] = hsbw_bbox[2];
+		    sbw[3] = hsbw_bbox[3];
		}
		present = metricsSideBearingAndWidth;
	    }
	} else {
	    if (present == metricsNone) {
-		sbw[0] = sbw_bbox[2] / 2;
-		sbw[1] = (pbfont->FontBBox.q.y + pbfont->FontBBox.p.y -
sbw_bbox[3]) / 2;
-		sbw[2] = sbw_bbox[2];
-		sbw[3] = sbw_bbox[3];
+		sbw[0] = hsbw_bbox[2] / 2;
+		sbw[1] = (pbfont->FontBBox.q.y + pbfont->FontBBox.p.y -
vsbw_bbox[3]) / 2;
+		sbw[2] = vsbw_bbox[2];
+		sbw[3] = vsbw_bbox[3];
		present = metricsSideBearingAndWidth;
	    }
	}
     } else {
-	code = pfont42->data.get_metrics(pfont42, glyph_index, 
-		    gs_type42_metrics_options_WMODE0_AND_BBOX, sbw_bbox);
-	if (code < 0)
-	    return code;
	if (present == metricsNone) {
-	    sbw[0] = sbw_bbox[0];
-	    sbw[1] = sbw_bbox[1];
-	    sbw[2] = sbw_bbox[2];
-	    sbw[3] = sbw_bbox[3];
+	    sbw[0] = hsbw_bbox[0];
+	    sbw[1] = hsbw_bbox[1];
+	    sbw[2] = hsbw_bbox[2];
+	    sbw[3] = hsbw_bbox[3];
	    present = metricsSideBearingAndWidth;
	}
     }
     w[0] = sbw[2];
     w[1] = sbw[3];
     if (!vertical) {
-	sbw_bbox[6] = (sbw_bbox[6] - sbw_bbox[4]) + sbw_bbox[0];
-	sbw_bbox[4] = sbw_bbox[0];
+	hsbw_bbox[6] = (hsbw_bbox[6] - hsbw_bbox[4]) + hsbw_bbox[0];
+	hsbw_bbox[4] = hsbw_bbox[0];
     }
     /* Note: The glyph bbox usn't useful for Dynalab fonts,
	which stretch subglyphs. Uniting with FontBBox helps.
	In same time, FontBBox with no glyph bbox
	doesn't work for 34_all.PS page 4. */
-    bbox.p.x = min(sbw_bbox[4], pbfont->FontBBox.p.y);
-    bbox.p.y = min(sbw_bbox[5], pbfont->FontBBox.p.y);
-    bbox.q.x = max(sbw_bbox[6], pbfont->FontBBox.q.x);
-    bbox.q.y = max(sbw_bbox[7], pbfont->FontBBox.q.y);
+    bbox.p.x = min(vertical ? vsbw_bbox[4] : hsbw_bbox[4],
pbfont->FontBBox.p.y);
+    bbox.p.y = min(vertical ? vsbw_bbox[5] : hsbw_bbox[5],
pbfont->FontBBox.p.y);
+    bbox.q.x = max(vertical ? vsbw_bbox[6] : hsbw_bbox[6],
pbfont->FontBBox.q.x);
+    bbox.q.y = max(vertical ? vsbw_bbox[7] : hsbw_bbox[7],
pbfont->FontBBox.q.y);
     return zchar_set_cache(i_ctx_p, pbfont, cnref,
			   (put_lsb && present == metricsSideBearingAndWidth ?
			    sbw : NULL),

Changing assignment for checking the patch.

Created attachment 4268 [details]
patch.txt

A suggested patch for this bug and for 688058. It is being tested now. Its
commitment to be postponed until gs/src reorganization.

The patch 4268 passed local test. he comment #14 patch must not go due to 
overcomplicated.

Patch to HEAD :
http://ghostscript.com/pipermail/gs-cvs/2008-August/008552.html