Skip to content
This repository has been archived by the owner. It is now read-only.

JDK-8217605: Add support for e-paper displays #369

Merged
merged 7 commits into from Apr 16, 2019
Merged

JDK-8217605: Add support for e-paper displays #369

merged 7 commits into from Apr 16, 2019

Conversation

@jgneff
Copy link
Contributor

@jgneff jgneff commented Feb 4, 2019

Closes #339

jgneff added 2 commits Feb 4, 2019
Fixes the Travis CI build failure:

--- Checking for white space errors
modules/javafx.graphics/src/main/native-glass/monocle/epd/mxcfb.h :tabs:
Found 1 whitespace or executable issues
Error in white space check
@jgneff
Copy link
Contributor Author

@jgneff jgneff commented Feb 4, 2019

My new unit tests succeeded, but the following unrelated test failed:

test.javafx.concurrent.ServiceLifecycleTest > cancelCalledFromOnSucceeded FAILED
    java.lang.AssertionError: expected:<SUCCEEDED> but was:<CANCELLED>
    at test.javafx.concurrent.ServiceLifecycleTest
        .cancelCalledFromOnSucceeded(ServiceLifecycleTest.java:1176)

The test case is:

@Test public void cancelCalledFromOnSucceeded() {
    final AtomicInteger cancelNotificationCount = new AtomicInteger();
    service.addEventFilter(WorkerStateEvent.WORKER_STATE_SUCCEEDED, workerStateEvent -> {
        service.cancel();
    });
    service.addEventFilter(WorkerStateEvent.WORKER_STATE_CANCELLED, event -> {
        cancelNotificationCount.incrementAndGet();
    });

    service.start();
    executor.executeScheduled();
    task.complete();
    assertEquals(Worker.State.SUCCEEDED, service.getState());
    assertEquals(0, cancelNotificationCount.get());
}

Any ideas? It was successful when it ran for the previous commit just a few minutes ago, and it runs successfully on my build machine.

@kevinrushforth
Copy link
Collaborator

@kevinrushforth kevinrushforth commented Feb 15, 2019

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Feb 22, 2019

This PR builds fine when using the armv6hf build, and it doesn't break the existing functionality. I don't have an e-paper device so I can't test it, but the code takes a similar approach as the other monocle implementations.
A drawback is that the EPD classes will show up in the javafx.graphics.jar for other implementations, but this is a more general issue we probably need to address with monocle.

@jgneff
Copy link
Contributor Author

@jgneff jgneff commented Feb 22, 2019

I don't have an e-paper device so I can't test it, but ...

I plan to address this problem in two ways. First, I'm creating videos of the new features using a test application that I will publish in GitHub. I also plan to request comments from developers who do have these devices and are familiar with the low-level programming interface, but who might be less familiar with Java. I wanted to get comments from the JavaFX experts first, though. Thanks again.

Copy link
Collaborator

@kevinrushforth kevinrushforth left a comment

I did a cursory look at the changes, and there is one problem, in that the mxcfb.h is third-party code, which cannot be accepted under the terms of the OCA. I don't have an alternative solution, but you will need to look for one.

Remove the native C header file "mxcfb.h" and the test image file
"16777216colors_diffpatt.png", which cannot be accepted as third-party
code under the terms of the Oracle Contributor Agreement.

In FramebufferY8Test, add an option to save the generated test image and
the converted Y8 grayscale image as PNG files. Also add tests to compare
the performance of the old and new RGB565 16-bit pixel conversions.
@jgneff
Copy link
Contributor Author

@jgneff jgneff commented Mar 15, 2019

JBS bug ID: https://bugs.openjdk.java.net/browse/JDK-8217605

Some kind of text-encoding tragedy occurred when my Request for Enhancement was added to the JDK Bug System. 😕 The problems are most visible on the bugs.java.com page, which is covered in black diamond question marks (������), but the same errors exist on the direct bugs.openjdk.java.net page as well.

It doesn't seem right to leave the official report in such a state. Can you replace the description with the ASCII-encoded version attached below (JDK-8217605.ASCII.txt)? That file is identical to the existing text except for the following changes:

  • Replaced mangled encodings for single quotes, double quotes, bullets, em dashes, and multiplication signs with their ASCII equivalents.
  • Removed double quotes around all words italicized in the original GitHub issue.
  • Fixed the trailing bracket on five links that were mistakenly surrounded by <&gt; instead of <>.

You should be able to do a direct copy and replace of the entire description. Thank you.

Attached file: JDK-8217605.ASCII.txt

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Mar 19, 2019

This looks ok to me now.

Copy link
Collaborator

@kevinrushforth kevinrushforth left a comment

The changes to the implementation look good.

My only remaining comments are related to the two new tests, since these new tests are executed on all platforms by default. In addition to the specific feedback, here are some general comments:

  1. I see that you added the new tests as javafx.graphics unit tests rather than in the system tests where all other Monocle tests are. The graphics tests use the StubToolkit which doesn't initialize glass (this may not be important for these tests). Also, the FramebufferY8Test takes 35 seconds to run on my somewhat old Linux box, which is a bit high, and might be another reason to move them to the system tests; the system tests are only run with "-PFULL_TEST=true" and are expected to take longer. I recommend to move them to tests/system/src/test/java/test/com/sun/glass/ui/monocle/.

  2. The EPDSettingsTest produces the following log errors:

Mar 19, 2019 10:00:03 AM com.sun.glass.ui.monocle.EPDSettings getInteger
SEVERE: Value of monocle.epd.bitsPerPixel=64 not in [8, 16, 32]; using default (32)
Mar 19, 2019 10:00:03 AM com.sun.glass.ui.monocle.EPDSettings getInteger
SEVERE: Value of monocle.epd.rotate=4 not in [0, 1, 2, 3]; using default (0)
Mar 19, 2019 10:00:03 AM com.sun.glass.ui.monocle.EPDSettings getInteger
SEVERE: Value of monocle.epd.waveformMode=5 not in [1, 2, 3, 4, 257]; using default (257)

My reading of the test is that this is expected. If so, can you add a print statement at the beginning of the tests that produce the log errors indicating that they are expected?

@kevinrushforth kevinrushforth self-requested a review Mar 19, 2019
Move the Monocle EPD unit tests to the system test directory where all
the other Monocle tests are located. Run the tests with the command:

    gradle -PFULL_TEST=true :systemTests:test \
        --tests test.com.sun.glass.ui.monocle.*

EPDSettingsTest: Print a notice of which error log messages are
expected.

EPDSettingsTest: Put the method parameters for Assert.assertEquals in
the correct order (expected, actual).

FramebufferY8Test: Use the @ignore annotation instead of a boolean value
to skip tests.

FramebufferY8Test: Print a warning message when the method in the
subclass is slower than the one it overrides in the superclass for the
16-bit RGB565 conversions.
@jgneff
Copy link
Contributor Author

@jgneff jgneff commented Mar 21, 2019

Thank you for the comments, Kevin.

Also, the FramebufferY8Test takes 35 seconds to run on my somewhat old Linux box, which is a bit high, and might be another reason to move them to the system tests;

The two calls to Assert.assertArrayEquals in FramebufferY8Test will get very, very much faster when we switch to JUnit version 4.12 from our current version 4.8.2. For details, see the thread with the subject, New unit tests 12 times slower under Gradle. On my somewhat new Linux box, JUnit 4.12 runs those particular tests 60 times faster, reducing their time to just 0.5 seconds instead of the current 30 seconds.

My reading of the test is that this is expected. If so, can you add a print statement at the beginning of the tests that produce the log errors indicating that they are expected?

The tests now print notices to standard output about the error log messages printed to standard error. I tried printing them to standard error, but decided it looked better to keep the notices separate from the errors.

Copy link
Collaborator

@kevinrushforth kevinrushforth left a comment

The changes look good with one exception. The messages about the log errors being expected need to go to System.err in order to be effective. That way a developer who runs gradle --info will see both the message and the log warning together and know that it is an expected result (we've tried to do that with other warnings as well). Anything printed to System.out goes only to a separate log visible when you go through the web interface, and not interleaved with the log messages they are describing.

@jgneff
Copy link
Contributor Author

@jgneff jgneff commented Mar 22, 2019

The messages about the log errors being expected need to go to System.err in order to be effective.

Thanks, Kevin. I went back and forth on that, but I was looking only through the Web interface. I'll change them so that the two new tests print the following:

$ gradle --info -PFULL_TEST=true :systemTests:test --tests test.com.sun.glass.ui.monocle.*
...
test.com.sun.glass.ui.monocle.EPDSettingsTest > testBitsPerPixel STANDARD_ERROR
    Verify the error log message for monocle.epd.bitsPerPixel=64.
    Mar 22, 2019 8:17:24 AM com.sun.glass.ui.monocle.EPDSettings getInteger
    SEVERE: Value of monocle.epd.bitsPerPixel=64 not in [8, 16, 32]; using default (32)

test.com.sun.glass.ui.monocle.EPDSettingsTest > testRotate STANDARD_ERROR
    Verify the error log message for monocle.epd.rotate=4.
    Mar 22, 2019 8:17:24 AM com.sun.glass.ui.monocle.EPDSettings getInteger
    SEVERE: Value of monocle.epd.rotate=4 not in [0, 1, 2, 3]; using default (0)

test.com.sun.glass.ui.monocle.EPDSettingsTest > testWaveformMode STANDARD_ERROR
    Verify the error log message for monocle.epd.waveformMode=5.
    Mar 22, 2019 8:17:24 AM com.sun.glass.ui.monocle.EPDSettings getInteger
    SEVERE: Value of monocle.epd.waveformMode=5 not in [1, 2, 3, 4, 257]; using default (257)
...
test.com.sun.glass.ui.monocle.FramebufferY8Test > timeCopyTo16 STANDARD_OUT
    Converted 10 frames of 4,096 × 4,096 px to RGB565 in 594 ms (59 ms/frame): Framebuffer.copyToBuffer
    Converted 10 frames of 4,096 × 4,096 px to RGB565 in 471 ms (47 ms/frame): FramebufferY8.copyToBuffer

test.com.sun.glass.ui.monocle.FramebufferY8Test > timeWriteTo16 STANDARD_OUT
    Converted 10 frames of 4,096 × 4,096 px to RGB565 in 601 ms (60 ms/frame): Framebuffer.write
    Converted 10 frames of 4,096 × 4,096 px to RGB565 in 474 ms (47 ms/frame): FramebufferY8.write

test.com.sun.glass.ui.monocle.FramebufferY8Test > saveImage SKIPPED

test.com.sun.glass.ui.monocle.FramebufferY8Test > saveImageY8 SKIPPED
...

By the way, the sole purpose of FramebufferY8Test is to demonstrate (for a follow-up pull request) that the methods in FramebufferY8 can replace the ones they override in Framebuffer. After doing that, we will be able to delete all of FramebufferY8, FramebufferY8Test, FramebufferY8Shim, and FramebufferY8SuperShim.

@jgneff
Copy link
Contributor Author

@jgneff jgneff commented Apr 1, 2019

I posted my JavaFX test application and some videos of the application running on an e-paper display. The images below link to the video pages. The System Tests video is rather long and boring, but if you follow along with the associated log file, it does cover all of the new system properties and e-paper features.

System Tests Duke Waving Doll Dancing

My primary focus has been on the platform performance. I have done minimal testing of the JavaFX controls and font rendering, but I already see that it would be nice to have a new four-level grayscale theme for the controls.

I was pleased to discover that I could test all of the e-paper capabilities relevant to JavaFX with just a few simple animation patterns. The full-screen image animations generally run at about 80 percent CPU usage and 30 percent of memory on a 2013 Kobo Touch N905C with an 800-MHz ARM Cortex-A8 processor and 256 MB of RAM. This is one of the oldest devices still supported by Kobo. Furthermore, the tests run the slower OpenJDK Client VM because the Server VM does not support ARM Cortex-A8.

Copy link
Collaborator

@kevinrushforth kevinrushforth left a comment

Looks good.

@jgneff
Copy link
Contributor Author

@jgneff jgneff commented Apr 8, 2019

Thank you, Kevin. What's next? Should I request review comments now from outside the JavaFX community (specifically, from these experts on Kobo programming), or would you prefer that I gather those comments in a separate issue? In either case, should I merge the more recent changes in the develop branch into this monocle-epd pull request branch now?

If this pull request gets merged into OpenJFX for version 13, I plan to create two more pull requests before the July deadline, just to cover all of the Kobo e-reader devices now available:

  • Add support for e-paper displays on the i.MX6SL processor
  • Add support for e-paper displays on the i.MX6SLL processor

If all of them get into OpenJFX 13, they have a chance of getting into Ubuntu Base 20.04 LTS, too, making it trivial to install the JavaFX support in a chroot environment on e-paper devices.

@kevinrushforth
Copy link
Collaborator

@kevinrushforth kevinrushforth commented Apr 9, 2019

What's next?

I think this is ready to merge. The JBS issue is assigned to Johan, although either one of us can sponsor this change.

Should I request review comments now from outside the JavaFX community (specifically, from these experts on Kobo programming), or would you prefer that I gather those comments in a separate issue?

Probably in a separate issue, unless you feel more feedback is needed prior to merging this initial change.

In either case, should I merge the more recent changes in the develop branch into this monocle-epd pull request branch now?

In general it is a good idea to trigger a build if there have been any changes in develop that might impact this fix. Probably no need in this specific case, since I have no reason to believe that a recent change in the develop branch will cause problems with this fix.

@jgneff
Copy link
Contributor Author

@jgneff jgneff commented Apr 9, 2019

Probably in a separate issue, unless you feel more feedback is needed prior to merging this initial change.

No, I'm quite happy with the changes you requested. I probably should write more documentation on setting up the device before asking for outside reviews, anyway.

Probably no need in this specific case, since I have no reason to believe that a recent change in the develop branch will cause problems with this fix.

I modified only two existing files, buildSrc/armv6hf.gradle for the build and modules/javafx.graphics/src/test/addExports for the tests, so this pull request should be very safe to merge.

Wait! Listing those two files made me realize that the change to addExports is no longer required now that I moved the new unit tests into the system tests. The corresponding file for the system tests, tests/system/src/test/addExports, already contains an entry for the required package:

--add-exports javafx.graphics/com.sun.glass.ui.monocle=ALL-UNNAMED

I'll remove my change to the unit test file right now, and then we'll be good to go (with only one existing file modified).

@kevinrushforth
Copy link
Collaborator

@kevinrushforth kevinrushforth commented Apr 9, 2019

I'll remove my change to the unit test file right now, and then we'll be good to go (with only one existing file modified).

OK. That will also trigger a build, which is good.

The entry for com.sun.glass.ui.monocle is no longer needed now that the
new Monocle EPD unit tests have been moved into the system tests. The
corresponding system test file, tests/system/src/test/addExports,
already contains the entry:

--add-exports javafx.graphics/com.sun.glass.ui.monocle=ALL-UNNAMED
@kevinrushforth
Copy link
Collaborator

@kevinrushforth kevinrushforth commented Apr 10, 2019

Build with reverted change in the graphics test addExports file passed. The change looks good.

@kevinrushforth kevinrushforth merged commit 3a424b8 into javafxports:develop Apr 16, 2019
2 checks passed
@jgneff jgneff deleted the monocle-epd branch Apr 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants