| Message ID | 20190812150232.1088-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 0c32433d8c742d2a52d44264c64faec2c7ac28f2 |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2019-08-12 18:02:32 +0300, Laurent Pinchart wrote: > Replace the manual implementation of asynchronous method invocation > through a custom message with Object::invokeMethod(). This simplifies > the thread RPC implementation. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I like the diffstat :-) Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > > This patch applies on top of the "[PATCH 00/18] Object & Thread > enhancements" series. > > src/android/camera_device.cpp | 8 +------- > src/android/camera_device.h | 4 +++- > src/android/camera_proxy.cpp | 8 +++----- > src/android/thread_rpc.cpp | 18 +----------------- > src/android/thread_rpc.h | 15 --------------- > 5 files changed, 8 insertions(+), 45 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index e2c1f2a246c8..999c51e6ba4a 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -70,14 +70,8 @@ CameraDevice::~CameraDevice() > /* > * Handle RPC request received from the associated proxy. > */ > -void CameraDevice::message(Message *message) > +void CameraDevice::call(ThreadRpc *rpc) > { > - if (message->type() != ThreadRpcMessage::type()) > - return Object::message(message); > - > - ThreadRpcMessage *rpcMessage = static_cast<ThreadRpcMessage *>(message); > - ThreadRpc *rpc = rpcMessage->rpc; > - > switch (rpc->tag) { > case ThreadRpc::ProcessCaptureRequest: > processCaptureRequest(rpc->request); > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index ac5b95c95104..4d834ceb08a5 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -26,13 +26,15 @@ > return nullptr; \ > } while(0); > > +class ThreadRpc; > + > class CameraDevice : public libcamera::Object > { > public: > CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> &camera); > ~CameraDevice(); > > - void message(libcamera::Message *message); > + void call(ThreadRpc *rpc); > > int open(); > void close(); > diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp > index f0cacac8025b..3eb2f9fbcffb 100644 > --- a/src/android/camera_proxy.cpp > +++ b/src/android/camera_proxy.cpp > @@ -9,6 +9,8 @@ > > #include <system/camera_metadata.h> > > +#include <libcamera/object.h> > + > #include "log.h" > #include "message.h" > #include "utils.h" > @@ -185,10 +187,6 @@ int CameraProxy::processCaptureRequest(camera3_capture_request_t *request) > > void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest) > { > - std::unique_ptr<ThreadRpcMessage> message = > - utils::make_unique<ThreadRpcMessage>(); > - message->rpc = &rpcRequest; > - > - cameraDevice_->postMessage(std::move(message)); > + cameraDevice_->invokeMethod(&CameraDevice::call, &rpcRequest); > rpcRequest.waitDelivery(); > } > diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp > index 295a05d7c676..f57891ff56bf 100644 > --- a/src/android/thread_rpc.cpp > +++ b/src/android/thread_rpc.cpp > @@ -5,19 +5,11 @@ > * thread_rpc.cpp - Inter-thread procedure call > */ > > +#include "thread.h" > #include "thread_rpc.h" > > -#include "message.h" > - > using namespace libcamera; > > -libcamera::Message::Type ThreadRpcMessage::rpcType_ = Message::Type::None; > - > -ThreadRpcMessage::ThreadRpcMessage() > - : Message(type()) > -{ > -} > - > void ThreadRpc::notifyReception() > { > { > @@ -32,11 +24,3 @@ void ThreadRpc::waitDelivery() > libcamera::MutexLocker locker(mutex_); > cv_.wait(locker, [&] { return delivered_; }); > } > - > -Message::Type ThreadRpcMessage::type() > -{ > - if (ThreadRpcMessage::rpcType_ == Message::Type::None) > - rpcType_ = Message::registerMessageType(); > - > - return rpcType_; > -} > diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h > index 6d8992839d0b..f577a5d9fb32 100644 > --- a/src/android/thread_rpc.h > +++ b/src/android/thread_rpc.h > @@ -12,9 +12,6 @@ > > #include <hardware/camera3.h> > > -#include "message.h" > -#include "thread.h" > - > class ThreadRpc > { > public: > @@ -39,16 +36,4 @@ private: > std::condition_variable cv_; > }; > > -class ThreadRpcMessage : public libcamera::Message > -{ > -public: > - ThreadRpcMessage(); > - ThreadRpc *rpc; > - > - static Message::Type type(); > - > -private: > - static libcamera::Message::Type rpcType_; > -}; > - > #endif /* __ANDROID_THREAD_RPC_H__ */ > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On Sat, Aug 17, 2019 at 05:45:11PM +0200, Niklas Söderlund wrote: > Hi Laurent, > > Thanks for your work. > > On 2019-08-12 18:02:32 +0300, Laurent Pinchart wrote: > > Replace the manual implementation of asynchronous method invocation > > through a custom message with Object::invokeMethod(). This simplifies > > the thread RPC implementation. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > The patch itself is good, it removes quite some stuff so it's good to go in my opinion. I'm just wondering, the current CameraProxy::threadRpcCall implemententation basically blocks the caller until the invoked methods has not been executed on the callee. Would it make sense to make out of this patter an invokeMethodsSynchronous() operation? In any case Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > I like the diffstat :-) > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > > This patch applies on top of the "[PATCH 00/18] Object & Thread > > enhancements" series. > > > > src/android/camera_device.cpp | 8 +------- > > src/android/camera_device.h | 4 +++- > > src/android/camera_proxy.cpp | 8 +++----- > > src/android/thread_rpc.cpp | 18 +----------------- > > src/android/thread_rpc.h | 15 --------------- > > 5 files changed, 8 insertions(+), 45 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index e2c1f2a246c8..999c51e6ba4a 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -70,14 +70,8 @@ CameraDevice::~CameraDevice() > > /* > > * Handle RPC request received from the associated proxy. > > */ > > -void CameraDevice::message(Message *message) > > +void CameraDevice::call(ThreadRpc *rpc) > > { > > - if (message->type() != ThreadRpcMessage::type()) > > - return Object::message(message); > > - > > - ThreadRpcMessage *rpcMessage = static_cast<ThreadRpcMessage *>(message); > > - ThreadRpc *rpc = rpcMessage->rpc; > > - > > switch (rpc->tag) { > > case ThreadRpc::ProcessCaptureRequest: > > processCaptureRequest(rpc->request); > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index ac5b95c95104..4d834ceb08a5 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -26,13 +26,15 @@ > > return nullptr; \ > > } while(0); > > > > +class ThreadRpc; > > + > > class CameraDevice : public libcamera::Object > > { > > public: > > CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> &camera); > > ~CameraDevice(); > > > > - void message(libcamera::Message *message); > > + void call(ThreadRpc *rpc); > > > > int open(); > > void close(); > > diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp > > index f0cacac8025b..3eb2f9fbcffb 100644 > > --- a/src/android/camera_proxy.cpp > > +++ b/src/android/camera_proxy.cpp > > @@ -9,6 +9,8 @@ > > > > #include <system/camera_metadata.h> > > > > +#include <libcamera/object.h> > > + > > #include "log.h" > > #include "message.h" > > #include "utils.h" > > @@ -185,10 +187,6 @@ int CameraProxy::processCaptureRequest(camera3_capture_request_t *request) > > > > void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest) > > { > > - std::unique_ptr<ThreadRpcMessage> message = > > - utils::make_unique<ThreadRpcMessage>(); > > - message->rpc = &rpcRequest; > > - > > - cameraDevice_->postMessage(std::move(message)); > > + cameraDevice_->invokeMethod(&CameraDevice::call, &rpcRequest); > > rpcRequest.waitDelivery(); > > } > > diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp > > index 295a05d7c676..f57891ff56bf 100644 > > --- a/src/android/thread_rpc.cpp > > +++ b/src/android/thread_rpc.cpp > > @@ -5,19 +5,11 @@ > > * thread_rpc.cpp - Inter-thread procedure call > > */ > > > > +#include "thread.h" > > #include "thread_rpc.h" > > > > -#include "message.h" > > - > > using namespace libcamera; > > > > -libcamera::Message::Type ThreadRpcMessage::rpcType_ = Message::Type::None; > > - > > -ThreadRpcMessage::ThreadRpcMessage() > > - : Message(type()) > > -{ > > -} > > - > > void ThreadRpc::notifyReception() > > { > > { > > @@ -32,11 +24,3 @@ void ThreadRpc::waitDelivery() > > libcamera::MutexLocker locker(mutex_); > > cv_.wait(locker, [&] { return delivered_; }); > > } > > - > > -Message::Type ThreadRpcMessage::type() > > -{ > > - if (ThreadRpcMessage::rpcType_ == Message::Type::None) > > - rpcType_ = Message::registerMessageType(); > > - > > - return rpcType_; > > -} > > diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h > > index 6d8992839d0b..f577a5d9fb32 100644 > > --- a/src/android/thread_rpc.h > > +++ b/src/android/thread_rpc.h > > @@ -12,9 +12,6 @@ > > > > #include <hardware/camera3.h> > > > > -#include "message.h" > > -#include "thread.h" > > - > > class ThreadRpc > > { > > public: > > @@ -39,16 +36,4 @@ private: > > std::condition_variable cv_; > > }; > > > > -class ThreadRpcMessage : public libcamera::Message > > -{ > > -public: > > - ThreadRpcMessage(); > > - ThreadRpc *rpc; > > - > > - static Message::Type type(); > > - > > -private: > > - static libcamera::Message::Type rpcType_; > > -}; > > - > > #endif /* __ANDROID_THREAD_RPC_H__ */ > > -- > > Regards, > > > > Laurent Pinchart > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Mon, Aug 19, 2019 at 04:44:38PM +0200, Jacopo Mondi wrote: > On Sat, Aug 17, 2019 at 05:45:11PM +0200, Niklas Söderlund wrote: > > On 2019-08-12 18:02:32 +0300, Laurent Pinchart wrote: > >> Replace the manual implementation of asynchronous method invocation > >> through a custom message with Object::invokeMethod(). This simplifies > >> the thread RPC implementation. > >> > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The patch itself is good, it removes quite some stuff so it's good to > go in my opinion. I'm just wondering, the current CameraProxy::threadRpcCall > implemententation basically blocks the caller until the invoked > methods has not been executed on the callee. Would it make sense to > make out of this patter an invokeMethodsSynchronous() operation? It's on my todo list :-) I'm thinking of adding an argument to invokeMethod() (as well to Signal::emit()) to select whether the method or slots should be called immediately (synchronously in the same thread), through the message queue, through the message queue with a synchronous wait, or determine the behaviour automatically depending on the thread of the caller and callee. > In any case > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > I like the diffstat :-) > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > >> --- > >> > >> This patch applies on top of the "[PATCH 00/18] Object & Thread > >> enhancements" series. > >> > >> src/android/camera_device.cpp | 8 +------- > >> src/android/camera_device.h | 4 +++- > >> src/android/camera_proxy.cpp | 8 +++----- > >> src/android/thread_rpc.cpp | 18 +----------------- > >> src/android/thread_rpc.h | 15 --------------- > >> 5 files changed, 8 insertions(+), 45 deletions(-) > >> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >> index e2c1f2a246c8..999c51e6ba4a 100644 > >> --- a/src/android/camera_device.cpp > >> +++ b/src/android/camera_device.cpp > >> @@ -70,14 +70,8 @@ CameraDevice::~CameraDevice() > >> /* > >> * Handle RPC request received from the associated proxy. > >> */ > >> -void CameraDevice::message(Message *message) > >> +void CameraDevice::call(ThreadRpc *rpc) > >> { > >> - if (message->type() != ThreadRpcMessage::type()) > >> - return Object::message(message); > >> - > >> - ThreadRpcMessage *rpcMessage = static_cast<ThreadRpcMessage *>(message); > >> - ThreadRpc *rpc = rpcMessage->rpc; > >> - > >> switch (rpc->tag) { > >> case ThreadRpc::ProcessCaptureRequest: > >> processCaptureRequest(rpc->request); > >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h > >> index ac5b95c95104..4d834ceb08a5 100644 > >> --- a/src/android/camera_device.h > >> +++ b/src/android/camera_device.h > >> @@ -26,13 +26,15 @@ > >> return nullptr; \ > >> } while(0); > >> > >> +class ThreadRpc; > >> + > >> class CameraDevice : public libcamera::Object > >> { > >> public: > >> CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> &camera); > >> ~CameraDevice(); > >> > >> - void message(libcamera::Message *message); > >> + void call(ThreadRpc *rpc); > >> > >> int open(); > >> void close(); > >> diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp > >> index f0cacac8025b..3eb2f9fbcffb 100644 > >> --- a/src/android/camera_proxy.cpp > >> +++ b/src/android/camera_proxy.cpp > >> @@ -9,6 +9,8 @@ > >> > >> #include <system/camera_metadata.h> > >> > >> +#include <libcamera/object.h> > >> + > >> #include "log.h" > >> #include "message.h" > >> #include "utils.h" > >> @@ -185,10 +187,6 @@ int CameraProxy::processCaptureRequest(camera3_capture_request_t *request) > >> > >> void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest) > >> { > >> - std::unique_ptr<ThreadRpcMessage> message = > >> - utils::make_unique<ThreadRpcMessage>(); > >> - message->rpc = &rpcRequest; > >> - > >> - cameraDevice_->postMessage(std::move(message)); > >> + cameraDevice_->invokeMethod(&CameraDevice::call, &rpcRequest); > >> rpcRequest.waitDelivery(); > >> } > >> diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp > >> index 295a05d7c676..f57891ff56bf 100644 > >> --- a/src/android/thread_rpc.cpp > >> +++ b/src/android/thread_rpc.cpp > >> @@ -5,19 +5,11 @@ > >> * thread_rpc.cpp - Inter-thread procedure call > >> */ > >> > >> +#include "thread.h" > >> #include "thread_rpc.h" > >> > >> -#include "message.h" > >> - > >> using namespace libcamera; > >> > >> -libcamera::Message::Type ThreadRpcMessage::rpcType_ = Message::Type::None; > >> - > >> -ThreadRpcMessage::ThreadRpcMessage() > >> - : Message(type()) > >> -{ > >> -} > >> - > >> void ThreadRpc::notifyReception() > >> { > >> { > >> @@ -32,11 +24,3 @@ void ThreadRpc::waitDelivery() > >> libcamera::MutexLocker locker(mutex_); > >> cv_.wait(locker, [&] { return delivered_; }); > >> } > >> - > >> -Message::Type ThreadRpcMessage::type() > >> -{ > >> - if (ThreadRpcMessage::rpcType_ == Message::Type::None) > >> - rpcType_ = Message::registerMessageType(); > >> - > >> - return rpcType_; > >> -} > >> diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h > >> index 6d8992839d0b..f577a5d9fb32 100644 > >> --- a/src/android/thread_rpc.h > >> +++ b/src/android/thread_rpc.h > >> @@ -12,9 +12,6 @@ > >> > >> #include <hardware/camera3.h> > >> > >> -#include "message.h" > >> -#include "thread.h" > >> - > >> class ThreadRpc > >> { > >> public: > >> @@ -39,16 +36,4 @@ private: > >> std::condition_variable cv_; > >> }; > >> > >> -class ThreadRpcMessage : public libcamera::Message > >> -{ > >> -public: > >> - ThreadRpcMessage(); > >> - ThreadRpc *rpc; > >> - > >> - static Message::Type type(); > >> - > >> -private: > >> - static libcamera::Message::Type rpcType_; > >> -}; > >> - > >> #endif /* __ANDROID_THREAD_RPC_H__ */
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index e2c1f2a246c8..999c51e6ba4a 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -70,14 +70,8 @@ CameraDevice::~CameraDevice() /* * Handle RPC request received from the associated proxy. */ -void CameraDevice::message(Message *message) +void CameraDevice::call(ThreadRpc *rpc) { - if (message->type() != ThreadRpcMessage::type()) - return Object::message(message); - - ThreadRpcMessage *rpcMessage = static_cast<ThreadRpcMessage *>(message); - ThreadRpc *rpc = rpcMessage->rpc; - switch (rpc->tag) { case ThreadRpc::ProcessCaptureRequest: processCaptureRequest(rpc->request); diff --git a/src/android/camera_device.h b/src/android/camera_device.h index ac5b95c95104..4d834ceb08a5 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -26,13 +26,15 @@ return nullptr; \ } while(0); +class ThreadRpc; + class CameraDevice : public libcamera::Object { public: CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> &camera); ~CameraDevice(); - void message(libcamera::Message *message); + void call(ThreadRpc *rpc); int open(); void close(); diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp index f0cacac8025b..3eb2f9fbcffb 100644 --- a/src/android/camera_proxy.cpp +++ b/src/android/camera_proxy.cpp @@ -9,6 +9,8 @@ #include <system/camera_metadata.h> +#include <libcamera/object.h> + #include "log.h" #include "message.h" #include "utils.h" @@ -185,10 +187,6 @@ int CameraProxy::processCaptureRequest(camera3_capture_request_t *request) void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest) { - std::unique_ptr<ThreadRpcMessage> message = - utils::make_unique<ThreadRpcMessage>(); - message->rpc = &rpcRequest; - - cameraDevice_->postMessage(std::move(message)); + cameraDevice_->invokeMethod(&CameraDevice::call, &rpcRequest); rpcRequest.waitDelivery(); } diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp index 295a05d7c676..f57891ff56bf 100644 --- a/src/android/thread_rpc.cpp +++ b/src/android/thread_rpc.cpp @@ -5,19 +5,11 @@ * thread_rpc.cpp - Inter-thread procedure call */ +#include "thread.h" #include "thread_rpc.h" -#include "message.h" - using namespace libcamera; -libcamera::Message::Type ThreadRpcMessage::rpcType_ = Message::Type::None; - -ThreadRpcMessage::ThreadRpcMessage() - : Message(type()) -{ -} - void ThreadRpc::notifyReception() { { @@ -32,11 +24,3 @@ void ThreadRpc::waitDelivery() libcamera::MutexLocker locker(mutex_); cv_.wait(locker, [&] { return delivered_; }); } - -Message::Type ThreadRpcMessage::type() -{ - if (ThreadRpcMessage::rpcType_ == Message::Type::None) - rpcType_ = Message::registerMessageType(); - - return rpcType_; -} diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h index 6d8992839d0b..f577a5d9fb32 100644 --- a/src/android/thread_rpc.h +++ b/src/android/thread_rpc.h @@ -12,9 +12,6 @@ #include <hardware/camera3.h> -#include "message.h" -#include "thread.h" - class ThreadRpc { public: @@ -39,16 +36,4 @@ private: std::condition_variable cv_; }; -class ThreadRpcMessage : public libcamera::Message -{ -public: - ThreadRpcMessage(); - ThreadRpc *rpc; - - static Message::Type type(); - -private: - static libcamera::Message::Type rpcType_; -}; - #endif /* __ANDROID_THREAD_RPC_H__ */
Replace the manual implementation of asynchronous method invocation through a custom message with Object::invokeMethod(). This simplifies the thread RPC implementation. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- This patch applies on top of the "[PATCH 00/18] Object & Thread enhancements" series. src/android/camera_device.cpp | 8 +------- src/android/camera_device.h | 4 +++- src/android/camera_proxy.cpp | 8 +++----- src/android/thread_rpc.cpp | 18 +----------------- src/android/thread_rpc.h | 15 --------------- 5 files changed, 8 insertions(+), 45 deletions(-)