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 cscore for PublishVideoOperation #866

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

SamCarlberg
Copy link
Member

@SamCarlberg SamCarlberg commented Nov 9, 2017

Updates JavaCV to 1.3.3 and javacpp-opencv to 3.2.0 in order to keep the same memory model between bindings.

Allows up to ten publish video operations on ports 1180 to 1189.

Note: this increases the size of the core jar by ~67MB from bundling the cscore and opencv binaries.

GRIP pipeline

grip-cscore-pipeline

Video streams in Shuffleboard using cscore

grip-streams-in-shuffleboard

Updates JavaCV to 1.3.3 and javacpp-opencv to 3.2.0
compile group: 'org.bytedeco', name: 'javacv', version: '1.3.3'
compile group: 'org.bytedeco.javacpp-presets', name: 'opencv', version: '3.2.0-1.3'
compile group: 'org.bytedeco.javacpp-presets', name: 'opencv', version: '3.2.0-1.3', classifier: os
//compile group: 'org.bytedeco.javacpp-presets', name: 'opencv-3.0.0-1.1', classifier: 'linux-frc'
Copy link
Member Author

Choose a reason for hiding this comment

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

Commented out because there's no opencv-3.2.0-1.3 with the linux-frc classifier and I'm worried about different memory models between binding versions (JavaCV with 3.0.0, OpenCV with 3.2.0) . This could get removed entirely if we remove the deploy functionality.

Copy link
Member

Choose a reason for hiding this comment

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

With this commented out as it is is the deploy functionality essentially nerfed? If so, then we should remove the deployer for this season.

@JLLeitschuh
Copy link
Member

JLLeitschuh commented Nov 9, 2017

Travis fails, this is a non-starter for me.
We need to introduce the docker based build system (probably in a separate PR) to resolve this first before this gets merged.

try {
Field nativeObjField = Mat.class.getField("nativeObj");
nativeObjField.setAccessible(true);
nativeObjField.setLong(openCvMat, javaCvMat.address());
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a terrible idea, but I understand what you are doing here.
Do you actually see speed benefits from doing this?
@saudet Is there any reason why doing this assignment could shoot us in the foot?

This is taking the memory address of the OpenCV mat and basically telling JavaCV to use it's memory instead.

Copy link

Choose a reason for hiding this comment

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

First it'd say, what is missing from JavaCV that you need to use the other set of bindings? IMO, we should fix that so we don't need to do things like that.

And even when that's necessary, we don't need to use reflection.

Just passing the pointer address around is sufficient, as long as you make sure they are from the same version of OpenCV, compiled with the same compiler, the same flags, etc and to make sure memory isn't prematurely deallocated and that it gets deallocated by the same binaries used to allocate it.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way of creating a Mat just using the memory address??

Copy link
Member

Choose a reason for hiding this comment

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

@SamCarlberg Ping on this? Thoughts on making sure the memory isn't inconsistently GCed?

Copy link
Contributor

@ThadHouse ThadHouse Apr 19, 2019

Choose a reason for hiding this comment

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

See my comment at the bottom. In the rewrite, all the memory mapping between different opencv versions will be gone. There will be no wpilib opencv interface anymore. And the API designed is allocation free.

Copy link
Contributor

@ThadHouse ThadHouse Apr 19, 2019

Choose a reason for hiding this comment

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

As an example, The following class would be added to GRIP. It will use the JavaCV version of OpenCV, and always copy from the raw image into the requested Mat. It is memory safe, and does not depend at all on the internal representation of how an OpenCV Mat looks. There's some tricks on the cscore jni side to avoid allocations when needed, but everything is in spec, no reflection or undefined behavior.

https://github.com/wpilibsuite/allwpilib/blob/13e15663b6570028a9222b4990d141d16fda3a72/cscore/src/dev/java/edu/wpi/cscore/RawCVMatSink.java

This would be what the source side looks like
https://github.com/wpilibsuite/allwpilib/blob/d56dee9c493e8de59340b22d4cc43bc8ba40239d/cscore/src/dev/java/edu/wpi/cscore/RawCVMatSource.java

We internally copy the pointer, so again not depending on any memory issues, or native layouts.

Copy link
Member

Choose a reason for hiding this comment

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

@SamCarlberg Do we want to wait for that update then?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so. Thad's offered to do do the rewrite with the new cscore stuff. Maybe we should also use the cameraserver library for doing the stream hosting instead of manually doing the Networktable side

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't be able to bring it in directly (because of the opencv dependency) but I could bring in most of it. There's a lot extra we wouldnt need there as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't be able to bring it in directly (because of the opencv dependency) but I could bring in most of it. There's a lot extra we wouldnt need there as well.


// Write to the /CameraPublisher table so the MJPEG streams are discoverable by other
// applications connected to the same NetworkTable server (eg Shuffleboard)
private static final ITable cameraPublisherTable = NetworkTable.getTable("/CameraPublisher");
Copy link
Member

Choose a reason for hiding this comment

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

Can you not load this statically? Instead do it at instantiation time.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would that gain?

Copy link
Member

Choose a reason for hiding this comment

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

If it fails then GRIP doesn't crash immediately.

Copy link
Member Author

Choose a reason for hiding this comment

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

So instead of crashing to desktop at initialization when creating the operations list, it'll crash to desktop when adding a new PublishVideo operation. I guess that's better because at least users will see it happens when adding the operation

Copy link
Member Author

@SamCarlberg SamCarlberg Nov 9, 2017

Choose a reason for hiding this comment

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

We'll also need to update NetworkTables to 4.0, but that's a different PR

logger.log(Level.SEVERE, "CameraServerJNI load failed! Exiting", e);
System.exit(31);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to happen statically? Can't this be done when the operation is constructed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then the cscore loading check would happen every time a new operation is created instead of only once, when the class loads. I'd prefer to just have a flag set so the operation can be created normally and just throw an exception in perform() so users can see the operation failing (and hopefully report it to us)

CameraServerJNI.getHostname();
} catch (Throwable e) {
logger.log(Level.SEVERE, "CameraServerJNI load failed! Exiting", e);
System.exit(31);
Copy link
Member Author

Choose a reason for hiding this comment

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

This should set a flag and make the operation throw an exception in perform() instead of suddenly exiting the entire application

Copy link
Member

Choose a reason for hiding this comment

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

Yea, we want to limit the number crashes that don't display the crash dialog.

Copy link
Contributor

Choose a reason for hiding this comment

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

Theres actually nothing to catch, because internally in the JNI loading classes we call System.exit(1). There would be no exception ever caught here. It would be impossible to catch.

Copy link
Contributor

Choose a reason for hiding this comment

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

After a little discussion, we will leave the IO Exception that the static constructor can throw in flight, that way its catchable. That will be a change on our side. We will do the same thing to ntcore as well.

Set a flag when cscore can't be loaded to make the operation perform() fail, instead of crashing to desktop
Still need to do mac hostname resolution
* {@link CameraServerJNI#getHostname() getHostName() function} only works on Linux, so we need to
* implement the method for Windows and Mac ourselves.
*/
private static String getHostName() {
Copy link
Member

Choose a reason for hiding this comment

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

Replace with: InetAddress.getLocalHost().getHostName()???

Copy link
Member Author

Choose a reason for hiding this comment

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

Oooh. That's much better 👍


if (!cscoreLoaded) {
throw new IllegalStateException(
"cscore could not be loaded. The image streaming server cannot be started.");
Copy link
Member

Choose a reason for hiding this comment

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

👍

private static final int PORT = 1180;
private static final byte[] MAGIC_NUMBER = {0x01, 0x00, 0x00, 0x00};
private static final int INITIAL_PORT = 1180;
private static final int MAX_STEP_COUNT = 10; // limit ports to 1180-1189
Copy link
Member

Choose a reason for hiding this comment

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

👍

serverSource.setResolution(input.size().width(), input.size().height());
serverSource.putFrame(publishMat);
if (lastFrame != -1) {
long dt = now - lastFrame;
Copy link
Member

Choose a reason for hiding this comment

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

I've seen this actually be 0 even though you are using nano seconds. (This is highly unlikely but we saw it in the camera source code in one bug) This will cause a DIV by zero exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

It'll actually be Double.Infinity per IEEE spec for floating-point numbers (1e9 / 0L = Infinity)

Copy link
Member

@JLLeitschuh JLLeitschuh Nov 9, 2017

Choose a reason for hiding this comment

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

(int) 1e9 / 0L will throw java.lang.ArithmeticException: / by zero because of the cast.
Cast in the wrong place.

Copy link
Member

Choose a reason for hiding this comment

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

In kotlin: (1e9 / 0L).toInt() is equal to 2147483647 which is an insanely high frame rate. Is that what you want to set it to?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's technically infinite since the new frame comes at the same time as the previous one. But it doesn't happen here because it's rate limited by the pipeline and the update rate of the pipeline sources. I seriously doubt dt will ever be less than a few hundred microseconds.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, that's fine then.

// Loading the CameraServerJNI class will load the appropriate platform-specific OpenCV JNI
CameraServerJNI.getHostname();
loaded = true;
} catch (Throwable e) {
Copy link
Member

Choose a reason for hiding this comment

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

Can native bindings ever throw an InterruptedException? If so, make sure that this handles it correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's most likely to be UnsatisfiedLinkError or a related error. No interrupted exceptions here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless we explicitly throw interrupted from the JNI level, nothing in JNI otherwise would throw it. And since its a dumb idea to interrupt a thread anyway, we would never throw it internally. Pretty much all that shows up is UnsatisfiedLinkError, or one of the few runtime exceptions we throw.

Note this should be using forceLoad(), not getHostname() (I don't think we had forceLoad() then), but I will my sure my version does that.

* @param openCvMat the OpenCV Mat wrapper object to copy into
* @throws RuntimeException if the OpenCV native pointer could not be set
*/
private static void copyJavaCvToOpenCvMat(opencv_core.Mat javaCvMat, Mat openCvMat)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some unit tests for this function to make sure that it gets covered. I don't want someone in the future to be updating dependencies and have this break because this code doesn't get run by a test.

InetAddress localHost = InetAddress.getLocalHost();
ourTable.putStringArray("streams",
new String[]{
generateStreamUrl(localHost.getHostName(), ourPort),
Copy link
Member Author

@SamCarlberg SamCarlberg Nov 9, 2017

Choose a reason for hiding this comment

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

@PeterJohnson Would it be prudent to add a ".local" to the end of the local hostname e.g. team-190-driverstation.local? I'm not sure how mDNS would handle it on an FRC field without the .local suffix

Copy link
Contributor

Choose a reason for hiding this comment

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

Note InetAddress.getLocalHost() may not give you the address you want if the machine has multiple network adapters (https://stackoverflow.com/questions/5813194/inetaddress-getlocalhost-does-not-return-expected-ip-address-from-c-windows-s). You probably want to use NetworkInterface.getNetworkInterfaces() instead. cscore provides the similar CameraServerJNI.getNetworkInterfaces() as well as the NetworkInterfacesChanged event for listening to network interface changes, but at the moment these only work on Linux. In either case, appending ".local" is almost certainly the wrong thing to do; you should publish the raw IP instead; appending ".local" will require the computer running GRIP to publish its mDNS address, etc.

The streams entry is an array for exactly this reason, so you can e.g. generate an entry for each network address the machine has (e.g. on the robot side we publish with both the USB and Ethernet IP addresses).

Copy link
Contributor

Choose a reason for hiding this comment

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

getNetworkInterfaces works on windows and mac now, so we will be able to use the cscore functions for that. My rewrite will do this

@BeforeClass
public static void initialize() {
// Make sure the OpenCV JNI is loaded
blackHole(PublishVideoOperation.DESCRIPTION);
Copy link
Member

Choose a reason for hiding this comment

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

Derpy but sure.

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;

public class PublishVideoOperationTest {
Copy link
Member

Choose a reason for hiding this comment

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

👍 Thanks for adding this test for us!

generateStreamUrl(localHost.getHostAddress(), ourPort)
});
} catch (UnknownHostException e) {
ourTable.putStringArray("streams", new String[0]);
Copy link
Member

@JLLeitschuh JLLeitschuh Nov 9, 2017

Choose a reason for hiding this comment

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

Should we log something here? Or maybe throw exceptions in perform?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? The documentation isn't very clear on what circumstances will cause this exception to be thrown. It might never be thrown for the local host since it appears to only happen if the host is unreachable.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. @PeterJohnson do you have any thoughts on what might cause this on the local system or if this is a non-issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like how the Java getLocalHost function works is it gets the local hostname, then resolves it into an address. So it's definitely possible to fail: https://stackoverflow.com/questions/1881546/inetaddress-getlocalhost-throws-unknownhostexception

@saudet
Copy link

saudet commented Nov 10, 2017 via email

@JLLeitschuh
Copy link
Member

@SamCarlberg The above solves our problem.

@SamCarlberg
Copy link
Member Author

SamCarlberg commented Nov 10, 2017

Well, kind of. I'm still wary about binary incompatibilities between the JavaCV builds and the WPI OpenCV builds. The perfect solution would be to have cscore accept raw image data (bytes, width, height) to avoid needing to use the OpenCV bindings altogether. Something like manually creating cscore images.

It'd mean memory copies (not terrible), but would let us use the OpenCV bindings as a runtime dependency for cscore instead of a compile-time dependency for GRIP and reduce developer confusion from having two OpenCV APIs in GRIP.

@PeterJohnson
Copy link
Contributor

I'm working on a raw image API for cscore for exactly that reason.

@saudet
Copy link

saudet commented Nov 10, 2017 via email

@SamCarlberg
Copy link
Member Author

Doesn't that still run the risk of binary incompatibilities?

@saudet
Copy link

saudet commented Nov 10, 2017 via email

@saudet
Copy link

saudet commented Nov 10, 2017 via email

// object that gets copied into. The data address will change, however, if the image is resized
// or changes type.
if (publishMat == null || publishMat.nativeObj != input.address()) {
publishMat = new Mat(input.address());
Copy link
Member

Choose a reason for hiding this comment

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

Can you keep the test you had for copyJavaCvToOpenCvMat arround for making sure that we know if the memory models are incompatible when we bump versions in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

@saudet
Copy link

saudet commented May 10, 2018

BTW, the JavaCPP Presets for OpenCV now bundle the official Java API as well, so it's possible to use both APIs with the same underlying binaries for OpenCV, avoiding any clashes:
https://github.com/bytedeco/javacpp-presets/tree/master/opencv#documentation

@JLLeitschuh
Copy link
Member

Wow, serious throwback to an old PR. Thanks @saudet!

@ThadHouse
Copy link
Contributor

I'm probably going to completely rewrite most of this using the RawFrame API I'm adding to CScore, but I want to leave it open for now for reference for some of the more weird ui stuff that I don't know.

@ThadHouse
Copy link
Contributor

@SamCarlberg do you want to move the unrelated stuff out of this PR into another PR? Like the DoublePointer stuff? Or has that already been done in other PR's?

@JLLeitschuh
Copy link
Member

@SamCarlberg is this PR outdated now that the one from @ThadHouse is merged?

@ThadHouse
Copy link
Contributor

My PR only did inputs, not output.

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.

5 participants