[libcamera-devel] [PATCH] hal: Simplify thread RPC with Object::invokeMethod()

[Laurent Pinchart]

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 at 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 at jmondi.org>
> 
> > I like the diffstat :-)
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at 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