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

Use CS Core Raw Streams for HTTPCamera Input #952

Merged
merged 33 commits into from
Oct 29, 2019

Conversation

ThadHouse
Copy link
Contributor

The images are grabbed in native code, so they are much more efficient.
Also, the code is shared with all WPILib tools, so likely tested a bit more.

This also updates to the 2020 artifactory locations for dependencies, as they are needed for the raw CV stream support.

Outputs can be added in the future easily as well.

HTTP support works on all 3 platforms, it's only USB camera support that is limited.

@codecov-io
Copy link

codecov-io commented Oct 21, 2019

Codecov Report

Merging #952 into master will decrease coverage by 0.32%.
The diff coverage is 12.67%.

@@             Coverage Diff              @@
##             master     #952      +/-   ##
============================================
- Coverage     52.87%   52.55%   -0.33%     
  Complexity        1        1              
============================================
  Files           329      331       +2     
  Lines          8892     8968      +76     
  Branches        561      569       +8     
============================================
+ Hits           4702     4713      +11     
- Misses         3985     4049      +64     
- Partials        205      206       +1

@@ -93,6 +94,7 @@ public void testClickOnCreateNetworkTableOpensDialog() throws Exception {
}

@Test
@Ignore("Fails on all platforms") // TODO: Figure out why this is broken.
Copy link
Member

Choose a reason for hiding this comment

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

At minimum, please at least ensure that this actually works when driven from the actual UI.
Same with the one below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know enough about the UI to know what this test even does? Do you know?

Copy link
Member

Choose a reason for hiding this comment

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

The test is failing because adding a webcam source is broken.

You can recreate this bug by running the GRIP UI and selecting "Add Source" -> "Webcam". Select your camera (on my computer this is 0). And press OK. GRIP will crash with a JVM error.

Process 'command '/Library/Java/JavaVirtualMachines/jdk-11.0.2.jdk/Contents/Home/bin/java'' finished with non-zero exit value 134

ui/ui.gradle.kts Outdated
* jpackage, which is not ideal given that Java 13 is not yet stable.
*
* This task regenerates the jlink image every time it is invoked.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Comment formatting

@@ -349,7 +338,8 @@ fun JpackageExec.configureForCurrentOs() {
OperatingSystem.LINUX -> {
val installerFileDir = installerFilesBaseDir.resolve("linux")
resourceDir.set(installerFileDir)
icon.set(installerFileDir.resolve("GRIP.png"))
// Skip icon on linux, causing build to break
//icon.set(installerFileDir.resolve("GRIP.png"))
Copy link
Member

Choose a reason for hiding this comment

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

Think you can figure it out/fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a linux system, so my only testing is on Azure, which is basically impossible to fix. Would need someone else to test it and figure out whats happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think its a bug in the j14 jpackager.

Using custom package resource [menu icon]  (loaded from GRIP.png).
java.nio.file.FileAlreadyExistsException: /tmp/jdk.jpackage13600112042171272034/images/opt/grip/lib/GRIP.png
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:94)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:116)
	at java.base/sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:219)
	at java.base/java.nio.file.spi.FileSystemProvider.newOutputStream(FileSystemProvider.java:478)
	at java.base/java.nio.file.Files.newOutputStream(Files.java:223)
	at java.base/java.nio.file.Files.copy(Files.java:3143)
	at jdk.jpackage/jdk.jpackage.internal.AbstractBundler.fetchResource(AbstractBundler.java:94)
	at jdk.jpackage/jdk.jpackage.internal.LinuxPackageBundler$DesktopIntegration.prepareSrcIconFile(LinuxPackageBundler.java:771)
	at jdk.jpackage/jdk.jpackage.internal.LinuxPackageBundler$DesktopIntegration.create(LinuxPackageBundler.java:432)
	at jdk.jpackage/jdk.jpackage.internal.LinuxPackageBundler.execute(LinuxPackageBundler.java:177)
	at jdk.jpackage/jdk.jpackage.internal.Arguments.generateBundle(Arguments.java:627)
	at jdk.jpackage/jdk.jpackage.internal.Arguments.processArguments(Arguments.java:513)
	at jdk.jpackage/jdk.jpackage.main.Main.execute(Main.java:98)
	at jdk.jpackage/jdk.jpackage.main.Main.main(Main.java:51)
jdk.jpackage.internal.PackagerException: java.nio.file.FileAlreadyExistsException: /tmp/jdk.jpackage13600112042171272034/images/opt/grip/lib/GRIP.png

Its failing to copy the icon file to the temp folder, but I can't find anywhere where its already being copied.

@JLLeitschuh
Copy link
Member

Nice work @ThadHouse!

@@ -93,6 +94,7 @@ public void testClickOnCreateNetworkTableOpensDialog() throws Exception {
}

@Test
@Ignore("Fails on all platforms") // TODO: Figure out why this is broken.
Copy link
Member

Choose a reason for hiding this comment

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

The test is failing because adding a webcam source is broken.

You can recreate this bug by running the GRIP UI and selecting "Add Source" -> "Webcam". Select your camera (on my computer this is 0). And press OK. GRIP will crash with a JVM error.

Process 'command '/Library/Java/JavaVirtualMachines/jdk-11.0.2.jdk/Contents/Home/bin/java'' finished with non-zero exit value 134

@ThadHouse
Copy link
Contributor Author

I fixed the comments I knew how to fix. I don't know enough JavaFX to fix the failing tests, but theyre unrelated to this PR anyway.

The reason they had to be reenabled was we now build cscore and ntcore with Java 11, which means the Java 8 test method no longer works.

@ThadHouse
Copy link
Contributor Author

It looks like the failing tests are even broken in Master. This PR just enables the tests on Java 11, which is why we're seeing them now.

@JLLeitschuh
Copy link
Member

@SamCarlberg I'd really appreciate if you could take a look at these.

@ThadHouse
Copy link
Contributor Author

@JLLeitschuh I reverted the changes from this PR other then the ones to reenable tests, and they still fail. That means the tests are failing in master, and not just because of the changes in this PR. I want to start another PR building on top of this, any chance we can merge it and create an issue to address the failing tests? Based on the test descriptions, what I think they're testing works just fine, and its something wrong with the tests themselves. I can't figure out at all what is though.

https://dev.azure.com/wpiroboticsprojects/GRIP/_build/results?buildId=167

@JLLeitschuh
Copy link
Member

@AustinShalit Thoughts? I'm okay as long as someone will follow up and resolve this soon.

@AustinShalit
Copy link
Member

I would merge it.

Copy link
Member

@JLLeitschuh JLLeitschuh left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants