Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8255387: Japanese characters were printed upside down on AIX #1218

Closed
wants to merge 6 commits into from

Conversation

toshiona
Copy link
Contributor

@toshiona toshiona commented Nov 16, 2020

Hi,

Could you review this fix? Under some special conditions, non-English characters were printed upside down.

At printing with raster image, the image was generated from bottom to top. So, each characters should also be drawn as vertical mirrored. However, freetype doesn't support to transform it if the font is using embedded bitmap and non-English. As the result, these Japanese characters were printed as upside down.

In this case, freetype should be prevented to use embedded bitmap.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8255387: Japanese characters were printed upside down on AIX

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1218/head:pull/1218
$ git checkout pull/1218

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 16, 2020

👋 Welcome back tnakamura! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Nov 16, 2020

@toshiona The following labels will be automatically applied to this pull request:

  • 2d
  • awt

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added 2d client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org labels Nov 16, 2020
@toshiona toshiona marked this pull request as ready for review Nov 16, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 16, 2020
@mlbridge
Copy link

mlbridge bot commented Nov 16, 2020

Webrevs

import java.awt.geom.AffineTransform;
import java.awt.image.BufferedImage;

public class PrintTranslateTest{
Copy link
Member

@mrserb mrserb Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really bind to the printing/"print/PrinterJob", look like the bug is reproduced in the common rendering as well?

Copy link
Contributor Author

@toshiona toshiona Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Sergey for your advice. I moved the test case under test/jdk/java/awt/font and changed the name. The original problem is at printing, but yes it's common rendering.

@openjdk openjdk bot removed the awt client-libs-dev@openjdk.org label Nov 20, 2020
@victordyakov
Copy link

victordyakov commented Nov 30, 2020

@mrserb @prrace please review

/*
* @test PrintTranslateTest
* @bug 8255387
* @summary Vertial mirrored characters should be drawn correctly
Copy link
Contributor

@prrace prrace Nov 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vertial -> Vertical

Copy link
Contributor Author

@toshiona toshiona Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, changed the summary description.

import java.awt.image.BufferedImage;

public class PrintTranslateTest{
static String target = "\u3042";
Copy link
Contributor

@prrace prrace Nov 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a space before {

Copy link
Contributor Author

@toshiona toshiona Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


static BufferedImage drawNormal(Font font) {
BufferedImage image = new BufferedImage(SIZE, SIZE,
BufferedImage.TYPE_BYTE_BINARY);
Copy link
Contributor

@prrace prrace Nov 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose you chose BYTE_BINARY so that AA glyphs have to rendered as mono making the comparison easier ?

Copy link
Contributor Author

@toshiona toshiona Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@@ -529,7 +529,8 @@ Java_sun_font_FreetypeFontScaler_createScalerContextNative(
*/
if ((aa != TEXT_AA_ON) && (fm != TEXT_FM_ON) &&
!context->doBold && !context->doItalize &&
(context->transform.yx == 0) && (context->transform.xy == 0))
(context->transform.yx == 0) && (context->transform.xy == 0) &&
(context->transform.yy > 0))
Copy link
Contributor

@prrace prrace Nov 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what about xx ? Mirroring horizontally ?

Copy link
Contributor Author

@toshiona toshiona Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the advice. Updated the patch and test to cover horizontal mirror case.

prrace
prrace approved these changes Dec 1, 2020
@openjdk
Copy link

openjdk bot commented Dec 1, 2020

@toshiona This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8255387: Japanese characters were printed upside down on AIX

Reviewed-by: prr, serb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 451 new commits pushed to the master branch:

  • 0a3e446: 8257993: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine/TestDescription.java crash intermittently
  • 46c9a86: 8245956: JavaCompiler still uses File API instead of Path API in a specific case
  • baf4c1a: 8257917: [JVMCI] crash when materializing boxed values under -Xcomp
  • fd5f6e2: 8257986: [JVMCI] ProblemList 2 reprofile JVMCI tests
  • 30de320: 6882207: Convert javap to use diamond operator internally
  • d33a689: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
  • e6b4c4d: 8238781: [macos] jpackage tests failed due to "hdiutil: convert failed" in various ways
  • b977a7b: 8257989: Error in gtest os_page_size_for_region_unaligned after 8257588
  • 5f03341: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue
  • 6dd06ad: 8254996: make jdk.net.UnixDomainPrincipal a record class
  • ... and 441 more: https://git.openjdk.java.net/jdk/compare/1e9a432d59fa3fb3d038c83c88fd7eeb3052960c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prrace, @mrserb) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 1, 2020
@prrace
Copy link
Contributor

prrace commented Dec 1, 2020

Approved.
Whilst the fix looks reasonable and important for on-screen, I am surprised it was an issue when printing.
At printer resolution it is really surprising that there are any bitmaps for the glyphs.
We should be talking about a 300 dpi printer minimum these days and so we should be requesting at least 50 pixel
high text for even 12pt nomimal size. Fonts usually don't have bitmaps at that size because they are large and less important.
So I am not sure under what circumstances you saw it.

Secondly, I think that unless we were going down the raster path (which I think you said we were) that we normally explicitly used filled shapes for CJK.

@mrserb
Copy link
Member

mrserb commented Dec 1, 2020

The testing is in progress.

@mrserb
Copy link
Member

mrserb commented Dec 2, 2020

The new test fails on the headless macs(I cannot reproduce it on my laptop):

----------System.out:(1/17)----------
ErrorCount:53,53
----------System.err:(13/1055)----------
java.lang.RuntimeException: Incorrect mirrored character with java.awt.Font[family=AppleMyungjo,name=AppleMyungjo,style=plain,size=12]
at MirrorTest.main(MirrorTest.java:147)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:567)
at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:831)

Do we need to mark it as headful, or it might be a product bug?

@toshiona
Copy link
Contributor Author

toshiona commented Dec 7, 2020

Apologize for the trouble. That's my testcase issue.
That error may occur on macOS High Sierra. Newer OSes (Mojave, Catalina, and Big Sur) may not have the problem. Differences between antialias on and off are bigger than the real error which I'd like to detect.
So, I revised the test to compare the center of gravity of each glyph.
Could you kindly re-review and retest this?

mrserb
mrserb approved these changes Dec 9, 2020
Copy link
Member

@mrserb mrserb left a comment

An updated test works fine.

@victordyakov
Copy link

victordyakov commented Dec 9, 2020

@toshiona you can push it as you have 2 approvals. As a reminder today is a last day for fixing P4s in jdk 16

@toshiona
Copy link
Contributor Author

toshiona commented Dec 9, 2020

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 9, 2020
@openjdk
Copy link

openjdk bot commented Dec 9, 2020

@toshiona
Your change (at version b6fd857) is now ready to be sponsored by a Committer.

@toshiona
Copy link
Contributor Author

toshiona commented Dec 9, 2020

@mrserb @prrace
Thank you so much for the review. Could you sponsor this patch?

@prrace
Copy link
Contributor

prrace commented Dec 10, 2020

/integrate

@openjdk
Copy link

openjdk bot commented Dec 10, 2020

@prrace Only the author (@toshiona) is allowed to issue the integrate command. As this PR is ready to be sponsored, and you are an eligible sponsor, did you mean to issue the /sponsor command?

@prrace
Copy link
Contributor

prrace commented Dec 10, 2020

/sponsor

@openjdk openjdk bot closed this Dec 10, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 10, 2020
@openjdk
Copy link

openjdk bot commented Dec 10, 2020

@prrace @toshiona Since your change was applied there have been 451 commits pushed to the master branch:

  • 0a3e446: 8257993: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine/TestDescription.java crash intermittently
  • 46c9a86: 8245956: JavaCompiler still uses File API instead of Path API in a specific case
  • baf4c1a: 8257917: [JVMCI] crash when materializing boxed values under -Xcomp
  • fd5f6e2: 8257986: [JVMCI] ProblemList 2 reprofile JVMCI tests
  • 30de320: 6882207: Convert javap to use diamond operator internally
  • d33a689: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
  • e6b4c4d: 8238781: [macos] jpackage tests failed due to "hdiutil: convert failed" in various ways
  • b977a7b: 8257989: Error in gtest os_page_size_for_region_unaligned after 8257588
  • 5f03341: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue
  • 6dd06ad: 8254996: make jdk.net.UnixDomainPrincipal a record class
  • ... and 441 more: https://git.openjdk.java.net/jdk/compare/1e9a432d59fa3fb3d038c83c88fd7eeb3052960c...master

Your commit was automatically rebased without conflicts.

Pushed as commit 53e537c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2d client-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants