Skip to content

WIP: RSDK-11082: Remove instances of C-style pointer manipulation #30

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.

Sign up for GitHub

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

Open
lia-viam wants to merge 4 commits into
base: main
Choose a base branch
from
Open

WIP: RSDK-11082: Remove instances of C-style pointer manipulation #30

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e59f48
rewrite raw camera image and rgb to pcd
lia-viam Jul 24, 2025
c7e807c
condense helper invocation
lia-viam Jul 24, 2025
c0c7fe6
use span
lia-viam Jul 25, 2025
4aa2f81
Merge branch 'main' into ptr-convert
lia-viam Jul 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 44 additions & 53 deletions src/module/orbbec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
#include <stdexcept>
#include <vector>

#include <boost/core/span.hpp>

#include <viam/sdk/common/proto_value.hpp>
#include <viam/sdk/components/camera.hpp>
#include <viam/sdk/components/component.hpp>
Expand Down Expand Up @@ -76,17 +78,23 @@ struct ViamOBDevice {
std::shared_ptr<ob::Config> config;
};

struct raw_camera_image {
using deleter_type = void (*)(unsigned char*);
using uniq = std::unique_ptr<unsigned char[], deleter_type>;
class raw_camera_image {
public:
explicit raw_camera_image(std::size_t sz) : bytes(new unsigned char[sz]), size(sz) {}

static constexpr deleter_type free_deleter = [](unsigned char* ptr) { free(ptr); };
boost::span<unsigned char const> span() const noexcept {
return boost::span{bytes.get(), size};
}

static constexpr deleter_type array_delete_deleter = [](unsigned char* ptr) { delete[] ptr; };
Copy link
Contributor Author

@lia-viam lia-viam Jul 25, 2025

Choose a reason for hiding this comment

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

note that

boost::span<unsigned char> span() noexcept {
Copy link
Contributor

@SebastianMunozP SebastianMunozP Jul 25, 2025

Choose a reason for hiding this comment

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

Not critical at all, but could we use std::uint8_t instead of unsigned char to express bytes?

Copy link
Contributor Author

@lia-viam lia-viam Jul 25, 2025

Choose a reason for hiding this comment

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

that works!

Copy link
Contributor Author

@lia-viam lia-viam Jul 28, 2025

Choose a reason for hiding this comment

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

@SebastianMunozP I was looking into this and it seems like we may want to use either unsigned char or std::byte, as per these guidelines: https://stackoverflow.com/a/77097674 but they advise against std::uint8_t

I believe this project is C++17 so we could use std::byte instead of unsigned char, do you have thoughts or preferences?

Copy link
Contributor

@SebastianMunozP SebastianMunozP Jul 28, 2025

Choose a reason for hiding this comment

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

huh, didn't know about std::byte :) , I agree, let's go with that then.

Copy link
Contributor Author

@lia-viam lia-viam Jul 28, 2025

Choose a reason for hiding this comment

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

just added some changes using std::byte. unfortunately since the Viam SDK is still on C++14 there is no access to std::byte so we end up having to convert back to unsigned char. Please let me know your thoughts, we can keep it this way or revert to the previous diff using unsigned char

Copy link
Contributor

@SebastianMunozP SebastianMunozP Jul 28, 2025

Choose a reason for hiding this comment

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

I see, in that case let's just go back to unsigned char and try this change maybe in the future.

return boost::span{bytes.get(), size};
}

uniq bytes;
size_t size;
private:
std::unique_ptr<unsigned char[]> bytes;
std::size_t size;
};

// STRUCTS END

// GLOBALS BEGIN
Expand Down Expand Up @@ -141,24 +149,24 @@ uint64_t timeSinceFrameUs(uint64_t nowUs, uint64_t imageTimeUs) {
}

std::vector<unsigned char> RGBPointsToPCD(std::shared_ptr<ob::Frame> frame, float scale) {
int numPoints = frame->dataSize() / sizeof(OBColorPoint);
assert(frame->is<ob::PointsFrame>());
Copy link
Contributor Author

@lia-viam lia-viam Jul 25, 2025

Choose a reason for hiding this comment

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

If this pattern comes up more often we could write a FrameView<T> class which performs the following asserts and returns a span of the data already set to type T

Copy link
Collaborator

@hexbabe hexbabe Aug 7, 2025

Choose a reason for hiding this comment

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

what kind of error message is produced when an assert fails?

Copy link
Contributor

@SebastianMunozP SebastianMunozP Aug 11, 2025

Choose a reason for hiding this comment

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

It includes the line like:

orbbec.cpp:152 frame->is<ob::PointsFrame>() assert failed

So it includes the line number and string of the failing assert.

Copy link
Contributor

@SebastianMunozP SebastianMunozP Aug 1, 2025

Choose a reason for hiding this comment

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

I like using asserts, but in this case I would prefer to detect the case of these two asserts and log it and early return. I think it would be better to handle this by looking at logs than forcing the module to terminate.

Cc. @nicksanford in case you want to weigh in.

Copy link
Collaborator

@nicksanford nicksanford Aug 1, 2025

Choose a reason for hiding this comment

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

  1. If the assert fails does it terminate the os process?
  2. If it terminates the os process does Viam server get an unambiguous log that would tell us immediately why the process terminated?
  3. Is it impossible for the assertion to ever fail in practice given the current state of the code?

If the answers to all those questions are 'yes' then this is fine.

If the answer to any of them is no then I'll need a justification for why this asset is here.

Copy link
Contributor

@SebastianMunozP SebastianMunozP Aug 1, 2025

Choose a reason for hiding this comment

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

  1. yes (via calling std::abort())
  2. yes (we will get a message stating the file/line of the assert and the assert that failed)
  3. I tested this module and the assertions did not failed

It's worth noting that as the code is at the moment, these asserts are being removed from the binary since this is a release build. I enabled them by creating a debug build and making sure that I could hit asserts.

Copy link
Collaborator

@nicksanford nicksanford Aug 4, 2025

Choose a reason for hiding this comment

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

Having assets fire in release builds sounds good to me.

I'd also be ok with us writing our own assert function. Whichever you feel is best.

Copy link
Contributor Author

@lia-viam lia-viam Aug 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So in the current code as is the assert wouldn't get hit unless someone called RGBPointsToPCD(not_an_rgb_point_frame). The asserts are there to show how we can check type safety given that we are dealing with polymorphic data but in this function only one type of input is acceptable.

Per another comment #30 (comment) the use of assert here was meant to sketch a general procedure that could be used if this pattern keeps coming up. Namely, we have an ob::Frame* frame, and we want to get a range which refers to a definite underlying type. In this case it would be nice to be able to just do

void f(std::shared_ptr<ob::Frame> frame){
    FrameView<OBColorPoint> frameView(frame);
}

and we would expect this to be an error if frame were not in fact an OB_FORMAT_RGB_POINT, similar to how some standard library containers have asserts or bounds checking in debug builds.

I think assert or throw is the right behavior here; this will fail loudly with a meaningful log message and viam-server can restart the module. An early return would result in further control flow being executed with an empty vector, which could be easy to miss.

A possible option, since we already use Boost, is Boost.Assert. This has the advantage of letting us

  • keep asserts in non-debug builds independent of NDEBUG
  • write a different assertion processing function (eg, log error, throw exception) rather than the assert behavior of calling abort

Copy link
Contributor

@SebastianMunozP SebastianMunozP Aug 5, 2025

Choose a reason for hiding this comment

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

That's a good idea, let's do that, let's change the asserts for BOOST_ASSERT

assert(frame->getFormat() == OBFormat::OB_FORMAT_RGB_POINT);

auto pointSpan = boost::span{reinterpret_cast<OBColorPoint*>(frame->data()), frame->dataSize() / sizeof(OBColorPoint)};
Copy link
Contributor

@SebastianMunozP SebastianMunozP Jul 25, 2025

Choose a reason for hiding this comment

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

Love this :)

Copy link
Collaborator

@hexbabe hexbabe Aug 7, 2025

Choose a reason for hiding this comment

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

how do we ensure that the span's underlying bytes do not get freed while using it?

Copy link
Contributor Author

@lia-viam lia-viam Aug 11, 2025

Choose a reason for hiding this comment

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

@hexbabe that's the responsibility of the calling code; span is a non-owning reference or "view" type, meaning it provides a different view or interface upon some underlying data without taking responsibility for the lifetime of the data. Similar to string_view, or language-level references (int i; int& j = i;).

see https://stackoverflow.com/a/45723820 for further discussion


OBColorPoint* points = (OBColorPoint*)frame->data();
std::vector<PointXYZRGB> pcdPoints;

for (int i = 0; i < numPoints; i++) {
OBColorPoint& p = points[i];
unsigned int r = (unsigned int)p.r;
unsigned int g = (unsigned int)p.g;
unsigned int b = (unsigned int)p.b;
std::transform(pointSpan.begin(), pointSpan.end(), std::back_inserter(pcdPoints), [scale](const OBColorPoint& p) {
auto r = static_cast<unsigned int>(p.r);
auto g = static_cast<unsigned int>(p.g);
auto b = static_cast<unsigned int>(p.b);

unsigned int rgb = (r << 16) | (g << 8) | b;
PointXYZRGB pt;
pt.x = (p.x * scale);
pt.y = (p.y * scale);
pt.z = (p.z * scale);
pt.rgb = rgb;
pcdPoints.push_back(pt);
}
return PointXYZRGB{p.x * scale, //
p.y * scale, //
p.z * scale, //
rgb};
});

std::stringstream header;
header << "VERSION .7\n"
Expand All @@ -174,32 +182,15 @@ std::vector<unsigned char> RGBPointsToPCD(std::shared_ptr<ob::Frame> frame, floa
std::string headerStr = header.str();
std::vector<unsigned char> pcdBytes;
pcdBytes.insert(pcdBytes.end(), headerStr.begin(), headerStr.end());
for (auto& p : pcdPoints) {
unsigned char* x = (unsigned char*)&p.x;
unsigned char* y = (unsigned char*)&p.y;
unsigned char* z = (unsigned char*)&p.z;
unsigned char* rgb = (unsigned char*)&p.rgb;

pcdBytes.push_back(x[0]);
pcdBytes.push_back(x[1]);
pcdBytes.push_back(x[2]);
pcdBytes.push_back(x[3]);
// Assert that PointXYZRGB is a POD type, and that it has no padding, thus can be copied as bytes.
// Since vector is contiguous, we can just copy the whole thing
static_assert(std::is_pod_v<PointXYZRGB>);
static_assert(sizeof(PointXYZRGB) == (3 * sizeof(float)) + sizeof(unsigned int), "PointXYZRGB has unexpected padding");

pcdBytes.push_back(y[0]);
pcdBytes.push_back(y[1]);
pcdBytes.push_back(y[2]);
pcdBytes.push_back(y[3]);

pcdBytes.push_back(z[0]);
pcdBytes.push_back(z[1]);
pcdBytes.push_back(z[2]);
pcdBytes.push_back(z[3]);

pcdBytes.push_back(rgb[0]);
pcdBytes.push_back(rgb[1]);
pcdBytes.push_back(rgb[2]);
pcdBytes.push_back(rgb[3]);
}
auto byteSpan = boost::as_bytes(boost::span{pcdPoints});
pcdBytes.insert(
pcdBytes.end(), reinterpret_cast<unsigned char const*>(byteSpan.begin()), reinterpret_cast<unsigned char const*>(byteSpan.end()));

return pcdBytes;
}
Expand Down Expand Up @@ -421,18 +412,18 @@ void listDevices(const ob::Context& ctx) {

raw_camera_image encodeDepthRAW(const unsigned char* data, const uint64_t width, const uint64_t height, const bool littleEndian) {
viam::sdk::Camera::depth_map m = xt::xarray<uint16_t>::from_shape({height, width});
std::copy(reinterpret_cast<const uint16_t*>(data), reinterpret_cast<const uint16_t*>(data) + height * width, m.begin());

for (size_t i = 0; i < m.size(); i++) {
m[i] = m[i] * mmToMeterMultiple;
}
auto depthPixels = boost::span{reinterpret_cast<uint16_t const*>(data), height * width};

std::transform(depthPixels.begin(), depthPixels.end(), m.begin(), [](uint16_t mi) { return mi * mmToMeterMultiple; });
Copy link
Collaborator

@hexbabe hexbabe Aug 7, 2025

Choose a reason for hiding this comment

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

this is sick

Copy link
Contributor Author

@lia-viam lia-viam Aug 11, 2025

Choose a reason for hiding this comment

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

may i recommend this for inspo 🙂
https://youtu.be/W2tWOdzgXHA?t=133


std::vector<unsigned char> encodedData = viam::sdk::Camera::encode_depth_map(m);

unsigned char* rawBuf = new unsigned char[encodedData.size()];
std::memcpy(rawBuf, encodedData.data(), encodedData.size());
raw_camera_image rawBuf(encodedData.size());

std::copy(encodedData.begin(), encodedData.end(), rawBuf.span().begin());
Copy link
Contributor Author

@lia-viam lia-viam Jul 25, 2025

Choose a reason for hiding this comment

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

note that most standard library implementations of std::copy will fall back to a compiler intrinsic version of memcpy or memmove when operating on contiguous iterators of arithmetic types


return raw_camera_image{raw_camera_image::uniq(rawBuf, raw_camera_image::array_delete_deleter), encodedData.size()};
return rawBuf;
}

// HELPERS END
Expand Down Expand Up @@ -682,7 +673,7 @@ vsdk::Camera::image_collection Orbbec::get_images() {
vsdk::Camera::raw_image depth_image;
depth_image.source_name = kDepthSourceName;
depth_image.mime_type = kDepthMimeTypeViamDep;
depth_image.bytes.assign(rci.bytes.get(), rci.bytes.get() + rci.size);
depth_image.bytes.assign(rci.span().begin(), rci.span().end());

vsdk::Camera::image_collection response;
response.images.emplace_back(std::move(color_image));
Expand Down
Loading