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
cpp: Add PixelBuffer<> and VariantPixelBuffer #1204
Conversation
Looks like legitimate Travis failure: https://travis-ci.org/openmicroscopy/bioformats/jobs/29435358 |
@melissalinkert Yes, it's missing a compat header for std::array on older systems. |
Travis builds now but segfaults; needs further investigation on ubuntu 12.04. |
Failures aside, the design seems good. |
Add all the interfaces and operators. Actual implementation and tests are currently stubbed out.
PixelBuffer<>
and VariantPixelBuffer
PixelBuffer<>
and VariantPixelBuffer
Also move some of the inline content of PixelProperties.h to PixelProperties.cpp.
…ility Also add testcases and correct errors found.
Tests were almost hitting the 64k windows symbol limit.
…ffers Boost.MultiArray doesn't use virtual methods and so is not dynamically castable, so can't be stored properly in a shared_ptr. Using Boost.Variant works around this design limitation. Update array() to return a reference to multi_array_ref for generality.
Travis was appeased by building with GCC 4.8 due to GCC 4.6 ICEing, which is an outright compiler bug. Might need further investigation, but not possible with travis; we need jenkins/docker for this. |
I have had a bit of a look at this, but it is a complex PR and somewhat out of my normal area of expertise. I see good conventions in the C++, but I have no real way of knowing if this is the right way to go about this or not. |
This version does not default-initialise the imaginary part correctly for templated types (i.e. Boost.Endian types). Explicitly initialise the imaginary part in all tests.
This reverts commit 8c24e19.
It lacks Boost.Container; substitute with std::array.
@dpwrussell Thanks for taking a look! I've added a few small fixes to make it compile correctly with older systems (GCC 4.6 and Boost from Ubuntu12.04), which should also make the jenkins job green again tomorrow. I think that as you say the main question is whether this is the right approach to take. I'd certainly be happy to simplify it by taking endian variants out and just using native types. It would just preclude memory mapping of endian-specific data. Though we could equally template that at a higher level e.g. by having VariantPixelBuffer templated on endianness so that each three specialisations would support the 11 pixel types, and have a higher-level abstract container for all three combinations to replace current VariantPixelBuffer usage. Or remove support entirely and have readers use Boost.Endian as required to do the byteswapping by hand. It really boils down to exactly what we intend this to be optimised for--it's currently completely general and comprehensive, but this can be refactored/scaled back either now or in following PRs. From the testing side of things, the testcases in place should be testing things very thoroughly; there are well over 1500 variations in over 100 tests being run on all these changes, testing every pixel type variation over every class and method. |
My preference would be to merge this now unless any further objections, and have any follow-up work in a separate PR. There are already a lot of changes, and adding more commits here is going to make re-review even more difficult. |
That's fine with me. Happy to rework things in a separate PR; if there's anything you want to discuss please just let me know. |
cpp: Add PixelBuffer<> and VariantPixelBuffer
See: https://trac.openmicroscopy.org.uk/ome/ticket/11827
--no-rebase
Adds support for 9D pixel data arrays of any supported pixel type and endianness. Summary of the design:
PixelBuffer
object along with type/endianness information.PixelBuffer
is templated on pixeltype/endianness.PixelBufferBase
pixeltype/endiantype attributes are to work around this fact.PixelBuffer<T>
contains the MultiArray as ashared_ptr<multi_array_ref<T>>
, orshared_ptr<multi_array<T>>
contained in a Boost.Variant; this permits the memory backing the array to be referenced from an external source or contained internally, respectively. Allows zero-copy (or reduced-copy) I/O where file formats and library interfaces permit it. Variant is used because the MultiArray types inherit from each other but have no vtables, making it unsafe to reference via a common base.shared_ptr
inPixelBuffer
allows replacement afterPixelBuffer
construction to resize and change the backing store.VariantPixelBuffer
can contain ashared_ptr
to anyPixelBuffer
which is specialised for one of the pixel types in the OME data model, using Boost.Variant.static_visitor
s) to process pixel data in any way. Because it's expanded once per pixeltype/endian combination, coverage for all pixel types is checked and enforced at compile time. Invalid/unsupported specialisations can be no-ops or throw an exception. This inheritance-free dynamic polymorphism is the primary reason for using Variant over simply storing astd::shared_ptr<PixelBufferBase>
: it restricts the allowed types to only those explicitly supported, and it enforces complete support for these types. It can be significantly faster than inheritance since there's no virtual call overhead for every method call, though there is one-time overhead in iterating over the typelist to despatch the corresponding visitor, but when processing large amounts of pixel data this overhead will be dwarfed by the overhead of the algorithm itself.VariantPixelBuffer
provides some generic methods which operate on all variant pixel buffer types usingstatic_visitor
plus some specialised methods to allow direct manipulation of pixel buffers of specific type (e.g.assign
andat
).VariantPixelBuffer
, but for cases where the pixel type is fixed, use ofPixelBuffer<pixeltype>
specialised for that type will be possible.BIT
type usesbool
, and this is typically stored asuint8_t
/uint32_t
; this allows proper array addressing and (de)referencing, so if stored as a bitmask on disc it will require expansion. Bits are not suitable due to not being directly addressable."XY"
. Reverse ordering is not possible when using the model enums, but is possible directly. A true nD model will allow much greater flexibility; for now 9D with these restrictions supports all existing cases.PixelBuffer
andVariantPixelBuffer
); may be optimised further in the future.pixelTypeFromBytes
toPixelProperties
; similar to the Java FormatTools implementation, but also supports complex types.PixelProperties
.cpp/test/ome-bioformats/formatreader
,cpp/test/ome-bioformats/pixelproperties
,cpp/test/ome-bioformats/pixelbuffer
andcpp/test/ome-bioformats/variantpixelbuffer
.FormatReader
for readPlane/openBytes using a VariantPixelBuffer.This should handle a large portion of the content of FormatTools, and will later on also be able to replace the use of plane indices entirely when we switch to a truly nD API; for now it will be limited to 2D when using the existing openBytes interface. The extent/range constructors and methods will allow specification of extents in all dimensions, and it's also possible to adjust the array indexing from 0..n to different ranges and so could be an alternative overloaded openBytes method (just pass in the buffer alone, which contains all the parameters you would normally provide in addition to the buffer).
Questions: