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

[Jacopo Mondi]

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 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?

In any case
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j


> 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
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190819/2bc1a3f4/attachment.sig>