[{"id":13046,"web_url":"https://patchwork.libcamera.org/comment/13046/","msgid":"<20201007123000.qsobgtydxdkgyf46@oden.dyn.berto.se>","date":"2020-10-07T12:30:00","subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2020-10-06 18:06:36 +0200, Jacopo Mondi wrote:\n> The CameraWork class creates a Worker instance and runs on an internal\n> thread. The worker waits on a set of fences before queueing a capture\n> requests to the the libcamera::Camera.\n\nI really like this solution!\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_worker.cpp | 67 +++++++++++++++++++++++++++++++\n> src/android/camera_worker.h | 75 +++++++++++++++++++++++++++++++++++\n> src/android/meson.build | 1 +\n> 3 files changed, 143 insertions(+)\n> create mode 100644 src/android/camera_worker.cpp\n> create mode 100644 src/android/camera_worker.h\n> \n> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> new file mode 100644\n> index 000000000000..2bb94775a195\n> --- /dev/null\n> +++ b/src/android/camera_worker.cpp\n> @@ -0,0 +1,67 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * camera_worker.cpp - Process capture request on behalf of the Camera HAL\n> + */\n> +\n> +#include \"camera_worker.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \"camera_device.h\"\n> +\n> +using namespace libcamera;\n> +\n> +CameraWorker::CameraWorker(const std::shared_ptr &camera)\n> +\t: camera_(camera), worker_(camera_.get())\n> +{\n> +\tworker_.moveToThread(&thread_);\n> +\tthread_.start();\n> +}\n> +\n> +void CameraWorker::queueRequest(std::unique_ptr request)\n> +{\n> +\tCaptureRequest *req = request.release();\n> +\tworker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);\n> +}\n> +\n> +int CameraWorker::Worker::waitFence(int fence)\n> +{\n> +\tstruct pollfd fds = { fence, POLLIN, 0 };\n> +\tconstexpr unsigned int timeoutMs = 300;\n> +\tint ret;\n> +\n> +\tdo {\n> +\t\tret = poll(&fds, 1, timeoutMs);\n> +\t\tif (ret == 0)\n> +\t\t\treturn -ETIME;\n> +\n> +\t\tif (ret > 0) {\n> +\t\t\tif (fds.revents & (POLLERR | POLLNVAL))\n> +\t\t\t\treturn -EINVAL;\n> +\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t} while (ret == -1 && (errno == EINTR || errno == EAGAIN));\n> +\n> +\treturn ret;\n> +}\n> +\n> +void CameraWorker::Worker::processRequest(CaptureRequest *request)\n> +{\n> +\tfor (int fence : request->acquireFences_) {\n> +\t\tif (fence == -1)\n> +\t\t\tcontinue;\n> +\n> +\t\tint ret = waitFence(fence);\n> +\t\tif (ret < 0)\n> +\t\t\treturn;\n> +\n> +\t\tclose(fence);\n> +\t}\n\nIf you move this to a new function CaptureRequest::waitFences() you cold \nremove the friend statement :-)\n\n> +\n> +\tcamera_->queueRequest(request->request_);\n> +\tdelete request;\n> +}\n> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> new file mode 100644\n> index 000000000000..24b176d6269c\n> --- /dev/null\n> +++ b/src/android/camera_worker.h\n> @@ -0,0 +1,75 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * camera_worker.h - Process capture request on behalf of the Camera HAL\n> + */\n> +#ifndef __ANDROID_CAMERA_WORKER_H__\n> +#define __ANDROID_CAMERA_WORKER_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/thread.h\"\n> +\n> +class CameraDevice;\n> +\n> +struct CaptureRequest {\n> +\tCaptureRequest(libcamera::Request *request)\n> +\t\t: request_(request)\n> +\t{\n> +\t}\n> +\n> +\tvoid addBuffer(libcamera::Stream *stream,\n> +\t\t libcamera::FrameBuffer *buffer, int fence)\n> +\t{\n> +\t\trequest_->addBuffer(stream, buffer);\n> +\t\tacquireFences_.push_back(fence);\n> +\t}\n\nIf you move these to the .cpp file you can get away with forward \ndeclaration of most of the classes used.\n\n> +\n> +private:\n> +\tfriend class CameraWorker;\n> +\n> +\tstd::vector acquireFences_;\n> +\tlibcamera::Request *request_;\n> +};\n> +\n> +class CameraWorker\n> +{\n> +public:\n> +\tCameraWorker(const std::shared_ptr &camera);\n> +\n> +\tvoid queueRequest(std::unique_ptr request);\n> +\n> +private:\n> +\tclass Worker : public libcamera::Object\n> +\t{\n> +\tpublic:\n> +\t\tWorker(libcamera::Camera *camera)\n> +\t\t\t: camera_(camera)\n> +\t\t{\n> +\t\t}\n> +\t\t~Worker()\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tvoid processRequest(CaptureRequest *request);\n> +\n> +\tprivate:\n> +\t\tint waitFence(int fence);\n> +\n> +\t\tlibcamera::Camera *camera_;\n> +\t};\n> +\n> +\tstd::shared_ptr camera_;\n\nCould you not remove this and store a shared_ptr inside the Worker?\n\n> +\n> +\tWorker worker_;\n> +\tlibcamera::Thread thread_;\n> +};\n> +\n> +#endif /* __ANDROID_CAMERA_WORKER_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 802bb89afe57..b2b2293cf62d 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -21,6 +21,7 @@ android_hal_sources = files([\n> 'camera_metadata.cpp',\n> 'camera_ops.cpp',\n> 'camera_stream.cpp',\n> + 'camera_worker.cpp',\n> 'jpeg/encoder_libjpeg.cpp',\n> 'jpeg/exif.cpp',\n> ])\n> -- \n> 2.28.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 51B4DBEEDF\n\tfor ;\n\tWed, 7 Oct 2020 12:30:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE94F60580;\n\tWed, 7 Oct 2020 14:30:02 +0200 (CEST)","from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBD7860553\n\tfor ;\n\tWed, 7 Oct 2020 14:30:01 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id a4so1745639lji.12\n\tfor ;\n\tWed, 07 Oct 2020 05:30:01 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ti17sm338696lja.45.2020.10.07.05.30.00\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 07 Oct 2020 05:30:00 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"mA70C5iG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=TtNL7px80IhG32Y5gVdqPfHbjXPBX2Zma3N3jN41WG4=;\n\tb=mA70C5iGIonCagZ6UygW/SBNIcUb4x7s5J+mxISI4nRWGOtetH2iNw4UlnxYVohPM2\n\tHik3jtAMMvcZF5B/EBNSSZYB14C8OqnDOEQ7LZtfjLtmBdFgMyC2vhD7zGZ+CoRfddV5\n\tvFqGXCTcdn00jza8DsHtEzGyoctOyIQfbeQ8++PNO3rRsYiNt2MqYENI0kJzM2zWGF/5\n\tX5ctdCeJ5iu8GpQw8Sjj3tsm86wSSxrwX7+FcV9quj7u9gYsVN89GZxOgtecOorBuGU0\n\t0GGMc9iS0V+3rPZCkdAv1krXD03N2VOSY8L9PuhnMHMB+tqAbjb0cYDYXaEjARRg3KIl\n\tCipA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=TtNL7px80IhG32Y5gVdqPfHbjXPBX2Zma3N3jN41WG4=;\n\tb=Ja3XFl9w2Trjbum4k6UKzU4yXLNQrp2HuO8RGl3oTvbJqXXGmpUuAxCqjRaRt3rHVO\n\tom3/nHAPRkoQ2nOj6gnFaWfwHDp89kdBgyr1n/gNNXBLH6m6OoHgp+nU6mglVD04T5Lk\n\tqFD8l1iBC4zL9tb+I1rhWcwZBIy601mhaTi4eHexloPz3RH7pQgmQH7H+N4Y3rL3LyQu\n\tjLzTmu715dwooNNjy9Frs7uGHFPQpxjB1T4jls9vUXDFWrk0Obm4Pc602xTbaMRbDuHH\n\tCq9EfiSCneCEIxmWB37Nd2VaieqSN49Cqa85inhFSoO52F0ux89cTfgym1sZdxwN3OJ8\n\tyYYQ==","X-Gm-Message-State":"AOAM532qL7cRb7TU95err+t8NkLhytDkfsDvi6/lbUxJmB3fZ1NqdbHd\n\t6flOqibvcjQZOyzAThujpqZZKonoLYQRrQ==","X-Google-Smtp-Source":"ABdhPJy7dtQOjfC1Wy6EYh0mxqyBheA4otQPGA/Ojia8VMvW4CEmLNEw2ANlHbMT9bF+hecIsmK8ew==","X-Received":"by 2002:a2e:9ad9:: with SMTP id p25mr999182ljj.256.1602073801336;\n\tWed, 07 Oct 2020 05:30:01 -0700 (PDT)","Date":"Wed, 7 Oct 2020 14:30:00 +0200","From":"Niklas =?utf-8?q?S=C3=B6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20201007123000.qsobgtydxdkgyf46@oden.dyn.berto.se>","References":"<20201006160637.29841-1-jacopo@jmondi.org>\n\t<20201006160637.29841-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201006160637.29841-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13067,"web_url":"https://patchwork.libcamera.org/comment/13067/","msgid":"<20201007133257.wpntwph3nattov7m@uno.localdomain>","date":"2020-10-07T13:32:57","subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Oct 07, 2020 at 02:30:00PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2020-10-06 18:06:36 +0200, Jacopo Mondi wrote:\n> > The CameraWork class creates a Worker instance and runs on an internal\n> > thread. The worker waits on a set of fences before queueing a capture\n> > requests to the the libcamera::Camera.\n>\n> I really like this solution!\n>\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_worker.cpp | 67 +++++++++++++++++++++++++++++++\n> > src/android/camera_worker.h | 75 +++++++++++++++++++++++++++++++++++\n> > src/android/meson.build | 1 +\n> > 3 files changed, 143 insertions(+)\n> > create mode 100644 src/android/camera_worker.cpp\n> > create mode 100644 src/android/camera_worker.h\n> >\n> > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> > new file mode 100644\n> > index 000000000000..2bb94775a195\n> > --- /dev/null\n> > +++ b/src/android/camera_worker.cpp\n> > @@ -0,0 +1,67 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * camera_worker.cpp - Process capture request on behalf of the Camera HAL\n> > + */\n> > +\n> > +#include \"camera_worker.h\"\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"camera_device.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +CameraWorker::CameraWorker(const std::shared_ptr &camera)\n> > +\t: camera_(camera), worker_(camera_.get())\n> > +{\n> > +\tworker_.moveToThread(&thread_);\n> > +\tthread_.start();\n> > +}\n> > +\n> > +void CameraWorker::queueRequest(std::unique_ptr request)\n> > +{\n> > +\tCaptureRequest *req = request.release();\n> > +\tworker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);\n> > +}\n> > +\n> > +int CameraWorker::Worker::waitFence(int fence)\n> > +{\n> > +\tstruct pollfd fds = { fence, POLLIN, 0 };\n> > +\tconstexpr unsigned int timeoutMs = 300;\n> > +\tint ret;\n> > +\n> > +\tdo {\n> > +\t\tret = poll(&fds, 1, timeoutMs);\n> > +\t\tif (ret == 0)\n> > +\t\t\treturn -ETIME;\n> > +\n> > +\t\tif (ret > 0) {\n> > +\t\t\tif (fds.revents & (POLLERR | POLLNVAL))\n> > +\t\t\t\treturn -EINVAL;\n> > +\n> > +\t\t\treturn 0;\n> > +\t\t}\n> > +\t} while (ret == -1 && (errno == EINTR || errno == EAGAIN));\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +void CameraWorker::Worker::processRequest(CaptureRequest *request)\n> > +{\n> > +\tfor (int fence : request->acquireFences_) {\n> > +\t\tif (fence == -1)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tint ret = waitFence(fence);\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn;\n> > +\n> > +\t\tclose(fence);\n> > +\t}\n>\n> If you move this to a new function CaptureRequest::waitFences() you cold\n> remove the friend statement :-)\n>\n\nHow can I do so ? A CameraRequest is created by the CameraDevice and\nmoved to the CameraWorker. Then the CameraWorker schedules it on its\nworker_ which runs in a dedicated thread (that's why it can wait\nwithout interrupting the HAL thread).\n\nShould I moveToThread() every CaptureRequest we receive ? It's\npotentially a costly operation as it interrupts the thread's even\ndispatcher and potentially moves messages from one queue to another.\n\nI don't think it's worth it ?\n\n> > +\n> > +\tcamera_->queueRequest(request->request_);\n> > +\tdelete request;\n> > +}\n> > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> > new file mode 100644\n> > index 000000000000..24b176d6269c\n> > --- /dev/null\n> > +++ b/src/android/camera_worker.h\n> > @@ -0,0 +1,75 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * camera_worker.h - Process capture request on behalf of the Camera HAL\n> > + */\n> > +#ifndef __ANDROID_CAMERA_WORKER_H__\n> > +#define __ANDROID_CAMERA_WORKER_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/thread.h\"\n> > +\n> > +class CameraDevice;\n> > +\n> > +struct CaptureRequest {\n> > +\tCaptureRequest(libcamera::Request *request)\n> > +\t\t: request_(request)\n> > +\t{\n> > +\t}\n> > +\n> > +\tvoid addBuffer(libcamera::Stream *stream,\n> > +\t\t libcamera::FrameBuffer *buffer, int fence)\n> > +\t{\n> > +\t\trequest_->addBuffer(stream, buffer);\n> > +\t\tacquireFences_.push_back(fence);\n> > +\t}\n>\n> If you move these to the .cpp file you can get away with forward\n> declaration of most of the classes used.\n\nNot really, as I can't forward declare symbols from a namespace.\n\n>\n> > +\n> > +private:\n> > +\tfriend class CameraWorker;\n> > +\n> > +\tstd::vector acquireFences_;\n> > +\tlibcamera::Request *request_;\n> > +};\n> > +\n> > +class CameraWorker\n> > +{\n> > +public:\n> > +\tCameraWorker(const std::shared_ptr &camera);\n> > +\n> > +\tvoid queueRequest(std::unique_ptr request);\n> > +\n> > +private:\n> > +\tclass Worker : public libcamera::Object\n> > +\t{\n> > +\tpublic:\n> > +\t\tWorker(libcamera::Camera *camera)\n> > +\t\t\t: camera_(camera)\n> > +\t\t{\n> > +\t\t}\n> > +\t\t~Worker()\n> > +\t\t{\n> > +\t\t}\n> > +\n> > +\t\tvoid processRequest(CaptureRequest *request);\n> > +\n> > +\tprivate:\n> > +\t\tint waitFence(int fence);\n> > +\n> > +\t\tlibcamera::Camera *camera_;\n> > +\t};\n> > +\n> > +\tstd::shared_ptr camera_;\n>\n> Could you not remove this and store a shared_ptr inside the Worker?\n>\n\nmmm, yes I probably should, there's no point in having the\nshared_ptr<> here and passing the raw pointer to the worker.\n\nThanks\n j\n\n> > +\n> > +\tWorker worker_;\n> > +\tlibcamera::Thread thread_;\n> > +};\n> > +\n> > +#endif /* __ANDROID_CAMERA_WORKER_H__ */\n> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > index 802bb89afe57..b2b2293cf62d 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -21,6 +21,7 @@ android_hal_sources = files([\n> > 'camera_metadata.cpp',\n> > 'camera_ops.cpp',\n> > 'camera_stream.cpp',\n> > + 'camera_worker.cpp',\n> > 'jpeg/encoder_libjpeg.cpp',\n> > 'jpeg/exif.cpp',\n> > ])\n> > --\n> > 2.28.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E7C07BEEE0\n\tfor ;\n\tWed, 7 Oct 2020 13:28:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8F814605BF;\n\tWed, 7 Oct 2020 15:28:59 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 05980605BE\n\tfor ;\n\tWed, 7 Oct 2020 15:28:57 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 700A06000C;\n\tWed, 7 Oct 2020 13:28:56 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 7 Oct 2020 15:32:57 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201007133257.wpntwph3nattov7m@uno.localdomain>","References":"<20201006160637.29841-1-jacopo@jmondi.org>\n\t<20201006160637.29841-2-jacopo@jmondi.org>\n\t<20201007123000.qsobgtydxdkgyf46@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201007123000.qsobgtydxdkgyf46@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13085,"web_url":"https://patchwork.libcamera.org/comment/13085/","msgid":"<20201007161332.dqpash72gg4igmwo@oden.dyn.berto.se>","date":"2020-10-07T16:13:32","subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2020-10-07 15:32:57 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Wed, Oct 07, 2020 at 02:30:00PM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2020-10-06 18:06:36 +0200, Jacopo Mondi wrote:\n> > > The CameraWork class creates a Worker instance and runs on an internal\n> > > thread. The worker waits on a set of fences before queueing a capture\n> > > requests to the the libcamera::Camera.\n> >\n> > I really like this solution!\n> >\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_worker.cpp | 67 +++++++++++++++++++++++++++++++\n> > > src/android/camera_worker.h | 75 +++++++++++++++++++++++++++++++++++\n> > > src/android/meson.build | 1 +\n> > > 3 files changed, 143 insertions(+)\n> > > create mode 100644 src/android/camera_worker.cpp\n> > > create mode 100644 src/android/camera_worker.h\n> > >\n> > > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> > > new file mode 100644\n> > > index 000000000000..2bb94775a195\n> > > --- /dev/null\n> > > +++ b/src/android/camera_worker.cpp\n> > > @@ -0,0 +1,67 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * camera_worker.cpp - Process capture request on behalf of the Camera HAL\n> > > + */\n> > > +\n> > > +#include \"camera_worker.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"camera_device.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +CameraWorker::CameraWorker(const std::shared_ptr &camera)\n> > > +\t: camera_(camera), worker_(camera_.get())\n> > > +{\n> > > +\tworker_.moveToThread(&thread_);\n> > > +\tthread_.start();\n> > > +}\n> > > +\n> > > +void CameraWorker::queueRequest(std::unique_ptr request)\n> > > +{\n> > > +\tCaptureRequest *req = request.release();\n> > > +\tworker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);\n> > > +}\n> > > +\n> > > +int CameraWorker::Worker::waitFence(int fence)\n> > > +{\n> > > +\tstruct pollfd fds = { fence, POLLIN, 0 };\n> > > +\tconstexpr unsigned int timeoutMs = 300;\n> > > +\tint ret;\n> > > +\n> > > +\tdo {\n> > > +\t\tret = poll(&fds, 1, timeoutMs);\n> > > +\t\tif (ret == 0)\n> > > +\t\t\treturn -ETIME;\n> > > +\n> > > +\t\tif (ret > 0) {\n> > > +\t\t\tif (fds.revents & (POLLERR | POLLNVAL))\n> > > +\t\t\t\treturn -EINVAL;\n> > > +\n> > > +\t\t\treturn 0;\n> > > +\t\t}\n> > > +\t} while (ret == -1 && (errno == EINTR || errno == EAGAIN));\n> > > +\n> > > +\treturn ret;\n> > > +}\n> > > +\n> > > +void CameraWorker::Worker::processRequest(CaptureRequest *request)\n> > > +{\n> > > +\tfor (int fence : request->acquireFences_) {\n> > > +\t\tif (fence == -1)\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tint ret = waitFence(fence);\n> > > +\t\tif (ret < 0)\n> > > +\t\t\treturn;\n> > > +\n> > > +\t\tclose(fence);\n> > > +\t}\n> >\n> > If you move this to a new function CaptureRequest::waitFences() you cold\n> > remove the friend statement :-)\n> >\n> \n> How can I do so ? A CameraRequest is created by the CameraDevice and\n> moved to the CameraWorker. Then the CameraWorker schedules it on its\n> worker_ which runs in a dedicated thread (that's why it can wait\n> without interrupting the HAL thread).\n> \n> Should I moveToThread() every CaptureRequest we receive ? It's\n> potentially a costly operation as it interrupts the thread's even\n> dispatcher and potentially moves messages from one queue to another.\n> \n> I don't think it's worth it ?\n\nSorry I think I expressed myself unclear. What I meant to say was that \nthe for loop could be moved to a function inside CaptureRequest that \nwould be called here instead of the loop. The two lines below and the \nfunction itself would remain. Or am I missing something?\n\n> \n> > > +\n> > > +\tcamera_->queueRequest(request->request_);\n> > > +\tdelete request;\n> > > +}\n> > > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> > > new file mode 100644\n> > > index 000000000000..24b176d6269c\n> > > --- /dev/null\n> > > +++ b/src/android/camera_worker.h\n> > > @@ -0,0 +1,75 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * camera_worker.h - Process capture request on behalf of the Camera HAL\n> > > + */\n> > > +#ifndef __ANDROID_CAMERA_WORKER_H__\n> > > +#define __ANDROID_CAMERA_WORKER_H__\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/thread.h\"\n> > > +\n> > > +class CameraDevice;\n> > > +\n> > > +struct CaptureRequest {\n> > > +\tCaptureRequest(libcamera::Request *request)\n> > > +\t\t: request_(request)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\tvoid addBuffer(libcamera::Stream *stream,\n> > > +\t\t libcamera::FrameBuffer *buffer, int fence)\n> > > +\t{\n> > > +\t\trequest_->addBuffer(stream, buffer);\n> > > +\t\tacquireFences_.push_back(fence);\n> > > +\t}\n> >\n> > If you move these to the .cpp file you can get away with forward\n> > declaration of most of the classes used.\n> \n> Not really, as I can't forward declare symbols from a namespace.\n\nGood point, did not think of that. But for the record you can forward \ndeclare in a different namespace,\n\n namespace libcamera {\n class Request;\n }\n\nBut I'm not sure I like how it looks ;-)\n\n> \n> >\n> > > +\n> > > +private:\n> > > +\tfriend class CameraWorker;\n> > > +\n> > > +\tstd::vector acquireFences_;\n> > > +\tlibcamera::Request *request_;\n> > > +};\n> > > +\n> > > +class CameraWorker\n> > > +{\n> > > +public:\n> > > +\tCameraWorker(const std::shared_ptr &camera);\n> > > +\n> > > +\tvoid queueRequest(std::unique_ptr request);\n> > > +\n> > > +private:\n> > > +\tclass Worker : public libcamera::Object\n> > > +\t{\n> > > +\tpublic:\n> > > +\t\tWorker(libcamera::Camera *camera)\n> > > +\t\t\t: camera_(camera)\n> > > +\t\t{\n> > > +\t\t}\n> > > +\t\t~Worker()\n> > > +\t\t{\n> > > +\t\t}\n> > > +\n> > > +\t\tvoid processRequest(CaptureRequest *request);\n> > > +\n> > > +\tprivate:\n> > > +\t\tint waitFence(int fence);\n> > > +\n> > > +\t\tlibcamera::Camera *camera_;\n> > > +\t};\n> > > +\n> > > +\tstd::shared_ptr camera_;\n> >\n> > Could you not remove this and store a shared_ptr inside the Worker?\n> >\n> \n> mmm, yes I probably should, there's no point in having the\n> shared_ptr<> here and passing the raw pointer to the worker.\n> \n> Thanks\n> j\n> \n> > > +\n> > > +\tWorker worker_;\n> > > +\tlibcamera::Thread thread_;\n> > > +};\n> > > +\n> > > +#endif /* __ANDROID_CAMERA_WORKER_H__ */\n> > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > index 802bb89afe57..b2b2293cf62d 100644\n> > > --- a/src/android/meson.build\n> > > +++ b/src/android/meson.build\n> > > @@ -21,6 +21,7 @@ android_hal_sources = files([\n> > > 'camera_metadata.cpp',\n> > > 'camera_ops.cpp',\n> > > 'camera_stream.cpp',\n> > > + 'camera_worker.cpp',\n> > > 'jpeg/encoder_libjpeg.cpp',\n> > > 'jpeg/exif.cpp',\n> > > ])\n> > > --\n> > > 2.28.0\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DFCD4BEEDF\n\tfor ;\n\tWed, 7 Oct 2020 16:13:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67F05607C7;\n\tWed, 7 Oct 2020 18:13:35 +0200 (CEST)","from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 159E2605BB\n\tfor ;\n\tWed, 7 Oct 2020 18:13:34 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id r24so2559268ljm.3\n\tfor ;\n\tWed, 07 Oct 2020 09:13:34 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tm10sm418654ljg.138.2020.10.07.09.13.32\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 07 Oct 2020 09:13:32 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"kRFobT04\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=hpNja8xoRKT6Xq1D67xT19h+7IGtkQGUP6du3isFjIs=;\n\tb=kRFobT04Aj1PuzEcA8g2d5pn7T4VKoGmplcB5O3871AekJrgpc7ff6mbzFAnYoYNl4\n\tw6qKT3CVSg4G/LdpZbLsPK4urnjSFDyF5XEhfpd4TH0sf6DiamYCiEXmZqG6PurMwadV\n\tGOz6WnzCS9tqItnqO8M1EQdA36cqn54uPQLPxeb9tNJFHRS7UQoF6Y+tqa3t5hE84uV8\n\trvrC3HQ8lVldneIwSttpgvYcgZpvpKd8W+mGgGryFCDUlIqHgd33qfU7y8Gat2hzQ54X\n\tpmuE52iqMCBR1YRwRXKu1yYJea9Nt92ESST5ZfIIzxmAuaP3cb+2u50UXYRH1FM7v2Qy\n\thdew==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=hpNja8xoRKT6Xq1D67xT19h+7IGtkQGUP6du3isFjIs=;\n\tb=FAeJP+duspDNY6lnxu+4unxFHvkYQ+snQ2kVRBnCH5RAP8srOBI3YBdieDCVjfnNxm\n\tW39weXq6h7VJSXeuP9Nc9Ir+Mue+09BWuUZuPtFQbNqOCSD3nfIEsVMtM4YRSdd2Y6gi\n\tiyl59eTOhbKjZA9yLvbelhBZ6vLpediOTHaYGN5xxIMFgaYDUvybPrCwoUzAINFDRxvA\n\t0783fC9z7W5M+/nAp+/A5wiJ7ZmnNIqU9Pj9KJuFd/a01w7UK8cG4yMW49F3UBxWtR4O\n\thya52XJhCV/9ntnfewStD+a2OHU9pPslYOtxzxZdsy505hyDnh7JjYExd3iND+zc1lE8\n\tnmMg==","X-Gm-Message-State":"AOAM532+czgdi3Woxeac7ClLZ3PyzUs8B9fQgucEP7KMmSmJbZKH4/E2\n\tT1lqVLg6RiOiwc0v8WZHKx3OnDgGAtJ2CA==","X-Google-Smtp-Source":"ABdhPJwQQ2fEXCvshKFaRfnmdDbIDPLc1jcL4Ffc62AO0GffMjyuhXI28+wIeZQoa7G2zeqEznSbhw==","X-Received":"by 2002:a2e:9ad3:: with SMTP id\n\tp19mr1420870ljj.374.1602087213360; \n\tWed, 07 Oct 2020 09:13:33 -0700 (PDT)","Date":"Wed, 7 Oct 2020 18:13:32 +0200","From":"Niklas =?utf-8?q?S=C3=B6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20201007161332.dqpash72gg4igmwo@oden.dyn.berto.se>","References":"<20201006160637.29841-1-jacopo@jmondi.org>\n\t<20201006160637.29841-2-jacopo@jmondi.org>\n\t<20201007123000.qsobgtydxdkgyf46@oden.dyn.berto.se>\n\t<20201007133257.wpntwph3nattov7m@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201007133257.wpntwph3nattov7m@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13086,"web_url":"https://patchwork.libcamera.org/comment/13086/","msgid":"<20201007162735.7hutnjwzeg2btt7p@uno.localdomain>","date":"2020-10-07T16:27:35","subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Oct 07, 2020 at 06:13:32PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> On 2020-10-07 15:32:57 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Wed, Oct 07, 2020 at 02:30:00PM +0200, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2020-10-06 18:06:36 +0200, Jacopo Mondi wrote:\n> > > > The CameraWork class creates a Worker instance and runs on an internal\n> > > > thread. The worker waits on a set of fences before queueing a capture\n> > > > requests to the the libcamera::Camera.\n> > >\n> > > I really like this solution!\n> > >\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/android/camera_worker.cpp | 67 +++++++++++++++++++++++++++++++\n> > > > src/android/camera_worker.h | 75 +++++++++++++++++++++++++++++++++++\n> > > > src/android/meson.build | 1 +\n> > > > 3 files changed, 143 insertions(+)\n> > > > create mode 100644 src/android/camera_worker.cpp\n> > > > create mode 100644 src/android/camera_worker.h\n> > > >\n> > > > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..2bb94775a195\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_worker.cpp\n> > > > @@ -0,0 +1,67 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Google Inc.\n> > > > + *\n> > > > + * camera_worker.cpp - Process capture request on behalf of the Camera HAL\n> > > > + */\n> > > > +\n> > > > +#include \"camera_worker.h\"\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"camera_device.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +CameraWorker::CameraWorker(const std::shared_ptr &camera)\n> > > > +\t: camera_(camera), worker_(camera_.get())\n> > > > +{\n> > > > +\tworker_.moveToThread(&thread_);\n> > > > +\tthread_.start();\n> > > > +}\n> > > > +\n> > > > +void CameraWorker::queueRequest(std::unique_ptr request)\n> > > > +{\n> > > > +\tCaptureRequest *req = request.release();\n> > > > +\tworker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);\n> > > > +}\n> > > > +\n> > > > +int CameraWorker::Worker::waitFence(int fence)\n> > > > +{\n> > > > +\tstruct pollfd fds = { fence, POLLIN, 0 };\n> > > > +\tconstexpr unsigned int timeoutMs = 300;\n> > > > +\tint ret;\n> > > > +\n> > > > +\tdo {\n> > > > +\t\tret = poll(&fds, 1, timeoutMs);\n> > > > +\t\tif (ret == 0)\n> > > > +\t\t\treturn -ETIME;\n> > > > +\n> > > > +\t\tif (ret > 0) {\n> > > > +\t\t\tif (fds.revents & (POLLERR | POLLNVAL))\n> > > > +\t\t\t\treturn -EINVAL;\n> > > > +\n> > > > +\t\t\treturn 0;\n> > > > +\t\t}\n> > > > +\t} while (ret == -1 && (errno == EINTR || errno == EAGAIN));\n> > > > +\n> > > > +\treturn ret;\n> > > > +}\n> > > > +\n> > > > +void CameraWorker::Worker::processRequest(CaptureRequest *request)\n> > > > +{\n> > > > +\tfor (int fence : request->acquireFences_) {\n> > > > +\t\tif (fence == -1)\n> > > > +\t\t\tcontinue;\n> > > > +\n> > > > +\t\tint ret = waitFence(fence);\n> > > > +\t\tif (ret < 0)\n> > > > +\t\t\treturn;\n> > > > +\n> > > > +\t\tclose(fence);\n> > > > +\t}\n> > >\n> > > If you move this to a new function CaptureRequest::waitFences() you cold\n> > > remove the friend statement :-)\n> > >\n> >\n> > How can I do so ? A CameraRequest is created by the CameraDevice and\n> > moved to the CameraWorker. Then the CameraWorker schedules it on its\n> > worker_ which runs in a dedicated thread (that's why it can wait\n> > without interrupting the HAL thread).\n> >\n> > Should I moveToThread() every CaptureRequest we receive ? It's\n> > potentially a costly operation as it interrupts the thread's even\n> > dispatcher and potentially moves messages from one queue to another.\n> >\n> > I don't think it's worth it ?\n>\n> Sorry I think I expressed myself unclear. What I meant to say was that\n> the for loop could be moved to a function inside CaptureRequest that\n> would be called here instead of the loop. The two lines below and the\n> function itself would remain. Or am I missing something?\n>\n\nPlease not that also request->request_ is part of the CaptureRequest,\nso also the two lines below should be moved there.\n\nAlso, CaptureRequest is created in the HAL thread so it would need to\ninvokeMethod() on a CameraWorker::Worker to delegate there the\nwaitFence() call. As I've pictured it it seems more complex that using\na friend statement, but I might be missing something.\n\n> >\n> > > > +\n> > > > +\tcamera_->queueRequest(request->request_);\n> > > > +\tdelete request;\n> > > > +}\n> > > > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> > > > new file mode 100644\n> > > > index 000000000000..24b176d6269c\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_worker.h\n> > > > @@ -0,0 +1,75 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Google Inc.\n> > > > + *\n> > > > + * camera_worker.h - Process capture request on behalf of the Camera HAL\n> > > > + */\n> > > > +#ifndef __ANDROID_CAMERA_WORKER_H__\n> > > > +#define __ANDROID_CAMERA_WORKER_H__\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/thread.h\"\n> > > > +\n> > > > +class CameraDevice;\n> > > > +\n> > > > +struct CaptureRequest {\n> > > > +\tCaptureRequest(libcamera::Request *request)\n> > > > +\t\t: request_(request)\n> > > > +\t{\n> > > > +\t}\n> > > > +\n> > > > +\tvoid addBuffer(libcamera::Stream *stream,\n> > > > +\t\t libcamera::FrameBuffer *buffer, int fence)\n> > > > +\t{\n> > > > +\t\trequest_->addBuffer(stream, buffer);\n> > > > +\t\tacquireFences_.push_back(fence);\n> > > > +\t}\n> > >\n> > > If you move these to the .cpp file you can get away with forward\n> > > declaration of most of the classes used.\n> >\n> > Not really, as I can't forward declare symbols from a namespace.\n>\n> Good point, did not think of that. But for the record you can forward\n> declare in a different namespace,\n>\n> namespace libcamera {\n> class Request;\n> }\n>\n> But I'm not sure I like how it looks ;-)\n\n:/ not super nice\n\n>\n> >\n> > >\n> > > > +\n> > > > +private:\n> > > > +\tfriend class CameraWorker;\n> > > > +\n> > > > +\tstd::vector acquireFences_;\n> > > > +\tlibcamera::Request *request_;\n> > > > +};\n> > > > +\n> > > > +class CameraWorker\n> > > > +{\n> > > > +public:\n> > > > +\tCameraWorker(const std::shared_ptr &camera);\n> > > > +\n> > > > +\tvoid queueRequest(std::unique_ptr request);\n> > > > +\n> > > > +private:\n> > > > +\tclass Worker : public libcamera::Object\n> > > > +\t{\n> > > > +\tpublic:\n> > > > +\t\tWorker(libcamera::Camera *camera)\n> > > > +\t\t\t: camera_(camera)\n> > > > +\t\t{\n> > > > +\t\t}\n> > > > +\t\t~Worker()\n> > > > +\t\t{\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tvoid processRequest(CaptureRequest *request);\n> > > > +\n> > > > +\tprivate:\n> > > > +\t\tint waitFence(int fence);\n> > > > +\n> > > > +\t\tlibcamera::Camera *camera_;\n> > > > +\t};\n> > > > +\n> > > > +\tstd::shared_ptr camera_;\n> > >\n> > > Could you not remove this and store a shared_ptr inside the Worker?\n> > >\n> >\n> > mmm, yes I probably should, there's no point in having the\n> > shared_ptr<> here and passing the raw pointer to the worker.\n> >\n> > Thanks\n> > j\n> >\n> > > > +\n> > > > +\tWorker worker_;\n> > > > +\tlibcamera::Thread thread_;\n> > > > +};\n> > > > +\n> > > > +#endif /* __ANDROID_CAMERA_WORKER_H__ */\n> > > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > > index 802bb89afe57..b2b2293cf62d 100644\n> > > > --- a/src/android/meson.build\n> > > > +++ b/src/android/meson.build\n> > > > @@ -21,6 +21,7 @@ android_hal_sources = files([\n> > > > 'camera_metadata.cpp',\n> > > > 'camera_ops.cpp',\n> > > > 'camera_stream.cpp',\n> > > > + 'camera_worker.cpp',\n> > > > 'jpeg/encoder_libjpeg.cpp',\n> > > > 'jpeg/exif.cpp',\n> > > > ])\n> > > > --\n> > > > 2.28.0\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B996CBEEE0\n\tfor ;\n\tWed, 7 Oct 2020 16:23:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 41B6360890;\n\tWed, 7 Oct 2020 18:23:38 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A7B0160733\n\tfor ;\n\tWed, 7 Oct 2020 18:23:37 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 14999E000B;\n\tWed, 7 Oct 2020 16:23:35 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 7 Oct 2020 18:27:35 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201007162735.7hutnjwzeg2btt7p@uno.localdomain>","References":"<20201006160637.29841-1-jacopo@jmondi.org>\n\t<20201006160637.29841-2-jacopo@jmondi.org>\n\t<20201007123000.qsobgtydxdkgyf46@oden.dyn.berto.se>\n\t<20201007133257.wpntwph3nattov7m@uno.localdomain>\n\t<20201007161332.dqpash72gg4igmwo@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201007161332.dqpash72gg4igmwo@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13096,"web_url":"https://patchwork.libcamera.org/comment/13096/","msgid":"<20201008040522.GV3937@pendragon.ideasonboard.com>","date":"2020-10-08T04:05:22","subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Oct 06, 2020 at 06:06:36PM +0200, Jacopo Mondi wrote:\n> The CameraWork class creates a Worker instance and runs on an internal\n\ns/CameraWork/CameraWorker/\n\n> thread. The worker waits on a set of fences before queueing a capture\n> requests to the the libcamera::Camera.\n\ns/the the/the/\n\nYou may want to copy some of the text from the cover letter to explain\nwhy this is needed.\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_worker.cpp | 67 +++++++++++++++++++++++++++++++\n> src/android/camera_worker.h | 75 +++++++++++++++++++++++++++++++++++\n> src/android/meson.build | 1 +\n> 3 files changed, 143 insertions(+)\n> create mode 100644 src/android/camera_worker.cpp\n> create mode 100644 src/android/camera_worker.h\n> \n> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> new file mode 100644\n> index 000000000000..2bb94775a195\n> --- /dev/null\n> +++ b/src/android/camera_worker.cpp\n> @@ -0,0 +1,67 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * camera_worker.cpp - Process capture request on behalf of the Camera HAL\n> + */\n> +\n> +#include \"camera_worker.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \"camera_device.h\"\n> +\n> +using namespace libcamera;\n> +\n> +CameraWorker::CameraWorker(const std::shared_ptr &camera)\n> +\t: camera_(camera), worker_(camera_.get())\n> +{\n> +\tworker_.moveToThread(&thread_);\n> +\tthread_.start();\n> +}\n\nYou need to stop the thread before deleting it. I would actually expose\nstart and stop through the API, as there's no need to have a thread\nrunning when the camera is closed.\n\n> +\n> +void CameraWorker::queueRequest(std::unique_ptr request)\n> +{\n> +\tCaptureRequest *req = request.release();\n> +\tworker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);\n\nAnother important reason to stop the thread manually is to make sure to\nsynchronize its message queue, as we want to guarantee that all requests\nwill be freed.\n\n> +}\n> +\n> +int CameraWorker::Worker::waitFence(int fence)\n> +{\n> +\tstruct pollfd fds = { fence, POLLIN, 0 };\n> +\tconstexpr unsigned int timeoutMs = 300;\n\nThat's rather arbitrary, I wonder how we should pick the timeout, but\nfor now it should be good enough.\n\n> +\tint ret;\n> +\n> +\tdo {\n> +\t\tret = poll(&fds, 1, timeoutMs);\n> +\t\tif (ret == 0)\n> +\t\t\treturn -ETIME;\n> +\n> +\t\tif (ret > 0) {\n> +\t\t\tif (fds.revents & (POLLERR | POLLNVAL))\n> +\t\t\t\treturn -EINVAL;\n> +\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t} while (ret == -1 && (errno == EINTR || errno == EAGAIN));\n> +\n> +\treturn ret;\n> +}\n> +\n> +void CameraWorker::Worker::processRequest(CaptureRequest *request)\n> +{\n> +\tfor (int fence : request->acquireFences_) {\n> +\t\tif (fence == -1)\n> +\t\t\tcontinue;\n> +\n> +\t\tint ret = waitFence(fence);\n\nIt would be possible to optimize this by waiting on all the fences at\nthe same time to minimize syscalls when multiple fences are ready at the\nsame time, but that may not be worth it.\n\n> +\t\tif (ret < 0)\n> +\t\t\treturn;\n> +\n> +\t\tclose(fence);\n> +\t}\n> +\n> +\tcamera_->queueRequest(request->request_);\n> +\tdelete request;\n> +}\n> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> new file mode 100644\n> index 000000000000..24b176d6269c\n> --- /dev/null\n> +++ b/src/android/camera_worker.h\n> @@ -0,0 +1,75 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * camera_worker.h - Process capture request on behalf of the Camera HAL\n> + */\n> +#ifndef __ANDROID_CAMERA_WORKER_H__\n> +#define __ANDROID_CAMERA_WORKER_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/thread.h\"\n> +\n> +class CameraDevice;\n> +\n> +struct CaptureRequest {\n> +\tCaptureRequest(libcamera::Request *request)\n> +\t\t: request_(request)\n> +\t{\n> +\t}\n> +\n> +\tvoid addBuffer(libcamera::Stream *stream,\n> +\t\t libcamera::FrameBuffer *buffer, int fence)\n> +\t{\n> +\t\trequest_->addBuffer(stream, buffer);\n> +\t\tacquireFences_.push_back(fence);\n> +\t}\n\nThe API isn't amazing, but that's because the best solution will be to\nadd fences support to the FrameBuffer class. Until we do so, this\nworkaround is fine.\n\nOverall I think this is totally fine for now.\n\n> +\n> +private:\n> +\tfriend class CameraWorker;\n> +\n> +\tstd::vector acquireFences_;\n> +\tlibcamera::Request *request_;\n> +};\n> +\n> +class CameraWorker\n> +{\n> +public:\n> +\tCameraWorker(const std::shared_ptr &camera);\n> +\n> +\tvoid queueRequest(std::unique_ptr request);\n> +\n> +private:\n> +\tclass Worker : public libcamera::Object\n> +\t{\n> +\tpublic:\n> +\t\tWorker(libcamera::Camera *camera)\n> +\t\t\t: camera_(camera)\n> +\t\t{\n> +\t\t}\n> +\t\t~Worker()\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tvoid processRequest(CaptureRequest *request);\n> +\n> +\tprivate:\n> +\t\tint waitFence(int fence);\n> +\n> +\t\tlibcamera::Camera *camera_;\n> +\t};\n> +\n> +\tstd::shared_ptr camera_;\n> +\n> +\tWorker worker_;\n> +\tlibcamera::Thread thread_;\n> +};\n> +\n> +#endif /* __ANDROID_CAMERA_WORKER_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 802bb89afe57..b2b2293cf62d 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -21,6 +21,7 @@ android_hal_sources = files([\n> 'camera_metadata.cpp',\n> 'camera_ops.cpp',\n> 'camera_stream.cpp',\n> + 'camera_worker.cpp',\n> 'jpeg/encoder_libjpeg.cpp',\n> 'jpeg/exif.cpp',\n> ])","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0B0A5BEEDF\n\tfor ;\n\tThu, 8 Oct 2020 04:06:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9337D605B7;\n\tThu, 8 Oct 2020 06:06:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5176A60356\n\tfor ;\n\tThu, 8 Oct 2020 06:06:05 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AE04759E;\n\tThu, 8 Oct 2020 06:06:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EFAmLlSu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602129964;\n\tbh=/rYezhG80sitHpMcXXPwF+94w4z0CLrpn9YXZG05d44=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EFAmLlSurbCR/bfKkiX+hIosTg2DXUGjFBTnlUilK7DjpQ7i+XjIhanmCxjmv6OTN\n\toRvR+IPgRZsuyPBhV7E+pgjtQ1JyKSyype4/woPRm/eEfgaCd81eXMSJbfvbO/oSTY\n\tsOGUSgv0l4n34KBi9M9Yhh+GfOurHCUXCpTXcIlM=","Date":"Thu, 8 Oct 2020 07:05:22 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201008040522.GV3937@pendragon.ideasonboard.com>","References":"<20201006160637.29841-1-jacopo@jmondi.org>\n\t<20201006160637.29841-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201006160637.29841-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13100,"web_url":"https://patchwork.libcamera.org/comment/13100/","msgid":"<20201008083624.gzpxyr3axlryefdg@uno.localdomain>","date":"2020-10-08T08:36:24","subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Oct 08, 2020 at 07:05:22AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Oct 06, 2020 at 06:06:36PM +0200, Jacopo Mondi wrote:\n> > The CameraWork class creates a Worker instance and runs on an internal\n>\n> s/CameraWork/CameraWorker/\n>\n> > thread. The worker waits on a set of fences before queueing a capture\n> > requests to the the libcamera::Camera.\n>\n> s/the the/the/\n>\n> You may want to copy some of the text from the cover letter to explain\n> why this is needed.\n>\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_worker.cpp | 67 +++++++++++++++++++++++++++++++\n> > src/android/camera_worker.h | 75 +++++++++++++++++++++++++++++++++++\n> > src/android/meson.build | 1 +\n> > 3 files changed, 143 insertions(+)\n> > create mode 100644 src/android/camera_worker.cpp\n> > create mode 100644 src/android/camera_worker.h\n> >\n> > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> > new file mode 100644\n> > index 000000000000..2bb94775a195\n> > --- /dev/null\n> > +++ b/src/android/camera_worker.cpp\n> > @@ -0,0 +1,67 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * camera_worker.cpp - Process capture request on behalf of the Camera HAL\n> > + */\n> > +\n> > +#include \"camera_worker.h\"\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"camera_device.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +CameraWorker::CameraWorker(const std::shared_ptr &camera)\n> > +\t: camera_(camera), worker_(camera_.get())\n> > +{\n> > +\tworker_.moveToThread(&thread_);\n> > +\tthread_.start();\n> > +}\n>\n> You need to stop the thread before deleting it. I would actually expose\n> start and stop through the API, as there's no need to have a thread\n> running when the camera is closed.\n>\n\nGood idea!\nI'll add start() and stop()\n\n> > +\n> > +void CameraWorker::queueRequest(std::unique_ptr request)\n> > +{\n> > +\tCaptureRequest *req = request.release();\n> > +\tworker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);\n>\n> Another important reason to stop the thread manually is to make sure to\n> synchronize its message queue, as we want to guarantee that all requests\n> will be freed.\n>\n\nmmm, so Thread exposes a start() and an exit() functions.\n\nstart() runs the underlying thread and make the connected event dispatcher\nprocess events. exit() interrupts the event dispatcher only, then the\nthread will be actually stopped at destruction time.\n\nAs we don't run events in the notifier, but rather poll manually here,\nis there any value in calling exit() ?\n\n> > +}\n> > +\n> > +int CameraWorker::Worker::waitFence(int fence)\n> > +{\n> > +\tstruct pollfd fds = { fence, POLLIN, 0 };\n> > +\tconstexpr unsigned int timeoutMs = 300;\n>\n> That's rather arbitrary, I wonder how we should pick the timeout, but\n> for now it should be good enough.\n\nYou got me... Copyed from the Rockchip HAL implementation.\nQuite arbitrary indeed!\n\n>\n> > +\tint ret;\n> > +\n> > +\tdo {\n> > +\t\tret = poll(&fds, 1, timeoutMs);\n> > +\t\tif (ret == 0)\n> > +\t\t\treturn -ETIME;\n> > +\n> > +\t\tif (ret > 0) {\n> > +\t\t\tif (fds.revents & (POLLERR | POLLNVAL))\n> > +\t\t\t\treturn -EINVAL;\n> > +\n> > +\t\t\treturn 0;\n> > +\t\t}\n> > +\t} while (ret == -1 && (errno == EINTR || errno == EAGAIN));\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +void CameraWorker::Worker::processRequest(CaptureRequest *request)\n> > +{\n> > +\tfor (int fence : request->acquireFences_) {\n> > +\t\tif (fence == -1)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tint ret = waitFence(fence);\n>\n> It would be possible to optimize this by waiting on all the fences at\n> the same time to minimize syscalls when multiple fences are ready at the\n> same time, but that may not be worth it.\n>\n\nI considered several options here, including using a custom event\nnotifier that accepts a set of fds instead of a single one to exploit\nour notifier/dispatcher infrastructure. Unfortunately, that would\nrequire manualy tracking of the number of completed fences here, or\nthe implementation of a custom dispatcher that works on a set of fds\nand only signal completion when all of them are done.\n\nAt the same time I considered instrumenting poll to work with multiple\nfds instead of going through a syscall for each one of them. In this\ncase too, manual tracking of the number of completed requests would be\nrequired, and once I considered that we deal with up to 3 buffers, and\nmost of the time with a single one, I considered writing such an\nerror-prone and quite-harsh to debug code not worth it.\n\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn;\n> > +\n> > +\t\tclose(fence);\n> > +\t}\n> > +\n> > +\tcamera_->queueRequest(request->request_);\n> > +\tdelete request;\n> > +}\n> > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> > new file mode 100644\n> > index 000000000000..24b176d6269c\n> > --- /dev/null\n> > +++ b/src/android/camera_worker.h\n> > @@ -0,0 +1,75 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * camera_worker.h - Process capture request on behalf of the Camera HAL\n> > + */\n> > +#ifndef __ANDROID_CAMERA_WORKER_H__\n> > +#define __ANDROID_CAMERA_WORKER_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/thread.h\"\n> > +\n> > +class CameraDevice;\n> > +\n> > +struct CaptureRequest {\n> > +\tCaptureRequest(libcamera::Request *request)\n> > +\t\t: request_(request)\n> > +\t{\n> > +\t}\n> > +\n> > +\tvoid addBuffer(libcamera::Stream *stream,\n> > +\t\t libcamera::FrameBuffer *buffer, int fence)\n> > +\t{\n> > +\t\trequest_->addBuffer(stream, buffer);\n> > +\t\tacquireFences_.push_back(fence);\n> > +\t}\n>\n> The API isn't amazing, but that's because the best solution will be to\n> add fences support to the FrameBuffer class. Until we do so, this\n> workaround is fine.\n>\n\nTo be honest I don't mind the intruduction of CaptureRequest in the\nCameraDevice, the rest I agree, it's awful.\n\nA proper library-wide solution is needed, but this closes a gap and\ndoes not introduce any techincal debt\n\nSo I do as well think that:\n> Overall I think this is totally fine for now.\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 91DB0BEEDF\n\tfor ;\n\tThu, 8 Oct 2020 08:32:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CE5B605BD;\n\tThu, 8 Oct 2020 10:32:26 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E562460358\n\tfor ;\n\tThu, 8 Oct 2020 10:32:24 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 652AF1BF210;\n\tThu, 8 Oct 2020 08:32:24 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 8 Oct 2020 10:36:24 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201008083624.gzpxyr3axlryefdg@uno.localdomain>","References":"<20201006160637.29841-1-jacopo@jmondi.org>\n\t<20201006160637.29841-2-jacopo@jmondi.org>\n\t<20201008040522.GV3937@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201008040522.GV3937@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13115,"web_url":"https://patchwork.libcamera.org/comment/13115/","msgid":"<20201008203326.GG3939@pendragon.ideasonboard.com>","date":"2020-10-08T20:33:26","subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Oct 08, 2020 at 10:36:24AM +0200, Jacopo Mondi wrote:\n> On Thu, Oct 08, 2020 at 07:05:22AM +0300, Laurent Pinchart wrote:\n> > On Tue, Oct 06, 2020 at 06:06:36PM +0200, Jacopo Mondi wrote:\n> > > The CameraWork class creates a Worker instance and runs on an internal\n> >\n> > s/CameraWork/CameraWorker/\n> >\n> > > thread. The worker waits on a set of fences before queueing a capture\n> > > requests to the the libcamera::Camera.\n> >\n> > s/the the/the/\n> >\n> > You may want to copy some of the text from the cover letter to explain\n> > why this is needed.\n> >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_worker.cpp | 67 +++++++++++++++++++++++++++++++\n> > > src/android/camera_worker.h | 75 +++++++++++++++++++++++++++++++++++\n> > > src/android/meson.build | 1 +\n> > > 3 files changed, 143 insertions(+)\n> > > create mode 100644 src/android/camera_worker.cpp\n> > > create mode 100644 src/android/camera_worker.h\n> > >\n> > > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> > > new file mode 100644\n> > > index 000000000000..2bb94775a195\n> > > --- /dev/null\n> > > +++ b/src/android/camera_worker.cpp\n> > > @@ -0,0 +1,67 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * camera_worker.cpp - Process capture request on behalf of the Camera HAL\n> > > + */\n> > > +\n> > > +#include \"camera_worker.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"camera_device.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +CameraWorker::CameraWorker(const std::shared_ptr &camera)\n> > > +\t: camera_(camera), worker_(camera_.get())\n> > > +{\n> > > +\tworker_.moveToThread(&thread_);\n> > > +\tthread_.start();\n> > > +}\n> >\n> > You need to stop the thread before deleting it. I would actually expose\n> > start and stop through the API, as there's no need to have a thread\n> > running when the camera is closed.\n> \n> Good idea!\n> I'll add start() and stop()\n> \n> > > +\n> > > +void CameraWorker::queueRequest(std::unique_ptr request)\n> > > +{\n> > > +\tCaptureRequest *req = request.release();\n> > > +\tworker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);\n> >\n> > Another important reason to stop the thread manually is to make sure to\n> > synchronize its message queue, as we want to guarantee that all requests\n> > will be freed.\n> \n> mmm, so Thread exposes a start() and an exit() functions.\n> \n> start() runs the underlying thread and make the connected event dispatcher\n> process events. exit() interrupts the event dispatcher only, then the\n> thread will be actually stopped at destruction time.\n> \n> As we don't run events in the notifier, but rather poll manually here,\n> is there any value in calling exit() ?\n\nUnless the Thread::run() method gets overridden (which isn't he case\nhere), it calls Thread::exec(), which runs an event loop. Thread::exit()\ninstructs the event loop to exit, which is required, otherwise the\nthread will never stop and Thread::wait() will block forever.\nThread::wait() is also mandatory to call before destroying the Thread\nobject.\n\nWe can't override Thread::run() to not call exec(), as we rely on the\nevent loop. That's how the messages used by Object::invokeMethod() are\ndispatched.\n\nThe conclusion is that we need to call exit() and wait() before\ndestroying the thread (this should happen in CameraWorker::stop()). We\nalso need to make sure that all messages posted to the thread's message\nqueue by worker_.invokeMethod() are handled before the thread is\nstopped. There are two sides to it:\n\n- We need to ensure that no messages get posted after calling\n Thread::exit(). This means not calling CameraWorker::queueRequest()\n after CameraWorker::stop(). Given that both functions are called from\n the camera service thread there should be no race condition, and given\n that stop() will be called when the camera is closed, queueRequest()\n won't be called anymore. I think we're safe here.\n\n- I believe there's a race condition in the thread class. Even if\n Thread::exit() and Thread::postMessage() (the latter called from\n Object::invokeMethod) are call from a single thread, the message could\n be added to the queue, and exit() could instruct Thread::exect() to\n return before the event dispatcher gets a change to dispatch messages.\n Thread::exec() contains\n\n\twhile (!data_->exit_.load(std::memory_order_acquire))\n\t\tdispatcher->processEvents();\n\n and EventDispatcherPoll::processEvents() calls\n Thread::dispatchMessages() at the very beginning, before sleeping.\n\n We don't implement flush() in the HAL, which I think would be related\n to the solution to this issue.\n\n Am I overthinking this, or is it a real issue ?\n\n> > > +}\n> > > +\n> > > +int CameraWorker::Worker::waitFence(int fence)\n> > > +{\n> > > +\tstruct pollfd fds = { fence, POLLIN, 0 };\n> > > +\tconstexpr unsigned int timeoutMs = 300;\n> >\n> > That's rather arbitrary, I wonder how we should pick the timeout, but\n> > for now it should be good enough.\n> \n> You got me... Copyed from the Rockchip HAL implementation.\n> Quite arbitrary indeed!\n\nMaybe a \\todo would help remembering the task ?\n\n> > > +\tint ret;\n> > > +\n> > > +\tdo {\n> > > +\t\tret = poll(&fds, 1, timeoutMs);\n> > > +\t\tif (ret == 0)\n> > > +\t\t\treturn -ETIME;\n> > > +\n> > > +\t\tif (ret > 0) {\n> > > +\t\t\tif (fds.revents & (POLLERR | POLLNVAL))\n> > > +\t\t\t\treturn -EINVAL;\n> > > +\n> > > +\t\t\treturn 0;\n> > > +\t\t}\n> > > +\t} while (ret == -1 && (errno == EINTR || errno == EAGAIN));\n> > > +\n> > > +\treturn ret;\n> > > +}\n> > > +\n> > > +void CameraWorker::Worker::processRequest(CaptureRequest *request)\n> > > +{\n> > > +\tfor (int fence : request->acquireFences_) {\n> > > +\t\tif (fence == -1)\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tint ret = waitFence(fence);\n> >\n> > It would be possible to optimize this by waiting on all the fences at\n> > the same time to minimize syscalls when multiple fences are ready at the\n> > same time, but that may not be worth it.\n> \n> I considered several options here, including using a custom event\n> notifier that accepts a set of fds instead of a single one to exploit\n> our notifier/dispatcher infrastructure. Unfortunately, that would\n> require manualy tracking of the number of completed fences here, or\n> the implementation of a custom dispatcher that works on a set of fds\n> and only signal completion when all of them are done.\n> \n> At the same time I considered instrumenting poll to work with multiple\n> fds instead of going through a syscall for each one of them. In this\n> case too, manual tracking of the number of completed requests would be\n> required, and once I considered that we deal with up to 3 buffers, and\n> most of the time with a single one, I considered writing such an\n> error-prone and quite-harsh to debug code not worth it.\n\nAck.\n\n> > > +\t\tif (ret < 0)\n> > > +\t\t\treturn;\n> > > +\n> > > +\t\tclose(fence);\n> > > +\t}\n> > > +\n> > > +\tcamera_->queueRequest(request->request_);\n> > > +\tdelete request;\n> > > +}\n> > > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> > > new file mode 100644\n> > > index 000000000000..24b176d6269c\n> > > --- /dev/null\n> > > +++ b/src/android/camera_worker.h\n> > > @@ -0,0 +1,75 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * camera_worker.h - Process capture request on behalf of the Camera HAL\n> > > + */\n> > > +#ifndef __ANDROID_CAMERA_WORKER_H__\n> > > +#define __ANDROID_CAMERA_WORKER_H__\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/thread.h\"\n> > > +\n> > > +class CameraDevice;\n> > > +\n> > > +struct CaptureRequest {\n> > > +\tCaptureRequest(libcamera::Request *request)\n> > > +\t\t: request_(request)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\tvoid addBuffer(libcamera::Stream *stream,\n> > > +\t\t libcamera::FrameBuffer *buffer, int fence)\n> > > +\t{\n> > > +\t\trequest_->addBuffer(stream, buffer);\n> > > +\t\tacquireFences_.push_back(fence);\n> > > +\t}\n> >\n> > The API isn't amazing, but that's because the best solution will be to\n> > add fences support to the FrameBuffer class. Until we do so, this\n> > workaround is fine.\n> \n> To be honest I don't mind the intruduction of CaptureRequest in the\n> CameraDevice, the rest I agree, it's awful.\n\nIt's not awful :-) It's just sub-optimal, and that's fine. I think\nyou've struct the right balance, the implementation does the job, it's\nclean and lean, not over-designed, and probably as good as it can get\nuntil this gets moved to the libcamera core.\n\n> A proper library-wide solution is needed, but this closes a gap and\n> does not introduce any techincal debt\n> \n> So I do as well think that:\n>\n> > Overall I think this is totally fine for now.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 89726BEEDF\n\tfor ;\n\tThu, 8 Oct 2020 20:34:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16570606FD;\n\tThu, 8 Oct 2020 22:34:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A508A60361\n\tfor ;\n\tThu, 8 Oct 2020 22:34:09 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1537159E;\n\tThu, 8 Oct 2020 22:34:09 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ko5/4fJf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602189249;\n\tbh=3CwJ7VTfNWU8my7o9hma443k73OY29yA/bL57vzQEks=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ko5/4fJf4/Ubg1xiUIy+VAvHqObSjW19A8uBo++L6JvVTN038BaJhGGTirMZDjXYM\n\tcevxLDs06bo6LdBbWSM1rkLvs362TBFPX9LaStjhJYd83bXcLAKapfVkWFXX+KJU7Y\n\tIt+zn/f4/HQCGHkXOJCQje0vPEiHcbLngLqjDKv8=","Date":"Thu, 8 Oct 2020 23:33:26 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201008203326.GG3939@pendragon.ideasonboard.com>","References":"<20201006160637.29841-1-jacopo@jmondi.org>\n\t<20201006160637.29841-2-jacopo@jmondi.org>\n\t<20201008040522.GV3937@pendragon.ideasonboard.com>\n\t<20201008083624.gzpxyr3axlryefdg@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201008083624.gzpxyr3axlryefdg@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13123,"web_url":"https://patchwork.libcamera.org/comment/13123/","msgid":"<20201009093159.5eqsin2szbjhzeqf@uno.localdomain>","date":"2020-10-09T09:31:59","subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Oct 08, 2020 at 11:33:26PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Thu, Oct 08, 2020 at 10:36:24AM +0200, Jacopo Mondi wrote:\n> > On Thu, Oct 08, 2020 at 07:05:22AM +0300, Laurent Pinchart wrote:\n> > > On Tue, Oct 06, 2020 at 06:06:36PM +0200, Jacopo Mondi wrote:\n> > > > The CameraWork class creates a Worker instance and runs on an internal\n> > >\n> > > s/CameraWork/CameraWorker/\n> > >\n> > > > thread. The worker waits on a set of fences before queueing a capture\n> > > > requests to the the libcamera::Camera.\n> > >\n> > > s/the the/the/\n> > >\n> > > You may want to copy some of the text from the cover letter to explain\n> > > why this is needed.\n> > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/android/camera_worker.cpp | 67 +++++++++++++++++++++++++++++++\n> > > > src/android/camera_worker.h | 75 +++++++++++++++++++++++++++++++++++\n> > > > src/android/meson.build | 1 +\n> > > > 3 files changed, 143 insertions(+)\n> > > > create mode 100644 src/android/camera_worker.cpp\n> > > > create mode 100644 src/android/camera_worker.h\n> > > >\n> > > > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..2bb94775a195\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_worker.cpp\n> > > > @@ -0,0 +1,67 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Google Inc.\n> > > > + *\n> > > > + * camera_worker.cpp - Process capture request on behalf of the Camera HAL\n> > > > + */\n> > > > +\n> > > > +#include \"camera_worker.h\"\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"camera_device.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +CameraWorker::CameraWorker(const std::shared_ptr &camera)\n> > > > +\t: camera_(camera), worker_(camera_.get())\n> > > > +{\n> > > > +\tworker_.moveToThread(&thread_);\n> > > > +\tthread_.start();\n> > > > +}\n> > >\n> > > You need to stop the thread before deleting it. I would actually expose\n> > > start and stop through the API, as there's no need to have a thread\n> > > running when the camera is closed.\n> >\n> > Good idea!\n> > I'll add start() and stop()\n> >\n> > > > +\n> > > > +void CameraWorker::queueRequest(std::unique_ptr request)\n> > > > +{\n> > > > +\tCaptureRequest *req = request.release();\n> > > > +\tworker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);\n> > >\n> > > Another important reason to stop the thread manually is to make sure to\n> > > synchronize its message queue, as we want to guarantee that all requests\n> > > will be freed.\n> >\n> > mmm, so Thread exposes a start() and an exit() functions.\n> >\n> > start() runs the underlying thread and make the connected event dispatcher\n> > process events. exit() interrupts the event dispatcher only, then the\n> > thread will be actually stopped at destruction time.\n> >\n> > As we don't run events in the notifier, but rather poll manually here,\n> > is there any value in calling exit() ?\n>\n> Unless the Thread::run() method gets overridden (which isn't he case\n> here), it calls Thread::exec(), which runs an event loop. Thread::exit()\n> instructs the event loop to exit, which is required, otherwise the\n> thread will never stop and Thread::wait() will block forever.\n> Thread::wait() is also mandatory to call before destroying the Thread\n> object.\n>\n\nRight, we poll manually, but we dispatch messages which indeed run on\nthe event loop\n\n> We can't override Thread::run() to not call exec(), as we rely on the\n> event loop. That's how the messages used by Object::invokeMethod() are\n> dispatched.\n>\n> The conclusion is that we need to call exit() and wait() before\n> destroying the thread (this should happen in CameraWorker::stop()). We\n> also need to make sure that all messages posted to the thread's message\n> queue by worker_.invokeMethod() are handled before the thread is\n> stopped. There are two sides to it:\n>\n> - We need to ensure that no messages get posted after calling\n> Thread::exit(). This means not calling CameraWorker::queueRequest()\n> after CameraWorker::stop(). Given that both functions are called from\n> the camera service thread there should be no race condition, and given\n> that stop() will be called when the camera is closed, queueRequest()\n> won't be called anymore. I think we're safe here.\n\nI agree we are.\n\n>\n> - I believe there's a race condition in the thread class. Even if\n> Thread::exit() and Thread::postMessage() (the latter called from\n> Object::invokeMethod) are call from a single thread, the message could\n> be added to the queue, and exit() could instruct Thread::exect() to\n> return before the event dispatcher gets a change to dispatch messages.\n> Thread::exec() contains\n>\n> \twhile (!data_->exit_.load(std::memory_order_acquire))\n> \t\tdispatcher->processEvents();\n>\n> and EventDispatcherPoll::processEvents() calls\n> Thread::dispatchMessages() at the very beginning, before sleeping.\n>\n> We don't implement flush() in the HAL, which I think would be related\n> to the solution to this issue.\n\nflush seems to be meant to be called before a new configuration\n\n * on the given device. The framework will use this to dump all state as\n * quickly as possible in order to prepare for a configure_streams() call.\n\nNot in the teardown path.\n\nIf I got you right we might hit a small (not that small actually)\nwindown where a message is added to the thread's queue but never\nprocessed as a Thread::exit() just after the queueing happens\ninterrupts the dispatcher for good.\n\nAs far as the HAL is concerned that pending request should be signaled\nas ERROR before the HAL::stop() completes ? In this case, would\ninstrumenting wait() with an additional message queue swip help ? We\ncan emit a signal for every non-handled message..\n\n>\n> Am I overthinking this, or is it a real issue ?\n>\n> > > > +}\n> > > > +\n> > > > +int CameraWorker::Worker::waitFence(int fence)\n> > > > +{\n> > > > +\tstruct pollfd fds = { fence, POLLIN, 0 };\n> > > > +\tconstexpr unsigned int timeoutMs = 300;\n> > >\n> > > That's rather arbitrary, I wonder how we should pick the timeout, but\n> > > for now it should be good enough.\n> >\n> > You got me... Copyed from the Rockchip HAL implementation.\n> > Quite arbitrary indeed!\n>\n> Maybe a \\todo would help remembering the task ?\n>\n> > > > +\tint ret;\n> > > > +\n> > > > +\tdo {\n> > > > +\t\tret = poll(&fds, 1, timeoutMs);\n> > > > +\t\tif (ret == 0)\n> > > > +\t\t\treturn -ETIME;\n> > > > +\n> > > > +\t\tif (ret > 0) {\n> > > > +\t\t\tif (fds.revents & (POLLERR | POLLNVAL))\n> > > > +\t\t\t\treturn -EINVAL;\n> > > > +\n> > > > +\t\t\treturn 0;\n> > > > +\t\t}\n> > > > +\t} while (ret == -1 && (errno == EINTR || errno == EAGAIN));\n> > > > +\n> > > > +\treturn ret;\n> > > > +}\n> > > > +\n> > > > +void CameraWorker::Worker::processRequest(CaptureRequest *request)\n> > > > +{\n> > > > +\tfor (int fence : request->acquireFences_) {\n> > > > +\t\tif (fence == -1)\n> > > > +\t\t\tcontinue;\n> > > > +\n> > > > +\t\tint ret = waitFence(fence);\n> > >\n> > > It would be possible to optimize this by waiting on all the fences at\n> > > the same time to minimize syscalls when multiple fences are ready at the\n> > > same time, but that may not be worth it.\n> >\n> > I considered several options here, including using a custom event\n> > notifier that accepts a set of fds instead of a single one to exploit\n> > our notifier/dispatcher infrastructure. Unfortunately, that would\n> > require manualy tracking of the number of completed fences here, or\n> > the implementation of a custom dispatcher that works on a set of fds\n> > and only signal completion when all of them are done.\n> >\n> > At the same time I considered instrumenting poll to work with multiple\n> > fds instead of going through a syscall for each one of them. In this\n> > case too, manual tracking of the number of completed requests would be\n> > required, and once I considered that we deal with up to 3 buffers, and\n> > most of the time with a single one, I considered writing such an\n> > error-prone and quite-harsh to debug code not worth it.\n>\n> Ack.\n>\n> > > > +\t\tif (ret < 0)\n> > > > +\t\t\treturn;\n> > > > +\n> > > > +\t\tclose(fence);\n> > > > +\t}\n> > > > +\n> > > > +\tcamera_->queueRequest(request->request_);\n> > > > +\tdelete request;\n> > > > +}\n> > > > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> > > > new file mode 100644\n> > > > index 000000000000..24b176d6269c\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_worker.h\n> > > > @@ -0,0 +1,75 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Google Inc.\n> > > > + *\n> > > > + * camera_worker.h - Process capture request on behalf of the Camera HAL\n> > > > + */\n> > > > +#ifndef __ANDROID_CAMERA_WORKER_H__\n> > > > +#define __ANDROID_CAMERA_WORKER_H__\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/thread.h\"\n> > > > +\n> > > > +class CameraDevice;\n> > > > +\n> > > > +struct CaptureRequest {\n> > > > +\tCaptureRequest(libcamera::Request *request)\n> > > > +\t\t: request_(request)\n> > > > +\t{\n> > > > +\t}\n> > > > +\n> > > > +\tvoid addBuffer(libcamera::Stream *stream,\n> > > > +\t\t libcamera::FrameBuffer *buffer, int fence)\n> > > > +\t{\n> > > > +\t\trequest_->addBuffer(stream, buffer);\n> > > > +\t\tacquireFences_.push_back(fence);\n> > > > +\t}\n> > >\n> > > The API isn't amazing, but that's because the best solution will be to\n> > > add fences support to the FrameBuffer class. Until we do so, this\n> > > workaround is fine.\n> >\n> > To be honest I don't mind the intruduction of CaptureRequest in the\n> > CameraDevice, the rest I agree, it's awful.\n>\n> It's not awful :-) It's just sub-optimal, and that's fine. I think\n> you've struct the right balance, the implementation does the job, it's\n> clean and lean, not over-designed, and probably as good as it can get\n> until this gets moved to the libcamera core.\n>\n> > A proper library-wide solution is needed, but this closes a gap and\n> > does not introduce any techincal debt\n> >\n> > So I do as well think that:\n> >\n> > > Overall I think this is totally fine for now.\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 24875BEEDF\n\tfor ;\n\tFri, 9 Oct 2020 09:28:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B65BC60725;\n\tFri, 9 Oct 2020 11:28:01 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 54F1960357\n\tfor ;\n\tFri, 9 Oct 2020 11:28:00 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id CC51E60008;\n\tFri, 9 Oct 2020 09:27:59 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 9 Oct 2020 11:31:59 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201009093159.5eqsin2szbjhzeqf@uno.localdomain>","References":"<20201006160637.29841-1-jacopo@jmondi.org>\n\t<20201006160637.29841-2-jacopo@jmondi.org>\n\t<20201008040522.GV3937@pendragon.ideasonboard.com>\n\t<20201008083624.gzpxyr3axlryefdg@uno.localdomain>\n\t<20201008203326.GG3939@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201008203326.GG3939@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13156,"web_url":"https://patchwork.libcamera.org/comment/13156/","msgid":"<20201009225134.GL25040@pendragon.ideasonboard.com>","date":"2020-10-09T22:51:34","subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Oct 09, 2020 at 11:31:59AM +0200, Jacopo Mondi wrote:\n> On Thu, Oct 08, 2020 at 11:33:26PM +0300, Laurent Pinchart wrote:\n> > On Thu, Oct 08, 2020 at 10:36:24AM +0200, Jacopo Mondi wrote:\n> > > On Thu, Oct 08, 2020 at 07:05:22AM +0300, Laurent Pinchart wrote:\n> > > > On Tue, Oct 06, 2020 at 06:06:36PM +0200, Jacopo Mondi wrote:\n> > > > > The CameraWork class creates a Worker instance and runs on an internal\n> > > >\n> > > > s/CameraWork/CameraWorker/\n> > > >\n> > > > > thread. The worker waits on a set of fences before queueing a capture\n> > > > > requests to the the libcamera::Camera.\n> > > >\n> > > > s/the the/the/\n> > > >\n> > > > You may want to copy some of the text from the cover letter to explain\n> > > > why this is needed.\n> > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/android/camera_worker.cpp | 67 +++++++++++++++++++++++++++++++\n> > > > > src/android/camera_worker.h | 75 +++++++++++++++++++++++++++++++++++\n> > > > > src/android/meson.build | 1 +\n> > > > > 3 files changed, 143 insertions(+)\n> > > > > create mode 100644 src/android/camera_worker.cpp\n> > > > > create mode 100644 src/android/camera_worker.h\n> > > > >\n> > > > > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> > > > > new file mode 100644\n> > > > > index 000000000000..2bb94775a195\n> > > > > --- /dev/null\n> > > > > +++ b/src/android/camera_worker.cpp\n> > > > > @@ -0,0 +1,67 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2020, Google Inc.\n> > > > > + *\n> > > > > + * camera_worker.cpp - Process capture request on behalf of the Camera HAL\n> > > > > + */\n> > > > > +\n> > > > > +#include \"camera_worker.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \"camera_device.h\"\n> > > > > +\n> > > > > +using namespace libcamera;\n> > > > > +\n> > > > > +CameraWorker::CameraWorker(const std::shared_ptr &camera)\n> > > > > +\t: camera_(camera), worker_(camera_.get())\n> > > > > +{\n> > > > > +\tworker_.moveToThread(&thread_);\n> > > > > +\tthread_.start();\n> > > > > +}\n> > > >\n> > > > You need to stop the thread before deleting it. I would actually expose\n> > > > start and stop through the API, as there's no need to have a thread\n> > > > running when the camera is closed.\n> > >\n> > > Good idea!\n> > > I'll add start() and stop()\n> > >\n> > > > > +\n> > > > > +void CameraWorker::queueRequest(std::unique_ptr request)\n> > > > > +{\n> > > > > +\tCaptureRequest *req = request.release();\n> > > > > +\tworker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);\n> > > >\n> > > > Another important reason to stop the thread manually is to make sure to\n> > > > synchronize its message queue, as we want to guarantee that all requests\n> > > > will be freed.\n> > >\n> > > mmm, so Thread exposes a start() and an exit() functions.\n> > >\n> > > start() runs the underlying thread and make the connected event dispatcher\n> > > process events. exit() interrupts the event dispatcher only, then the\n> > > thread will be actually stopped at destruction time.\n> > >\n> > > As we don't run events in the notifier, but rather poll manually here,\n> > > is there any value in calling exit() ?\n> >\n> > Unless the Thread::run() method gets overridden (which isn't he case\n> > here), it calls Thread::exec(), which runs an event loop. Thread::exit()\n> > instructs the event loop to exit, which is required, otherwise the\n> > thread will never stop and Thread::wait() will block forever.\n> > Thread::wait() is also mandatory to call before destroying the Thread\n> > object.\n> \n> Right, we poll manually, but we dispatch messages which indeed run on\n> the event loop\n> \n> > We can't override Thread::run() to not call exec(), as we rely on the\n> > event loop. That's how the messages used by Object::invokeMethod() are\n> > dispatched.\n> >\n> > The conclusion is that we need to call exit() and wait() before\n> > destroying the thread (this should happen in CameraWorker::stop()). We\n> > also need to make sure that all messages posted to the thread's message\n> > queue by worker_.invokeMethod() are handled before the thread is\n> > stopped. There are two sides to it:\n> >\n> > - We need to ensure that no messages get posted after calling\n> > Thread::exit(). This means not calling CameraWorker::queueRequest()\n> > after CameraWorker::stop(). Given that both functions are called from\n> > the camera service thread there should be no race condition, and given\n> > that stop() will be called when the camera is closed, queueRequest()\n> > won't be called anymore. I think we're safe here.\n> \n> I agree we are.\n> \n> >\n> > - I believe there's a race condition in the thread class. Even if\n> > Thread::exit() and Thread::postMessage() (the latter called from\n> > Object::invokeMethod) are call from a single thread, the message could\n> > be added to the queue, and exit() could instruct Thread::exect() to\n> > return before the event dispatcher gets a change to dispatch messages.\n> > Thread::exec() contains\n> >\n> > \twhile (!data_->exit_.load(std::memory_order_acquire))\n> > \t\tdispatcher->processEvents();\n> >\n> > and EventDispatcherPoll::processEvents() calls\n> > Thread::dispatchMessages() at the very beginning, before sleeping.\n> >\n> > We don't implement flush() in the HAL, which I think would be related\n> > to the solution to this issue.\n> \n> flush seems to be meant to be called before a new configuration\n> \n> * on the given device. The framework will use this to dump all state as\n> * quickly as possible in order to prepare for a configure_streams() call.\n> \n> Not in the teardown path.\n\nIndeed. I however wonder if in practice there will always be a flush()\nbefore close().\n\n> If I got you right we might hit a small (not that small actually)\n> windown where a message is added to the thread's queue but never\n> processed as a Thread::exit() just after the queueing happens\n> interrupts the dispatcher for good.\n\nCorrect.\n\n> As far as the HAL is concerned that pending request should be signaled\n> as ERROR before the HAL::stop() completes ? In this case, would\n> instrumenting wait() with an additional message queue swip help ? We\n> can emit a signal for every non-handled message..\n\nHow does that work on the camera service side though ? Does it expect\nclose() to block until all requests are returned ? Can it even support\nrequest completion callbacks while close() is in progress ? The android\ncamera HAL API doesn't have an explicit stop(), it's pretty annoying :-S\n\n> > Am I overthinking this, or is it a real issue ?\n> >\n> > > > > +}\n> > > > > +\n> > > > > +int CameraWorker::Worker::waitFence(int fence)\n> > > > > +{\n> > > > > +\tstruct pollfd fds = { fence, POLLIN, 0 };\n> > > > > +\tconstexpr unsigned int timeoutMs = 300;\n> > > >\n> > > > That's rather arbitrary, I wonder how we should pick the timeout, but\n> > > > for now it should be good enough.\n> > >\n> > > You got me... Copyed from the Rockchip HAL implementation.\n> > > Quite arbitrary indeed!\n> >\n> > Maybe a \\todo would help remembering the task ?\n> >\n> > > > > +\tint ret;\n> > > > > +\n> > > > > +\tdo {\n> > > > > +\t\tret = poll(&fds, 1, timeoutMs);\n> > > > > +\t\tif (ret == 0)\n> > > > > +\t\t\treturn -ETIME;\n> > > > > +\n> > > > > +\t\tif (ret > 0) {\n> > > > > +\t\t\tif (fds.revents & (POLLERR | POLLNVAL))\n> > > > > +\t\t\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\t\t\treturn 0;\n> > > > > +\t\t}\n> > > > > +\t} while (ret == -1 && (errno == EINTR || errno == EAGAIN));\n> > > > > +\n> > > > > +\treturn ret;\n> > > > > +}\n> > > > > +\n> > > > > +void CameraWorker::Worker::processRequest(CaptureRequest *request)\n> > > > > +{\n> > > > > +\tfor (int fence : request->acquireFences_) {\n> > > > > +\t\tif (fence == -1)\n> > > > > +\t\t\tcontinue;\n> > > > > +\n> > > > > +\t\tint ret = waitFence(fence);\n> > > >\n> > > > It would be possible to optimize this by waiting on all the fences at\n> > > > the same time to minimize syscalls when multiple fences are ready at the\n> > > > same time, but that may not be worth it.\n> > >\n> > > I considered several options here, including using a custom event\n> > > notifier that accepts a set of fds instead of a single one to exploit\n> > > our notifier/dispatcher infrastructure. Unfortunately, that would\n> > > require manualy tracking of the number of completed fences here, or\n> > > the implementation of a custom dispatcher that works on a set of fds\n> > > and only signal completion when all of them are done.\n> > >\n> > > At the same time I considered instrumenting poll to work with multiple\n> > > fds instead of going through a syscall for each one of them. In this\n> > > case too, manual tracking of the number of completed requests would be\n> > > required, and once I considered that we deal with up to 3 buffers, and\n> > > most of the time with a single one, I considered writing such an\n> > > error-prone and quite-harsh to debug code not worth it.\n> >\n> > Ack.\n> >\n> > > > > +\t\tif (ret < 0)\n> > > > > +\t\t\treturn;\n> > > > > +\n> > > > > +\t\tclose(fence);\n> > > > > +\t}\n> > > > > +\n> > > > > +\tcamera_->queueRequest(request->request_);\n> > > > > +\tdelete request;\n> > > > > +}\n> > > > > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> > > > > new file mode 100644\n> > > > > index 000000000000..24b176d6269c\n> > > > > --- /dev/null\n> > > > > +++ b/src/android/camera_worker.h\n> > > > > @@ -0,0 +1,75 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2020, Google Inc.\n> > > > > + *\n> > > > > + * camera_worker.h - Process capture request on behalf of the Camera HAL\n> > > > > + */\n> > > > > +#ifndef __ANDROID_CAMERA_WORKER_H__\n> > > > > +#define __ANDROID_CAMERA_WORKER_H__\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \"libcamera/internal/thread.h\"\n> > > > > +\n> > > > > +class CameraDevice;\n> > > > > +\n> > > > > +struct CaptureRequest {\n> > > > > +\tCaptureRequest(libcamera::Request *request)\n> > > > > +\t\t: request_(request)\n> > > > > +\t{\n> > > > > +\t}\n> > > > > +\n> > > > > +\tvoid addBuffer(libcamera::Stream *stream,\n> > > > > +\t\t libcamera::FrameBuffer *buffer, int fence)\n> > > > > +\t{\n> > > > > +\t\trequest_->addBuffer(stream, buffer);\n> > > > > +\t\tacquireFences_.push_back(fence);\n> > > > > +\t}\n> > > >\n> > > > The API isn't amazing, but that's because the best solution will be to\n> > > > add fences support to the FrameBuffer class. Until we do so, this\n> > > > workaround is fine.\n> > >\n> > > To be honest I don't mind the intruduction of CaptureRequest in the\n> > > CameraDevice, the rest I agree, it's awful.\n> >\n> > It's not awful :-) It's just sub-optimal, and that's fine. I think\n> > you've struct the right balance, the implementation does the job, it's\n> > clean and lean, not over-designed, and probably as good as it can get\n> > until this gets moved to the libcamera core.\n> >\n> > > A proper library-wide solution is needed, but this closes a gap and\n> > > does not introduce any techincal debt\n> > >\n> > > So I do as well think that:\n> > >\n> > > > Overall I think this is totally fine for now.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 29DA8BEEE0\n\tfor ;\n\tFri, 9 Oct 2020 22:52:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADEB760725;\n\tSat, 10 Oct 2020 00:52:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C59B760359\n\tfor ;\n\tSat, 10 Oct 2020 00:52:18 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 36549528;\n\tSat, 10 Oct 2020 00:52:18 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VRGh2h1C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602283938;\n\tbh=HVT0YsCUcRrhCCNBxes+K5QQW7SWH4NijHbn6kWZFzA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VRGh2h1CqT0NqafQSpTIiqEVrqZvvs/iJuWMjJLp3Txoiev7862QmMACA8s5ldHfd\n\t3yuHQsw2kxOy3saKrpaYgeo+aI1yRKCnoiHld0uGKyCMxhKGv8aJTmkIv4KawXc+eJ\n\tAT4NClUMD3OBU2U4EH7PmFco5jY4GveJKk4TdVGM=","Date":"Sat, 10 Oct 2020 01:51:34 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201009225134.GL25040@pendragon.ideasonboard.com>","References":"<20201006160637.29841-1-jacopo@jmondi.org>\n\t<20201006160637.29841-2-jacopo@jmondi.org>\n\t<20201008040522.GV3937@pendragon.ideasonboard.com>\n\t<20201008083624.gzpxyr3axlryefdg@uno.localdomain>\n\t<20201008203326.GG3939@pendragon.ideasonboard.com>\n\t<20201009093159.5eqsin2szbjhzeqf@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201009093159.5eqsin2szbjhzeqf@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC 1/2] android: camera_worker: Introduce\n\tCameraWorker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]