Jules, You are very right - I've been bitten by that change-reluctance from the other side before, so I definitely see your point. My only concern was that it decreased stability, by deallocating memory in a different way than it was allocated. You said you've tested it and it works with the OpenCV camera plugin, though, so that's promising. I imagine the full API change will probably be made shortly, but you're right that we shouldn't put off changing a broken interface a little just because it doesn't fix it. I've just got a few minor tweaks I'd like to make (mostly build-system stuff, but also exposing those allocation functions through a C-safe header in osvrUtil so plugins can allocate memory the way "we" do), will test it on Android, then I'll merge your pull request.
Discussion still open on the preferred interfaces on the pluginkit side, if anyone wants to speak in favor of option B, by the way, but reflection has suggested that probably option A is stronger. Ryan On 10/23/2015 6:11 AM, Jules Blok wrote: > Hi Ryan, > > I completely understand why you want this changed. But I don't think > we should postpone changes to the allocation/deallocation code just > because it updates a non-ideal solution. The code I've proposed just > moves the buffer allocation out of OpenCV and into OSVR. This does not > significantly affect the plugin compatibility as long as you are not > mixing VC++ runtimes, but that is already something that is likely to > crash the server. > > I think we should merge the PR now and implement a new > allocation/deallocation method later. The sooner we do this, the less > developers will be depending on the current design. I've done some > basic tests and the code seems to work fine, if it has any negative > impact we can always revert the changes and submit a different solution. > > Reluctance to make necessary changes to design flaws because it does > nothing to improve stability does not promote further contribution. > > Regards, > Jules Blok > > > 2015-10-22 16:34 GMT+02:00 Ryan Pavlik <r...@sensics.com > <mailto:r...@sensics.com>>: > > On 10/16/2015 7:38 AM, Jules Blok wrote: >> Hi everyone, >> >> There has been a lot of discussion in my pull request to remove >> the OpenCV dependency when compiling the ClientKit from source. >> You can find the complete discussion in the Pull Request: >> https://github.com/OSVR/OSVR-Core/pull/210 >> >> Ryan Pavlik has some concerns regarding the >> allocation/deallocation of the buffers used to store the image >> data. Currently ImagingComponent is used by both the server and >> the plugin, which could theoretically be built using different >> VC++ runtimes. This would result in a crash when the server >> allocates the buffer with a different VC++ runtime than the >> plugin uses to deallocate the same buffer. >> >> So instead both allocation and deallocation should become the >> responsibility of plugin. I'd like to implement that by having an >> internal allocation (and deallocation) function in the PluginKit >> that will be part of the plugin context. So when the server >> allocates a buffer for a plugin it will use the allocation >> function stored in the plugin context. >> >> I will try and implement that next week, let me know if anyone >> would like to see this done differently. >> >> Regards, >> Jules Blok >> > > So I'll add some context/clarification here. > > One of the design goals of the imager interface was to avoid unnecessary > copies. As such, the PluginKit interface osvrDeviceImagingReportFrame > (typically/intended to be used through the C++ wrapper) accepts a buffer > with the image and transfers ownership of that buffer to the server. The > C++ wrapper using OpenCV performs a single copy of the supplied image > and transfers the ownership of that copy in the PluginKit API call. > > The initial use cases suggested that OpenCV would be widely used to > supply images, and thus that C API contains a design flaw that escaped > notice earlier: it does not allow you to explicitly specify a deleter > for that buffer, even though ownership is transferred. Once ownership is > transferred, the buffer ends up internally managed by a smart pointer > whose deleter is presently set to be the OpenCV deallocation function, > which, beyond just different VC++ versions, results in crashes if you > even use a different OpenCV version to build your plugin than was used > to build core, or if you allocate a buffer yourself with new or malloc - > this has been observed on Windows as well as on Android. > > So, whether or not it might sometimes work, the API call itself is > fundamentally broken from a design point of view. However, fixing this > will require breaking the PluginKit API and ABI, so while I think it's > still early enough that this is OK, I'd like to get it right this time. > > An additional motivation to fixing this API, as Jules brought up, is > because it mandates the only required use of OpenCV to build the core - > those allocation/deallocation functions. Particularly for client apps > that are building OSVR as a submodule instead of consuming a binary > package (thus being exposed to build-time dependencies of ClientKit's > implementation, not just the intended external dependencies), requiring > OpenCV to build is a burden and conceptually unnecessary. We can perform > aligned allocations without it. > > So, there are two basic possibilities for the replacement > osvrDeviceImagingReportFrame function: > > A - Take a buffer from the plugin without transferring ownership, making > the single copy inside of the OSVR-Core implementation (thus we are free > to use whatever allocation/deallocation we like internally and are not > exposed to the mechanics of how a plugin allocates its buffer(s) as it > keeps ownership) > > B - Take a buffer as well as a deleter function from the plugin, > transferring ownership and using the plugin-provided deleter when > required to free the transferred buffer. (In this case, the plugin would > likely make the single copy.) > > (There's actually a third way, but it could co-exist and is probably not > an urgent need: have two methods, one to allocate/reserve a buffer and > let the plugin either copy or directly store data into it then send it > with a second call. It would allow some plugins to avoid one copy they > might otherwise do internally, but it's a pretty extreme use case, I > think, and is a harder to use API. And, as I mentioned, it could be > added later and coexist with either A or B quite happily.) > > Both the options only have to make a single copy at this level, though A > has the flexibility of totally hiding the copying from the plugin so > internal optimizations are unlimited. B has the advantage that its > signature differs from the current one (has an added parameter, deleter) > so it might be easier to catch plugins that must be ported, though we > could just change the name of the replacement function. A has the > advantage that it allows flexibility internally in OSVR-Core: we could > copy into a pre-allocated ring of buffers that get re-used, instead of > allocating and freeing repeatedly - to achieve this with B, each plugin > would be responsible for that functionality. B has the disadvantage that > it's easier to use incorrectly - easier for plugin writers to "use after > free" because of the ownership transfer involved, just as with the > current API. However, it wouldn't be the first PluginKit API method that > takes a deleter. (B also requires that we pass a deleter down through > the stack along with the buffer, though I'm not sure off the top of my > head how much implementation hassle that would be.) > > After writing this up, it seems to me like option A is the preferable > one, since it is an easier API to use correctly/harder to use > incorrectly and provides more flexibility for us to optimize the core. > However, since this impacts anyone providing imager data through OSVR, I > encouraged Jules to reach out to the list for feedback and I've now sent > this followup with what I see as the two main options for fixing the > PluginKit image transmission design flaw, a flaw highlighted by Jules' > work on being able to build subsets of the core repo without needing > OpenCV. > > Feedback on the two options would be greatly appreciated (particularly > if you're involved with the plugin-side imaging), even if it's just a > +1/thumbsup. The actual API/ABI break will be done at a tagged release > and will be available ahead of time in a branch to ease porting, and > since we're still in 0-land it will not result in a bump of any > sonames/abi-level-names, etc. (There are a few other cleanups that > involve simpler ABI breaks that might land at the same time, just to > tear the bandage off quickly) > > Thanks! > > Ryan > > PS. Just to clarify the specifics, while I brought up client > applications, none of this particular discussion actually impacts the > ClientKit API or ABI, it's all plugin-side. > > -- > Ryan A. Pavlik, Ph.D. > Senior Software Engineer > Sensics, Inc. > www.sensics.com <http://www.sensics.com> > > -- > OSVR-devel mailing list > osvr-de...@getosvr.org <mailto:osvr-de...@getosvr.org> > Options and Unsubscribe: > > http://lists.getosvr.org/options.cgi/osvr-devel-getosvr.org/jules.blok%40gmail.com > Archives: http://dir.gmane.org/gmane.comp.vr.osvr.devel > > List homepage: > http://lists.getosvr.org/listinfo.cgi/osvr-devel-getosvr.org > > -- Ryan A. Pavlik, Ph.D. Senior Software Engineer Sensics, Inc. www.sensics.com <http://www.sensics.com>
-- OSVR-devel mailing list osvr-de...@getosvr.org Options and Unsubscribe: http://lists.getosvr.org/options.cgi/osvr-devel-getosvr.org/archive%40mail-archive.com Archives: http://dir.gmane.org/gmane.comp.vr.osvr.devel List homepage: http://lists.getosvr.org/listinfo.cgi/osvr-devel-getosvr.org
