[RSDK-13311] - Simplify gstreamer state#37
Conversation
csi_camera.cpp
Outdated
| GstMessage* msg = gst_bus_pop(bus); | ||
| if (msg != nullptr) { | ||
| catch_pipeline(); | ||
| catch_pipeline(msg); |
There was a problem hiding this comment.
can catch_pipeline throw an exception? If it does we will leak memory here since gst_message_unref below wouldn't be called.
There was a problem hiding this comment.
[optional] You cold create a RAII msg wrapper class that takes care of this, so that it its destructor the gst_message_unref function is called. But really up to you if the added complexity makes sense here.
There was a problem hiding this comment.
Good call out, added try/catch that handles unref. Went with try/catch over an RAII wrapper since there's only one call site, didn't feel worth adding a new class for it.
|
|
||
| std::vector<unsigned char> CSICamera::buff_to_vec(GstBuffer* buff) { | ||
| if (buff == nullptr) { | ||
| throw Exception("Cannot convert null buffer to vector"); |
There was a problem hiding this comment.
would std::runtime_error be more appropriate than a bare Exception?
There was a problem hiding this comment.
Not entirely sure, I have been using the utility from viam/sdk/common/exception.hpp so the SDK wrapper can bubble up as a gRPC error message. Why should we consider std::runtime_error here?
There was a problem hiding this comment.
oh just a best practice callout. In general, the more precise we can be with the error type, the easier it is to debug. But I think if it bubbles up as a gRPC error in RDK logs, then it's optional
There was a problem hiding this comment.
Ok gotcha, this code path is hit from Images so we get the err msg out through the gRPC call.
4 participants
Description
Simple PR to reduce the amount of state in the core CSICamera class.
Tests
Reviews
small change, 1 approval