ClientKit: Remove dependency on OpenCV from Imaging.

[CrossVR]

Wrapping the image data into an OpenCV type should be the responsibility of the application. OpenCV is a very large dependency and it should not be depended on just for a few lines of code.

Fixes issue #77.

[rpavlik]

This is just in the optional C++ header-only wrapper. While it's recommended, because of typing reasons, it's not strictly required for use of even the imaging interface. There's not much point in having this extra layer wrapping the callbacks without marshalling the data into the OpenCV type - that's what this file exists for. If you just want the bytes, there's the C interface.

[CrossVR]

But I thought we didn't want to encourage people to downgrade to the C interface just to get rid of a dependency? See issue #44.

[CrossVR]

@rpavlik Just to quote the specific argument:

What I really wouldn't want to see is the (slightly unsafe, less usable) C API being used (or a wrapper re-invented) in C++ games/engines because the wrapper uses things that aren't allowed.

[rpavlik]

Touche - good point :D Probably could split this into two layers - a lifetime-management layer and then a opencv layer on top, so you can use whichever you like. Especially with Imaging at the moment,I really don't want people using the C API because you actually get a lock on an element of a shared-memory ring buffer, so failing to release that is a problem.

[CrossVR]

Exactly, but don't we already have that opencv layer? OpenCVTypeDispatch.h is all a developer would need to wrap the buffer pointer into a matrix using the cv::Mat constructor, it would basically be a one-liner.

[rpavlik]

Thanks for working on this! I'd really rather see a "halfway" approach - being able to build without OpenCV optionally, mangling the library names so it's clear imaging support might be degraded. (It's annoying that the C++ standard doesn't provide all the alignment guarantees needed to support efficient processing with SIMD instructions, GPU processing, etc, but them's the breaks.

None of the changes in the util headers or in the clientkit headers should be needed to build without OpenCV - you should be able to just turn off the building of the "header dependency tests" (which you'd probably want to do in a submodule situation anyway) and then, in proper C++ form, you won't pay for what you don't use.

Thanks for pointing me to this - GitHub's notification system is befuddling and I typically end up getting notified only about things I don't want to be...

[CrossVR]

I'm not really convinced this is at all useful, most people will use their own image processing library. They'll load the image into something like a texture and never even use OpenCV, but currently they'll have to pull it in as a huge dependency.

Even if we make it optional for the developers that do use OpenCV, this wrapper only saves them a few lines of code at most. Is it really worth the added complexity of having two different versions of this header in the ClientKit?

[CrossVR]

@rpavlik I'm done implementing those changes. I think benefits of removing the OpenCV dependency outweighs the benefits of having the wrapper and the lifetime management inside the same structure.

If we still want to take care of the lifetime management we should make a child class of cv::Mat that actually supports std::shared_ptr data pointers.

[CrossVR]

@rpavlik Any feedback on this PR?

[d235j]

I'm curious as to what (if anything) is holding this up, too; OpenCV is a fairly heavyweight dependency and while it's very useful, I don't think pulling in large dependencies is a great idea.

[rpavlik]

I agree, it's primarily lack of time to review, as well as hesitancy regarding breaking API/ABI.

The ClientKit header patches, as long as they come with matching fixes to the corresponding clients, are fine and easy to approve: those are to semi-optional pieces of the API (the C++ header-only stuff - semi-optional just for imaging because not managing imaging lifetime correctly right now is the only way to freeze the server from the client), don't affect already-compiled client apps, but some extra work to update clients using that wrapper to the new API. In general, pretty good.

However, the change in Common (the change in allocation/deallocation in ImagingComponent) actually breaks our plugin ABI, and would result in all plugins that do imaging crashing the server. (At least on Windows, mixing allocation and deallocation, or doing so over .dll boundaries, is a fast lane to crashtown).

Now, the core deallocating a buffer allocated by a plugin seemed like a good idea at the time because it meant the core didn't do a copy/allocation, and I didn't expect opposition to using OpenCV (considered it the de-facto standard and that delay-loading the DLL in common was good enough to hide the "cost"). However, in retrospect a function with a user-provided deleter would have been better (same advantages: reduced copying, clear ownership semantics, plus no mandatory dependency).

I agree that this (meaning defaulting to C++ standard aligned allocation in imaging component, as well as the change to receive a deleter function from PluginKit) should be done (it's tripped up several folks), we just need to take it to the mailing list probably and hash out how to do it - probably safest and easiest to break the API at the same time removing the old function and creating a new, differently-named function to replace it that takes both the buffer and the deleter. Haven't actually had a non-backward-compatible API/ABI break in pluginkit in months, so you see why I am hesitant :) (There are other things I'd like to break too, see #53 which is already implemented internally, we're just ignoring one parameter - so one break invites the question of doing more breaks all at once or instead giving "version 2" functions in addition...)

CC some folks recently touching imaging stuff: @JeroMiya @zachkinstner

[d235j]

@rpavlik are we at a point yet where OSVR is stable enough that breaking API changes are an issue? Obviously this would have to undergo more serious discussion (mailing list is not a bad idea here), but considering that OSVR is not yet widely used, it's better we do breaking API changes that are needed now rather than putting in place non-deal API. At some point it might be worthwhile doing a review of sorts going over the API and finalizing things.

I have to look at the allocation/deallocation stuff more closely. I've been through that mess on Windows myself and it's not something you want to deal with. It gets even worse when someone decides to replace operator new and delete, and it would not surprise me if some codebases do this.

[CrossVR]

@rpavlik The allocator and deallocator work exactly like their OpenCV counterpart. It stores the pointer to the original buffer at index -1 on allocation and uses that pointer when deallocating. It should be no problem for it to deallocate buffers allocated by OpenCV and vice-versa. Have you tested whether it actually breaks the ABI?

The only risk is when OpenCV has been compiled with a different Visual C++ runtime than the server. But that's not a problem, both OpenCV and OSVR use the same VC12 runtime.

Also, as long as OSVR is not at version 1.0 yet I think we should make API (and ABI) changes that make sense, even if they are breaking, instead of already building up a ton of legacy APIs. I see no reason for anyone to expect that version 0.2 and version 0.3 should be backwards compatible. Backwards compatibility is nice, but it should never hold back sensible changes.

You can't hide the cost of including a dependency, OSVR is an open-source project so developers are going to build it from source within their projects as it is much more convenient for debugging and version management.

[rpavlik]

No, I haven't had a chance, but based on the number of crashes I've heard of when people didn't use the exact build of OpenCV we used to allocate their buffer, and the number of times cross DLL deallocation has bitten me, it makes me nervous. (Thinking of an ABI break as in change of behavior that affects library consumers, not directly the binary interface itself, of course. The subtle kind of break that's harder to test for) It would be great if it didn't break ABI, then we could put off the real fix for longer, but I simply haven't had a chance to test it on Windows. Easiest way would be to make a windows build then try to use a plugin (OpenCV camera, leap motion) compiled against a shipped binary snapshot. Testing on Android would be harder for poor @JeroMiya

[d235j]

This is likely to only be an issue on Windows due to the nature of static linking and (de)allocators on that platform. I've spent hours debugging issues caused by (de)allocations across DLL boundaries and this is something we should avoid if possible.
It does look like the changes here use raw malloc and free though, which should be safe.

[CrossVR]

@rpavlik I can do the test, but will that really be enough to convince you the changes are safe?

You're getting the crash because people are mixing the Visual C++ versions of the OpenCV DLLs. This change will actually fix the crash because OSVR will always do the allocation/deallocation with its own Visual C++ version.

Except if people are using a different Visual C++ version to compile the plugins. That is probably the issue you're facing. You should cross the DLL boundary again and not make the plugin responsible for the deallocation, instead it should call back to the server for the deallocation. Or vice-versa, which is what you're suggesting.

[rpavlik]

It's not that I need convincing the changes are safe, it's that if they happen to be ABI-safe right now that's purely coincidence, and the correct fix is to fix the API.

If you split this pull request into ClientKit-only and core, I think we can get stuff merged sooner.

[CrossVR]

Doesn't the ClientKit rely on ImagingComponent?

[rpavlik]

Its implementation does, but its API doesn't. This accomplishes something (reducing required libraries for the client API) on its own.

[CrossVR]

I need to compile the implementation, that's the whole reason I'm making these changes.

So how do you want the API fixed? Will the plugin be responsible for allocation/deallocation or will the server be responsible?

[rpavlik]

I think the plugin should be responsible as mentioned in #210 (comment) but please email the devel list to have a public discussion so that other stakeholders can participate.

[CrossVR]

@rpavlik Could you send that email to the devel list? You know more about this than I do.

[CrossVR]

@rpavlik No response from the mailing list yet, so I assume everyone is fine with the the current idea?

[CrossVR]

Okay, I'll make the allocation/deallocation API changes this week and then we can merge it.

[CrossVR]

@rpavlik I've tried looking at the code base today regarding memory management, but it is a lot more complicated than I anticipated. I don't think I'll be able to do that within a week and I honestly don't have any more time for it than that.

Since this PR doesn't make the memory issue any worse, could we please address that issue in a different PR?

I did the test you specified and I had no problems retrieving an image from my webcam using the OpenCV plugin and Imaging example from the 0.6 snapshot with an OSVR server compiled from the code in this PR.

[rpavlik]

I'll see when I can take a look at the pull request. If nothing else I can merge the API-only stuff easily. I've posted a followup on the mailing list (or at least I tried so, though I don't see it yet) laying out the ABI/API break stuff. Quite interesting that you were able to use a plugin from the snapshot (Windows, I'm assuming, since we don't publish other snapshot binaries?) with this PR - given how crash-happy that particular code has been, I'm pleasantly surprised. We'll fix the API and whole thing soon here.

[rpavlik]

I'm not terribly familiar with the C++11 alignment functionality yet - are we guaranteed here that aligned > buffer so that below, when we do aligned[-1] we're not writing out of bounds?

[CrossVR]

Yeah that's an error, there should be a buffer+1 there. Thanks for
catching it, will correct it tomorrow.

[CrossVR]

@rpavlik I've made the necessary changes, it's ready for review.

Do note that merging just the API changes without the underlying source changes will not help me since I will still be confronted with the same dependency when compiling the ClientKit.

[CrossVR]

@rpavlik You mentioned that you'd like to move the allocator/deallocator to osvrUtil? Are there any other changes you need? I can make those changes this weekend so that the PR can be merged without any more work from your side.

[rpavlik]

oh, thanks! That would be great - yeah, just moved to Util, probably into an implementation file, with a public header exporting the functions, and maybe adjust the C++ wrapper on the pluginkit side so that it uses that, instead of the OpenCV matrix clone.
(The silly build stuff would take me longer to describe than to do.)

[CrossVR]

@rpavlik I'm done moving the allocator function into its own header. I'm not sure what to do with the PluginKit, using these new allocator functions there would result in copying a buffer which we want to avoid.

[rpavlik]

There's already a buffer copy hidden there. I can take care of it - I put it there. I plan to review and merge this today.

[rpavlik]

I've pulled this locally and am just making a few little changes before pushing it to master. Will leave this open until that's in and merged.

[rpavlik]

OK, I've opened the new pull request #260 which is just this one, merged into master, with a few changes. Waiting on some help on making sure I didn't break code somebody else understands better than me, and a positive travis-ci result, then should be clear to go.