[{"id":12424,"web_url":"https://patchwork.libcamera.org/comment/12424/","msgid":"<20200910113505.GT4095624@oden.dyn.berto.se>","date":"2020-09-10T11:35:05","subject":"Re: [libcamera-devel] [PATCH 7/8] android: camera_device: Use\n\tlibcamera buffer pool","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-09-09 17:54:56 +0200, Jacopo Mondi wrote:\n> Now that we have reserved and made available to the camera HAL a\n> pool of libcamera allocated buffers, use them when a CameraStream\n> instance that requires internal allocation is processed.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------\n> 1 file changed, 34 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index a7cb1be03b9a..f94b313581e7 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1422,6 +1422,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \tfor (unsigned int i = 0; i < descriptor->numBuffers; ++i) {\n> \t\tCameraStream *cameraStream =\n> \t\t\tstatic_cast(camera3Buffers[i].stream->priv);\n> +\t\tconst StreamConfiguration &config = config_->at(cameraStream->index());\n> +\t\tStream *stream = config.stream();\n> \n> \t\t/*\n> \t\t * Keep track of which stream the request belongs to and store\n> @@ -1430,27 +1432,39 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\tdescriptor->buffers[i].stream = camera3Buffers[i].stream;\n> \t\tdescriptor->buffers[i].buffer = camera3Buffers[i].buffer;\n> \n> -\t\t/* Software streams are handled after hardware streams complete. */\n> -\t\tif (cameraStream->format() == formats::MJPEG)\n> +\t\t/* Mapped streams don't need to be added to the Request. */\n> +\t\tif (cameraStream->type() == CameraStream::Type::Mapped)\n> \t\t\tcontinue;\n> \n> -\t\t/*\n> -\t\t * Create a libcamera buffer using the dmabuf descriptors of\n> -\t\t * the camera3Buffer for each stream. The FrameBuffer is\n> -\t\t * directly associated with the Camera3RequestDescriptor for\n> -\t\t * lifetime management only.\n> -\t\t */\n> -\t\tFrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);\n> +\t\tFrameBuffer *buffer;\n> +\t\tif (cameraStream->type() == CameraStream::Type::Direct) {\n> +\t\t\t/*\n> +\t\t\t * Create a libcamera buffer using the dmabuf\n> +\t\t\t * descriptors of the camera3Buffer for each stream and\n> +\t\t\t * associate it with the Camera3RequestDescriptor for\n> +\t\t\t * lifetime management only.\n> +\t\t\t */\n> +\t\t\tbuffer = createFrameBuffer(*camera3Buffers[i].buffer);\n> +\t\t\tdescriptor->frameBuffers.emplace_back(buffer);\n> +\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * Get the frame buffer from the CameraStream internal\n> +\t\t\t * buffer pool. The lifetime management of internal\n> +\t\t\t * buffers is connected to the one of the\n> +\t\t\t * FrameBufferAllocator instance.\n> +\t\t\t *\n> +\t\t\t * The retrieved buffer has to be returned to the\n> +\t\t\t * allocator once it has been processed.\n> +\t\t\t */\n> +\t\t\tbuffer = allocator_.getBuffer(stream);\n\nShould we add a LOG(Error) if there are no free buffers to retrieve form \nthe allocator?\n\n> +\t\t}\n> \t\tif (!buffer) {\n> \t\t\tLOG(HAL, Error) << \"Failed to create buffer\";\n> \t\t\tdelete request;\n> \t\t\tdelete descriptor;\n> \t\t\treturn -ENOMEM;\n> \t\t}\n> -\t\tdescriptor->frameBuffers.emplace_back(buffer);\n> -\n> -\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n> -\t\tStream *stream = streamConfiguration->stream();\n> \n> \t\trequest->addBuffer(stream, buffer);\n> \t}\n> @@ -1561,6 +1575,13 @@ void CameraDevice::requestComplete(Request *request)\n> \t\tconst uint32_t jpeg_orientation = 0;\n> \t\tresultMetadata->addEntry(ANDROID_JPEG_ORIENTATION,\n> \t\t\t\t\t &jpeg_orientation, 1);\n> +\n> +\t\t/*\n> +\t\t * Return the FrameBuffer to the CameraStream now that we're\n> +\t\t * done processing it.\n> +\t\t */\n> +\t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> +\t\t\tallocator_.returnBuffer(stream, buffer);\n\nSeeing how the getBuffer() and returnBuffer() is used here could it not \nbe replaced with a std::queue in a similar fashion we have in \npipeline handlers?\n\n> \t}\n> \n> \t/* Prepare to call back the Android camera stack. */\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 8ABFCC3B5B\n\tfor ;\n\tThu, 10 Sep 2020 11:35:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1903762D86;\n\tThu, 10 Sep 2020 13:35:09 +0200 (CEST)","from mail-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3854962D60\n\tfor ;\n\tThu, 10 Sep 2020 13:35:07 +0200 (CEST)","by mail-lf1-x144.google.com with SMTP id w11so3408637lfn.2\n\tfor ;\n\tThu, 10 Sep 2020 04:35:07 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tr7sm1284389lfn.84.2020.09.10.04.35.06\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 10 Sep 2020 04:35:06 -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=\"bPMPloNo\"; 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=fYm7jFhudOEbhYlIFeDMXZPyPNn2CAUb1FCQ3UAjMwk=;\n\tb=bPMPloNoxJVB0EKbyhKyzR1xyvRmjOvrIbY8sb8qJncrMnLP9O436Nqbfv3anTGcsu\n\tAnzw9XE9c5kp8t695JkKaQ+MuZMY0zd0NyMFTHNSHxSc0kJ7ZS2ANuoJ8kBvAlp5fzvj\n\tem3LoszdBsFPVdhJXY2CF1jIl2Xm0Xw1iIJtjVu6xmIUmP2ksOBp7jdcRhg01yCfBwDr\n\t44oUFYYrt3hBbER8dUZzv7NFNuRZx1pifqD5jNBMhGUTFHlLW4fHNASCG7Xp7/eZsRBk\n\t7uZbVE9HEwBgRc8FLTY2GVQXDgOTGdymJFRcATVzqVTHj+sI/zFp7Ow1A86nJOe0zZKC\n\tAK0Q==","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=fYm7jFhudOEbhYlIFeDMXZPyPNn2CAUb1FCQ3UAjMwk=;\n\tb=dlA4vRDSQgF8vA+QpjBVvz1ZMy4VXfGoBvj4dAyWBmMEQucw5YbdjD7wyJlG3vVun5\n\thYohn+HuSvI5WcN5GIX9BPtmlTKxKMhkevvnBjzmq2EElGe3UYJQk/C85LAX6LPO7P/6\n\tvwYEdjq7l+O0BtxY5sCKSBru5dR/RmsP/cglfuQJOzWlwChw7/eUmMPSR8GH05k5csse\n\t3F0PQt3FfPQw7k6bmMub+Mx02iB3I6a7+hdFgb7xiOCPm2jpOfzMWkC2gpr+oByWtxYI\n\t2103pqfHDNbxA7FAcihn1Z4Gk326KFuNAxudIMRq6BKHBFPEQ/+3hrUcIUr1GP5r0Sx6\n\tGz2w==","X-Gm-Message-State":"AOAM531q3xnUEgi13RSOZHomFnGENSccSugVUd31bEajudZtDMXrfJqs\n\t1N4Pw3XcfnwwK8alrLbqJ2q3l20bwmddDg==","X-Google-Smtp-Source":"ABdhPJyaceRTzr95zG6XqJW4qEr7uHiQfsS9B7X8eNQ/8KTsn7L67CruIgG+MgYG0kazZp8tqeRXWA==","X-Received":"by 2002:a19:8906:: with SMTP id l6mr1813103lfd.136.1599737706638;\n\tThu, 10 Sep 2020 04:35:06 -0700 (PDT)","Date":"Thu, 10 Sep 2020 13:35:05 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200910113505.GT4095624@oden.dyn.berto.se>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200909155457.153907-8-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 7/8] android: camera_device: Use\n\tlibcamera buffer pool","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":"hanlinchen@chromium.org, 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":12427,"web_url":"https://patchwork.libcamera.org/comment/12427/","msgid":"<20200910122300.6udykfxg3xtizcl2@uno.localdomain>","date":"2020-09-10T12:23:00","subject":"Re: [libcamera-devel] [PATCH 7/8] android: camera_device: Use\n\tlibcamera buffer pool","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Sep 10, 2020 at 01:35:05PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2020-09-09 17:54:56 +0200, Jacopo Mondi wrote:\n> > Now that we have reserved and made available to the camera HAL a\n> > pool of libcamera allocated buffers, use them when a CameraStream\n> > instance that requires internal allocation is processed.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------\n> > 1 file changed, 34 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index a7cb1be03b9a..f94b313581e7 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1422,6 +1422,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \tfor (unsigned int i = 0; i < descriptor->numBuffers; ++i) {\n> > \t\tCameraStream *cameraStream =\n> > \t\t\tstatic_cast(camera3Buffers[i].stream->priv);\n> > +\t\tconst StreamConfiguration &config = config_->at(cameraStream->index());\n> > +\t\tStream *stream = config.stream();\n> >\n> > \t\t/*\n> > \t\t * Keep track of which stream the request belongs to and store\n> > @@ -1430,27 +1432,39 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\tdescriptor->buffers[i].stream = camera3Buffers[i].stream;\n> > \t\tdescriptor->buffers[i].buffer = camera3Buffers[i].buffer;\n> >\n> > -\t\t/* Software streams are handled after hardware streams complete. */\n> > -\t\tif (cameraStream->format() == formats::MJPEG)\n> > +\t\t/* Mapped streams don't need to be added to the Request. */\n> > +\t\tif (cameraStream->type() == CameraStream::Type::Mapped)\n> > \t\t\tcontinue;\n> >\n> > -\t\t/*\n> > -\t\t * Create a libcamera buffer using the dmabuf descriptors of\n> > -\t\t * the camera3Buffer for each stream. The FrameBuffer is\n> > -\t\t * directly associated with the Camera3RequestDescriptor for\n> > -\t\t * lifetime management only.\n> > -\t\t */\n> > -\t\tFrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);\n> > +\t\tFrameBuffer *buffer;\n> > +\t\tif (cameraStream->type() == CameraStream::Type::Direct) {\n> > +\t\t\t/*\n> > +\t\t\t * Create a libcamera buffer using the dmabuf\n> > +\t\t\t * descriptors of the camera3Buffer for each stream and\n> > +\t\t\t * associate it with the Camera3RequestDescriptor for\n> > +\t\t\t * lifetime management only.\n> > +\t\t\t */\n> > +\t\t\tbuffer = createFrameBuffer(*camera3Buffers[i].buffer);\n> > +\t\t\tdescriptor->frameBuffers.emplace_back(buffer);\n> > +\n> > +\t\t} else {\n> > +\t\t\t/*\n> > +\t\t\t * Get the frame buffer from the CameraStream internal\n> > +\t\t\t * buffer pool. The lifetime management of internal\n> > +\t\t\t * buffers is connected to the one of the\n> > +\t\t\t * FrameBufferAllocator instance.\n> > +\t\t\t *\n> > +\t\t\t * The retrieved buffer has to be returned to the\n> > +\t\t\t * allocator once it has been processed.\n> > +\t\t\t */\n> > +\t\t\tbuffer = allocator_.getBuffer(stream);\n>\n> Should we add a LOG(Error) if there are no free buffers to retrieve form\n> the allocator?\n>\n> > +\t\t}\n> > \t\tif (!buffer) {\n\nIt's catched here\n\n> > \t\t\tLOG(HAL, Error) << \"Failed to create buffer\";\n> > \t\t\tdelete request;\n> > \t\t\tdelete descriptor;\n> > \t\t\treturn -ENOMEM;\n> > \t\t}\n> > -\t\tdescriptor->frameBuffers.emplace_back(buffer);\n> > -\n> > -\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n> > -\t\tStream *stream = streamConfiguration->stream();\n> >\n> > \t\trequest->addBuffer(stream, buffer);\n> > \t}\n> > @@ -1561,6 +1575,13 @@ void CameraDevice::requestComplete(Request *request)\n> > \t\tconst uint32_t jpeg_orientation = 0;\n> > \t\tresultMetadata->addEntry(ANDROID_JPEG_ORIENTATION,\n> > \t\t\t\t\t &jpeg_orientation, 1);\n> > +\n> > +\t\t/*\n> > +\t\t * Return the FrameBuffer to the CameraStream now that we're\n> > +\t\t * done processing it.\n> > +\t\t */\n> > +\t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> > +\t\t\tallocator_.returnBuffer(stream, buffer);\n>\n> Seeing how the getBuffer() and returnBuffer() is used here could it not\n> be replaced with a std::queue in a similar fashion we have in\n> pipeline handlers?\n>\n\nThat's what I've done initially, but that's basically repeting the\nsame pattern in several parts of libcamera.\n\nWhen I saw\n\nvoid CIO2Device::tryReturnBuffer(FrameBuffer *buffer)\n{\n\t/*\n\t * \\todo Once more pipelines deal with buffers that may be allocated\n\t * internally or externally this pattern might become a common need. At\n\t * that point this check should be moved to something clever in\n\t * FrameBuffer.\n\t */\n\tfor (const std::unique_ptr &buf : buffers_) {\n\t\tif (buf.get() == buffer) {\n\t\t\tavailableBuffers_.push(buffer);\n\t\t\tbreak;\n\t\t}\n\t}\n}\n\nI'm not saying using an std::queue<> is \"clever\" as it's surely\nefficient in insertion/removal but occupies quite some memory, but I\nconsidered this worth to be brought to be to be part of the\nFrameBufferAllocator interface.\n\nMy intention was to replace the custom implementations in the\npipelines by using the newly defined getBuffer()/returnBuffer()\nmethods calls.\n\n\n> > \t}\n> >\n> > \t/* Prepare to call back the Android camera stack. */\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 186F2BDB1D\n\tfor ;\n\tThu, 10 Sep 2020 12:19:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BDAD62D60;\n\tThu, 10 Sep 2020 14:19:14 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5218C62CE1\n\tfor ;\n\tThu, 10 Sep 2020 14:19:13 +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 relay5-d.mail.gandi.net (Postfix) with ESMTPSA id A74D01C0014;\n\tThu, 10 Sep 2020 12:19:12 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 10 Sep 2020 14:23:00 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200910122300.6udykfxg3xtizcl2@uno.localdomain>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-8-jacopo@jmondi.org>\n\t<20200910113505.GT4095624@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200910113505.GT4095624@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 7/8] android: camera_device: Use\n\tlibcamera buffer pool","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":"hanlinchen@chromium.org, 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":12448,"web_url":"https://patchwork.libcamera.org/comment/12448/","msgid":"","date":"2020-09-11T08:46:06","subject":"Re: [libcamera-devel] [PATCH 7/8] android: camera_device: Use\n\tlibcamera buffer pool","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Thu, Sep 10, 2020 at 9:19 PM Jacopo Mondi wrote:\n>\n> Hi Niklas,\n>\n> On Thu, Sep 10, 2020 at 01:35:05PM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2020-09-09 17:54:56 +0200, Jacopo Mondi wrote:\n> > > Now that we have reserved and made available to the camera HAL a\n> > > pool of libcamera allocated buffers, use them when a CameraStream\n> > > instance that requires internal allocation is processed.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------\n> > > 1 file changed, 34 insertions(+), 13 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index a7cb1be03b9a..f94b313581e7 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -1422,6 +1422,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {\n> > > CameraStream *cameraStream =\n> > > static_cast(camera3Buffers[i].stream->priv);\n> > > + const StreamConfiguration &config = config_->at(cameraStream->index());\n> > > + Stream *stream = config.stream();\n> > >\n> > > /*\n> > > * Keep track of which stream the request belongs to and store\n> > > @@ -1430,27 +1432,39 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > descriptor->buffers[i].stream = camera3Buffers[i].stream;\n> > > descriptor->buffers[i].buffer = camera3Buffers[i].buffer;\n> > >\n> > > - /* Software streams are handled after hardware streams complete. */\n> > > - if (cameraStream->format() == formats::MJPEG)\n> > > + /* Mapped streams don't need to be added to the Request. */\n> > > + if (cameraStream->type() == CameraStream::Type::Mapped)\n> > > continue;\n> > >\n> > > - /*\n> > > - * Create a libcamera buffer using the dmabuf descriptors of\n> > > - * the camera3Buffer for each stream. The FrameBuffer is\n> > > - * directly associated with the Camera3RequestDescriptor for\n> > > - * lifetime management only.\n> > > - */\n> > > - FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);\n> > > + FrameBuffer *buffer;\n> > > + if (cameraStream->type() == CameraStream::Type::Direct) {\n> > > + /*\n> > > + * Create a libcamera buffer using the dmabuf\n> > > + * descriptors of the camera3Buffer for each stream and\n> > > + * associate it with the Camera3RequestDescriptor for\n> > > + * lifetime management only.\n> > > + */\n> > > + buffer = createFrameBuffer(*camera3Buffers[i].buffer);\n> > > + descriptor->frameBuffers.emplace_back(buffer);\n> > > +\n> > > + } else {\n> > > + /*\n> > > + * Get the frame buffer from the CameraStream internal\n> > > + * buffer pool. The lifetime management of internal\n> > > + * buffers is connected to the one of the\n> > > + * FrameBufferAllocator instance.\n> > > + *\n> > > + * The retrieved buffer has to be returned to the\n> > > + * allocator once it has been processed.\n> > > + */\n> > > + buffer = allocator_.getBuffer(stream);\n> >\n> > Should we add a LOG(Error) if there are no free buffers to retrieve form\n> > the allocator?\n> >\n> > > + }\n> > > if (!buffer) {\n>\n> It's catched here\n>\n> > > LOG(HAL, Error) << \"Failed to create buffer\";\n> > > delete request;\n> > > delete descriptor;\n\nAlthough this is not so related to your change, shall we be able to\nuse std::unique_ptr or std::shared_ptr more here to specify the\nownerships?\nIMO, camera_device.cpp seems to have a lot of raw pointers, which\nmakes readers to track their ownerships.\nFor example, descriptor->frameBuffers.emplace_back(buffe) takes the\nownership by constructing std::unique_ptr.\nIt is not figured out unless seeing frameBuffers declaration.\n|buffer| should be std::unique_ptr from scratch by letting\ncreateFrameBuffer() return std::unique_ptr.\nI don't know how feasible it is to make request and descriptor be\nstd::unique_ptr/shared_ptr because it is necessary to change libcamera\ninterface, but I prefer making it so much.\n\nReviewed-by: Hirokazu Honda \n\n> > > return -ENOMEM;\n> > > }\n> > > - descriptor->frameBuffers.emplace_back(buffer);\n> > > -\n> > > - StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n> > > - Stream *stream = streamConfiguration->stream();\n> > >\n> > > request->addBuffer(stream, buffer);\n> > > }\n> > > @@ -1561,6 +1575,13 @@ void CameraDevice::requestComplete(Request *request)\n> > > const uint32_t jpeg_orientation = 0;\n> > > resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION,\n> > > &jpeg_orientation, 1);\n> > > +\n> > > + /*\n> > > + * Return the FrameBuffer to the CameraStream now that we're\n> > > + * done processing it.\n> > > + */\n> > > + if (cameraStream->type() == CameraStream::Type::Internal)\n> > > + allocator_.returnBuffer(stream, buffer);\n> >\n> > Seeing how the getBuffer() and returnBuffer() is used here could it not\n> > be replaced with a std::queue in a similar fashion we have in\n> > pipeline handlers?\n> >\n>\n> That's what I've done initially, but that's basically repeting the\n> same pattern in several parts of libcamera.\n>\n> When I saw\n>\n> void CIO2Device::tryReturnBuffer(FrameBuffer *buffer)\n> {\n> /*\n> * \\todo Once more pipelines deal with buffers that may be allocated\n> * internally or externally this pattern might become a common need. At\n> * that point this check should be moved to something clever in\n> * FrameBuffer.\n> */\n> for (const std::unique_ptr &buf : buffers_) {\n> if (buf.get() == buffer) {\n> availableBuffers_.push(buffer);\n> break;\n> }\n> }\n> }\n>\n> I'm not saying using an std::queue<> is \"clever\" as it's surely\n> efficient in insertion/removal but occupies quite some memory, but I\n> considered this worth to be brought to be to be part of the\n> FrameBufferAllocator interface.\n>\n> My intention was to replace the custom implementations in the\n> pipelines by using the newly defined getBuffer()/returnBuffer()\n> methods calls.\n>\n>\n> > > }\n> > >\n> > > /* Prepare to call back the Android camera stack. */\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> 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 CCC66C3B5B\n\tfor ;\n\tFri, 11 Sep 2020 08:46:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D96A62D38;\n\tFri, 11 Sep 2020 10:46:19 +0200 (CEST)","from mail-ed1-x543.google.com (mail-ed1-x543.google.com\n\t[IPv6:2a00:1450:4864:20::543])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE23B62901\n\tfor ;\n\tFri, 11 Sep 2020 10:46:17 +0200 (CEST)","by mail-ed1-x543.google.com with SMTP id ay8so9129632edb.8\n\tfor ;\n\tFri, 11 Sep 2020 01:46:17 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"XjRYzS8Z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=qYVPd1ocgnQgpmRnLVHx5+SPZq8VufmbahkSDc5eliY=;\n\tb=XjRYzS8ZdieSAbj8AmaUtxbXKwQxZtkIoTDZNaS31T6RZA0CM25YlVU/3skDgkb6ul\n\txY0W7MaWDV3Pa3O5kGUNZhcwnLJQZyQPMla+tzrGmAaQ4dl5YQPZ0EPROkYWLJ6tIoGU\n\tRxs26ByALCWqxlSMAGsJ+5cQJ3vTttMHO0DLQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=qYVPd1ocgnQgpmRnLVHx5+SPZq8VufmbahkSDc5eliY=;\n\tb=J27xxqBjd/P9Jk3DwP6iava5xfEUsT+Sx9QrnSBvsa6SkvYpxmHCrvGEKmEizdpjxp\n\t1SvvbSdmOIECIVf0geczZ8RtTAslFJtIqIMPTDvcDUuzIf/m2+LRhaZeOYhg9urWQcfn\n\tBAh34rVts5i3ysRMylyykO34iJkwBNTpTbLmsnn3rRgHlF9vedVGgEwwpYE+vQ1WMRDR\n\tBYb4wtn3ltpC1cbkbgesBphQTErBpStbpS+YQVO2A1BGBzqaS+iTqDWA/xJGmPTZg6oa\n\tDmed5scwtv9KdTYAWPk3Z2lqP8d2v/1BLKGc76/S/bYjVeh94CkrsVOCMlFekh+tVW2W\n\tmsMg==","X-Gm-Message-State":"AOAM532rRtfx6gXe3t9k90LoCi1w8b7H5FLPZeLV7WFNGXuGd5zqQmIi\n\tfOhPVhqDDdN1xV2I5vMfCAJ/0T1WiREJ0085M2MRLA==","X-Google-Smtp-Source":"ABdhPJzmwTT0O6ZojNaqC3Yf07r4DrdH16iBuqaQSRIYB6Ka0SPwTjmpW6kwHaG1PfiBMQlqYi74yHvTsxaJFFOE+HA=","X-Received":"by 2002:a50:e44b:: with SMTP id e11mr910065edm.73.1599813977474; \n\tFri, 11 Sep 2020 01:46:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-8-jacopo@jmondi.org>\n\t<20200910113505.GT4095624@oden.dyn.berto.se>\n\t<20200910122300.6udykfxg3xtizcl2@uno.localdomain>","In-Reply-To":"<20200910122300.6udykfxg3xtizcl2@uno.localdomain>","From":"Hirokazu Honda ","Date":"Fri, 11 Sep 2020 17:46:06 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 7/8] android: camera_device: Use\n\tlibcamera buffer pool","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":"Hanlin Chen ,\n\tlibcamera-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":12455,"web_url":"https://patchwork.libcamera.org/comment/12455/","msgid":"<20200911162800.wxr3cya4i54cljfh@uno.localdomain>","date":"2020-09-11T16:28:00","subject":"Re: [libcamera-devel] [PATCH 7/8] android: camera_device: Use\n\tlibcamera buffer pool","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Fri, Sep 11, 2020 at 05:46:06PM +0900, Hirokazu Honda wrote:\n> On Thu, Sep 10, 2020 at 9:19 PM Jacopo Mondi wrote:\n> >\n> > Hi Niklas,\n> >\n> > On Thu, Sep 10, 2020 at 01:35:05PM +0200, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2020-09-09 17:54:56 +0200, Jacopo Mondi wrote:\n> > > > Now that we have reserved and made available to the camera HAL a\n> > > > pool of libcamera allocated buffers, use them when a CameraStream\n> > > > instance that requires internal allocation is processed.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------\n> > > > 1 file changed, 34 insertions(+), 13 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index a7cb1be03b9a..f94b313581e7 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -1422,6 +1422,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {\n> > > > CameraStream *cameraStream =\n> > > > static_cast(camera3Buffers[i].stream->priv);\n> > > > + const StreamConfiguration &config = config_->at(cameraStream->index());\n> > > > + Stream *stream = config.stream();\n> > > >\n> > > > /*\n> > > > * Keep track of which stream the request belongs to and store\n> > > > @@ -1430,27 +1432,39 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > descriptor->buffers[i].stream = camera3Buffers[i].stream;\n> > > > descriptor->buffers[i].buffer = camera3Buffers[i].buffer;\n> > > >\n> > > > - /* Software streams are handled after hardware streams complete. */\n> > > > - if (cameraStream->format() == formats::MJPEG)\n> > > > + /* Mapped streams don't need to be added to the Request. */\n> > > > + if (cameraStream->type() == CameraStream::Type::Mapped)\n> > > > continue;\n> > > >\n> > > > - /*\n> > > > - * Create a libcamera buffer using the dmabuf descriptors of\n> > > > - * the camera3Buffer for each stream. The FrameBuffer is\n> > > > - * directly associated with the Camera3RequestDescriptor for\n> > > > - * lifetime management only.\n> > > > - */\n> > > > - FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);\n> > > > + FrameBuffer *buffer;\n> > > > + if (cameraStream->type() == CameraStream::Type::Direct) {\n> > > > + /*\n> > > > + * Create a libcamera buffer using the dmabuf\n> > > > + * descriptors of the camera3Buffer for each stream and\n> > > > + * associate it with the Camera3RequestDescriptor for\n> > > > + * lifetime management only.\n> > > > + */\n> > > > + buffer = createFrameBuffer(*camera3Buffers[i].buffer);\n> > > > + descriptor->frameBuffers.emplace_back(buffer);\n> > > > +\n> > > > + } else {\n> > > > + /*\n> > > > + * Get the frame buffer from the CameraStream internal\n> > > > + * buffer pool. The lifetime management of internal\n> > > > + * buffers is connected to the one of the\n> > > > + * FrameBufferAllocator instance.\n> > > > + *\n> > > > + * The retrieved buffer has to be returned to the\n> > > > + * allocator once it has been processed.\n> > > > + */\n> > > > + buffer = allocator_.getBuffer(stream);\n> > >\n> > > Should we add a LOG(Error) if there are no free buffers to retrieve form\n> > > the allocator?\n> > >\n> > > > + }\n> > > > if (!buffer) {\n> >\n> > It's catched here\n> >\n> > > > LOG(HAL, Error) << \"Failed to create buffer\";\n> > > > delete request;\n> > > > delete descriptor;\n>\n> Although this is not so related to your change, shall we be able to\n> use std::unique_ptr or std::shared_ptr more here to specify the\n> ownerships?\n> IMO, camera_device.cpp seems to have a lot of raw pointers, which\n> makes readers to track their ownerships.\n> For example, descriptor->frameBuffers.emplace_back(buffe) takes the\n> ownership by constructing std::unique_ptr.\n> It is not figured out unless seeing frameBuffers declaration.\n\nI sadly know, having chased for a day a bug I've introduced by not\nvery well understanding that.\n\n> |buffer| should be std::unique_ptr from scratch by letting\n> createFrameBuffer() return std::unique_ptr.\n> I don't know how feasible it is to make request and descriptor be\n> std::unique_ptr/shared_ptr because it is necessary to change libcamera\n> interface, but I prefer making it so much.\n\nWe could indeed do better, and here we have quite some space for\nimprovements without changing the libcamera API.\n\nI'll see what I can do for next version without being too invasive.\n\n>\n> Reviewed-by: Hirokazu Honda \n>\n> > > > return -ENOMEM;\n> > > > }\n> > > > - descriptor->frameBuffers.emplace_back(buffer);\n> > > > -\n> > > > - StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n> > > > - Stream *stream = streamConfiguration->stream();\n> > > >\n> > > > request->addBuffer(stream, buffer);\n> > > > }\n> > > > @@ -1561,6 +1575,13 @@ void CameraDevice::requestComplete(Request *request)\n> > > > const uint32_t jpeg_orientation = 0;\n> > > > resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION,\n> > > > &jpeg_orientation, 1);\n> > > > +\n> > > > + /*\n> > > > + * Return the FrameBuffer to the CameraStream now that we're\n> > > > + * done processing it.\n> > > > + */\n> > > > + if (cameraStream->type() == CameraStream::Type::Internal)\n> > > > + allocator_.returnBuffer(stream, buffer);\n> > >\n> > > Seeing how the getBuffer() and returnBuffer() is used here could it not\n> > > be replaced with a std::queue in a similar fashion we have in\n> > > pipeline handlers?\n> > >\n> >\n> > That's what I've done initially, but that's basically repeting the\n> > same pattern in several parts of libcamera.\n> >\n> > When I saw\n> >\n> > void CIO2Device::tryReturnBuffer(FrameBuffer *buffer)\n> > {\n> > /*\n> > * \\todo Once more pipelines deal with buffers that may be allocated\n> > * internally or externally this pattern might become a common need. At\n> > * that point this check should be moved to something clever in\n> > * FrameBuffer.\n> > */\n> > for (const std::unique_ptr &buf : buffers_) {\n> > if (buf.get() == buffer) {\n> > availableBuffers_.push(buffer);\n> > break;\n> > }\n> > }\n> > }\n> >\n> > I'm not saying using an std::queue<> is \"clever\" as it's surely\n> > efficient in insertion/removal but occupies quite some memory, but I\n> > considered this worth to be brought to be to be part of the\n> > FrameBufferAllocator interface.\n> >\n> > My intention was to replace the custom implementations in the\n> > pipelines by using the newly defined getBuffer()/returnBuffer()\n> > methods calls.\n> >\n> >\n> > > > }\n> > > >\n> > > > /* Prepare to call back the Android camera stack. */\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> > 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 526F5BDB1D\n\tfor ;\n\tFri, 11 Sep 2020 16:24:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B4AFD62D86;\n\tFri, 11 Sep 2020 18:24:13 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3DBFA60534\n\tfor ;\n\tFri, 11 Sep 2020 18:24:12 +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 relay12.mail.gandi.net (Postfix) with ESMTPSA id 40DAC20000A;\n\tFri, 11 Sep 2020 16:24:10 +0000 (UTC)"],"Date":"Fri, 11 Sep 2020 18:28:00 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20200911162800.wxr3cya4i54cljfh@uno.localdomain>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-8-jacopo@jmondi.org>\n\t<20200910113505.GT4095624@oden.dyn.berto.se>\n\t<20200910122300.6udykfxg3xtizcl2@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 7/8] android: camera_device: Use\n\tlibcamera buffer pool","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":"Hanlin Chen ,\n\tlibcamera-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\" "}}]