add const ADatatype::getRaw(), Buffer::getData(); add copy+move Buffer::setData()

[diablodale]

Adds const to two member methods, enables operation on const and constref objects
Also add/clarify the copy and move Buffer::setData() functions to ensure there is no fallback to internal copy+move when a single move should be used instead.

All 65 tests + examples pass. And I have been using these changes since ~7 December which had considerable testing over the last month.

Fixes #330

[diablodale]

@szabi-luxonis would you please enable the CI to run? I want to see those results :-)

[SzabolcsGergely]

@szabi-luxonis would you please enable the CI to run? I want to see those results :-)

Running.

[themarpe]

IMO, this consts should be overloads - eg:

std::vector<std::uint8_t>& getData();
const std::vector<std::uint8_t>& getData() const;

Same with getRaw.

Otherwise, one might have a const message but still being able to modify the data when calling getData.

[diablodale]

Humm...🤔
Today's main code is already errant. It allows changes to the bytes in the buffer through directly access to members or apis like serialize().

• This PR is only interested in the ImgFrame and its base classes ADatatype, Buffer, RawBuffer, RawImgFrame. Anything can create ImgFrame; perhaps message is one example.
• You describe a new idea/semantic/behavior for a const ImgFrame. The existing depthai api does not have that idea/semantic ...it doesn't special-case such behavior. If you want to add that idea as a new feature, then this ImgFrame class and all its possible base(s) need to be reviewed and special-case behavior on all of them. For example serialize(), getCvFrame(), getFrame() would also need changes. Current main code is already ImgFrame::serialize() const. It is a const member function and returns a non-const shared_ptr to a non-const RawBuffer.
• Somehow make changes to RawBuffer and RawBuffer::raw such that the data they contain is unchangable yet can still be assigned to the member std::shared_ptr<RawBuffer> ADatatype::raw which demands a non-const RawBuffer. The object of a class can be const while some data contained deeper within still be non-const. const is only enforcing that the member fields directly on the const object can't be changed. Today's const ImgFrame enforces that the shared_ptr<RawBuffer> can't be changed to another shared_ptr. This myImgFrame.raw = otherSharedPtr; is forbidden. depthai-core main shared_ptr points to a non-const RawBuffer. If you want to special-case behavior such that a const ImgFrame has a const shared_ptr<const RawBuffer>, then I think c++ templates need to be involved. That amount of work does not make sense to me when zerocopy is coming soon and will rework much of this code. And sometime later also the meta topic of "const" throughout depthai-core.

The code so far in this PR enables code like...

auto preview = previewQueue->get<dai::ImgFrame>();
auto theArray = preview->getData();
theArray[0] = 111;
auto theRaw = preview->getRaw();
theRaw->data.front() = 222;

// works because ImgFrame has a shared_ptr<RawBuffer>. RawBuffer is non-const.
const dai::ImgFrame theConstFrame{};
auto theNonConstArray = theConstFrame.getData();
theNonConstArray[1] = 111;
auto theNonConstRaw = theConstFrame.getRaw();
theNonConstRaw->data.front() = 222;

Is there value in special-casing getData() and getRaw() while the other member apis and direct field access remain errant? 😵
getData() is easy. But getRaw() likely needs templates because I have to change it to shared_ptr and work might cascade inside the Rawbuffer to enforce no changes to bytes. But then I'm reworking RawBuffer which is going to be refactored with zerocopy.

Or broading the work to add the new semantic throughout ImgFrame and all its apis/bases doesn't make sense to me with zerocopy refactoring all this code. 🤷‍♀️

Comments? Ideas?

[diablodale]

ewww, I could probably std::const_pointer_cast<const RawBuffer>() rather than using templates for getRaw().
Other APIs are still errant.

[themarpe]

Agree the points above - some of the current API already breaks this behavior (nodes and getting properties, for example)

Otherwise I'm not fully decided yet on whether const should be transitive or not. I guess should depend on specific cases.

Lets leave this as is for the current PR and we'll address const on messages when refactoring later 👍

[diablodale]

Got it. 👍 "transitive" that's a good way to describe

opencv support

depthai optional opencv support functions get/setFrame() use a mix of getData() and direct field access img.data to read and write to them. They will need updates for transitive const.

Also, the OpenCV api does not support the idea of a Mat or UMat that contains constant data. The class only operates with void* and uchar*. One can make the U/Mat header const, and accessors like Mat::at() will return const references. But...the data within it is always assumed to be writeable. And the widely-used Mat::data pointer is always char*.

const uchar* const buf = zerocopy.constdataptr();
cv::Mat myMat(640, 480, CV_8U, buf); // valid; constructor param is void*
*myMat.data = 12; // .data returns a non-const `uchar*`; this is undefined behavior or a crash
myMat.data[1] = 23; // this is undefined behavior or a crash
myMat.at(2) = 34; // returns non-const reference; this is undefined behavior or a crash

const cv::Mat myConstMat(640, 480, CV_8U, buf);
*myMat.data = 12; // .data returns a non-const `uchar*`; this is undefined behavior or a crash
myMat.data[1] = 23; // this is undefined behavior or a crash
myMat.at(2) = 34; // won't compile; returns const reference which can not be an lvalue

prevent silent copies of buffer

I wrote the code needed to implement your transitive suggestion above. getData() and getRaw() both worked as desired. I lost about half an hour because of my own subtle bug. I used auto rather than auto& when calling getData() which lead to silent copy of data. It is my bug. Though, it highlights how easy it is to silently cause a copy of a whole buffer. I recommend the zerocopy refactor delete both copy assign and copy constructor...and instead provide a zerocopy::clone() factory if someone really needs to copy the whole buffer.

void myfuncconst(const dai::ImgFrame& frame) {
    auto theConstRaw = frame.getRaw();
    auto s0 = theConstRaw->data.size();
    // below correctly fails to compile, because front() returns `const uchar&`
    //theConstRaw->data.front() = 56;

    // dev mistake `auto` vs `auto&`
    // `auto` creates a vector<uchar> var and COPIES all the vector data from the const ref return
    // works, COPIES all vector data
    auto theFalseConstData = frame.getData();
    auto s1 = theFalseConstData.size();
    // and changes my COPY and not the original frame data
    theFalseConstData.front() = 33;
    theFalseConstData[1] = 12;

    // `auto&` creates a const vector<uchar>& var and assigns the ref from the const ref return
    auto& theConstDataRef = frame.getData();
    auto s2 = theConstDataRef.size();
    // correctly fails, because front() returns `const uchar&`
    theConstDataRef.front() = 33;
    // correctly fails, because operator[]() returns `const uchar&`
    theConstDataRef[1] = 12;
}