Use cscore for PublishVideoOperation #866

SamCarlberg · 2017-11-09T04:27:55Z

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

Video streams in Shuffleboard using cscore

Updates JavaCV to 1.3.3 and javacpp-opencv to 3.2.0

SamCarlberg · 2017-11-09T04:30:50Z

build.gradle

+        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'


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.

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 · 2017-11-09T14:36:47Z

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.

JLLeitschuh · 2017-11-09T15:46:32Z

core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java

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


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.

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.

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

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

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.

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.

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

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

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.

JLLeitschuh · 2017-11-09T15:47:43Z

core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java

+
+  // 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");


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

What would that gain?

If it fails then GRIP doesn't crash immediately.

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

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

JLLeitschuh · 2017-11-09T15:48:30Z

core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java

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


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

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)

SamCarlberg · 2017-11-09T15:49:06Z

core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java

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


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

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

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.

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

JLLeitschuh · 2017-11-09T18:13:26Z

core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java

+   * {@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() {


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

Oooh. That's much better 👍

JLLeitschuh · 2017-11-09T18:13:50Z

core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java

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


JLLeitschuh · 2017-11-09T18:14:34Z

core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java

-  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


JLLeitschuh · 2017-11-09T18:19:35Z

core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java

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


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.

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

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

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?

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.

Okay, that's fine then.

JLLeitschuh · 2017-11-09T18:21:32Z

core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java

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


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

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

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.

JLLeitschuh · 2017-11-09T18:49:08Z

core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java

+   * @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)


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.

SamCarlberg · 2017-11-09T20:31:39Z

core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java

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


@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

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).

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

JLLeitschuh · 2017-11-09T20:31:44Z

core/src/test/java/edu/wpi/grip/core/operations/composite/PublishVideoOperationTest.java

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


Derpy but sure.

JLLeitschuh · 2017-11-09T20:32:32Z

core/src/test/java/edu/wpi/grip/core/operations/composite/PublishVideoOperationTest.java

+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+
+public class PublishVideoOperationTest {


👍 Thanks for adding this test for us!

JLLeitschuh · 2017-11-09T20:33:42Z

core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java

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


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

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.

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

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 · 2017-11-10T03:06:19Z

https://docs.opencv.org/java/2.4.2/org/opencv/core/Mat.html#Mat(long) ?

JLLeitschuh · 2017-11-10T03:08:24Z

@SamCarlberg The above solves our problem.

SamCarlberg · 2017-11-10T03:16:31Z

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 · 2017-11-10T03:18:47Z

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

saudet · 2017-11-10T03:30:57Z

We can still share the data of the image without sharing the whole Mat struct. That would be a much better solution.

SamCarlberg · 2017-11-10T03:39:56Z

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

saudet · 2017-11-10T03:46:13Z

Just sharing image data? No worries at all, just need to make sure deallocation is done right.

saudet · 2017-11-10T03:49:58Z

On Windows though, symbols are loaded globally for the process, but I think the Java bindings from OpenCV don't expose the C++ API anyway, so should be alright.

JLLeitschuh · 2017-11-10T19:25:51Z

core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java

+    // 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());


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?

Fix an issue with codegen tests from the JavaCV update

Should fix problems resulting from incompatible binaries

saudet · 2018-05-10T06:20:04Z

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 · 2018-05-10T15:28:42Z

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

ThadHouse · 2019-04-19T01:24:41Z

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 · 2019-04-19T04:35:47Z

@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 · 2020-08-17T18:30:43Z

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

ThadHouse · 2020-08-17T18:50:10Z

My PR only did inputs, not output.

SamCarlberg added 2 commits November 8, 2017 23:04

Use cscore for PublishVideoOperation

d16e05d

Updates JavaCV to 1.3.3 and javacpp-opencv to 3.2.0

Smart port reuse

af7ebeb

SamCarlberg commented Nov 9, 2017

View reviewed changes

Improve NetworkTable code

1691326

JLLeitschuh reviewed Nov 9, 2017

View reviewed changes

SamCarlberg commented Nov 9, 2017

View reviewed changes

Updates from review. Add lots of documentation, fix hostname on windows

86829a7

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

SamCarlberg force-pushed the cscore-publishing branch from 5308536 to 86829a7 Compare November 9, 2017 18:10

JLLeitschuh reviewed Nov 9, 2017

View reviewed changes

SamCarlberg commented Nov 9, 2017

View reviewed changes

JLLeitschuh reviewed Nov 9, 2017

View reviewed changes

Use InetAddress.localhost() for getting hostname, add test for mat copy

a65ffd2

SamCarlberg force-pushed the cscore-publishing branch from 7b224a9 to a65ffd2 Compare November 9, 2017 21:19

Use Mat ctor for copying pointer, use NetworkInterface to get host IPs

40343c8

JLLeitschuh reviewed Nov 10, 2017

View reviewed changes

SamCarlberg added 5 commits November 10, 2017 14:43

Re-add test for compatiblity between JavaCV and OpenCV

93022f7

Fix an issue with codegen tests from the JavaCV update

Remove unused static list of network interfaces, use assume() in test

b4bff3b

Copy bytes instead of native pointers

c1869f8

Should fix problems resulting from incompatible binaries

Merge branch 'master' into cscore-publishing

54da545

Remove unneeded preloading

644cab8

SamCarlberg mentioned this pull request Apr 20, 2019

m_jpeg stream import #930

Open

Use cscore for PublishVideoOperation #866

Are you sure you want to change the base?

Use cscore for PublishVideoOperation #866

Conversation

SamCarlberg commented Nov 9, 2017 • edited Loading

GRIP pipeline

Video streams in Shuffleboard using cscore

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLLeitschuh commented Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThadHouse Apr 19, 2019 • edited Loading

Choose a reason for hiding this comment

ThadHouse Apr 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamCarlberg Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLLeitschuh Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamCarlberg Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLLeitschuh Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saudet commented Nov 10, 2017 via email

JLLeitschuh commented Nov 10, 2017

SamCarlberg commented Nov 10, 2017 • edited Loading

PeterJohnson commented Nov 10, 2017

saudet commented Nov 10, 2017 via email

SamCarlberg commented Nov 10, 2017

saudet commented Nov 10, 2017 via email

saudet commented Nov 10, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saudet commented May 10, 2018

JLLeitschuh commented May 10, 2018

ThadHouse commented Apr 19, 2019

ThadHouse commented Apr 19, 2019

JLLeitschuh commented Aug 17, 2020

ThadHouse commented Aug 17, 2020

SamCarlberg commented Nov 9, 2017 •

edited

Loading

JLLeitschuh commented Nov 9, 2017 •

edited

Loading

ThadHouse Apr 19, 2019 •

edited

Loading

ThadHouse Apr 19, 2019 •

edited

Loading

SamCarlberg Nov 9, 2017 •

edited

Loading

JLLeitschuh Nov 9, 2017 •

edited

Loading

SamCarlberg Nov 9, 2017 •

edited

Loading

JLLeitschuh Nov 9, 2017 •

edited

Loading

SamCarlberg commented Nov 10, 2017 •

edited

Loading