Patch Detail
Show a patch.
GET /api/1.1/patches/9458/?format=api
{ "id": 9458, "url": "https://patchwork.libcamera.org/api/1.1/patches/9458/?format=api", "web_url": "https://patchwork.libcamera.org/patch/9458/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20200902130846.55910-6-jacopo@jmondi.org>", "date": "2020-09-02T13:08:44", "name": "[libcamera-devel,RFC,5/7] android: camera_device: Rework CameraStream handling", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "c1a21c94af5b11ec64a4ed72422f082a999fc56e", "submitter": { "id": 3, "url": "https://patchwork.libcamera.org/api/1.1/people/3/?format=api", "name": "Jacopo Mondi", "email": "jacopo@jmondi.org" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/9458/mbox/", "series": [ { "id": 1258, "url": "https://patchwork.libcamera.org/api/1.1/series/1258/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=1258", "date": "2020-09-02T13:08:39", "name": "android: camera_device: Turn CameraStream into a class", "version": 1, "mbox": "https://patchwork.libcamera.org/series/1258/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/9458/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/9458/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "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 BF92EBF019\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 2 Sep 2020 13:05:17 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D2A862984;\n\tWed, 2 Sep 2020 15:05:17 +0200 (CEST)", "from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E402260374\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 2 Sep 2020 15:05:16 +0200 (CEST)", "from uno.lan (93-34-118-233.ip49.fastwebnet.it [93.34.118.233])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 8FEC210000A;\n\tWed, 2 Sep 2020 13:05:15 +0000 (UTC)" ], "From": "Jacopo Mondi <jacopo@jmondi.org>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Wed, 2 Sep 2020 15:08:44 +0200", "Message-Id": "<20200902130846.55910-6-jacopo@jmondi.org>", "X-Mailer": "git-send-email 2.28.0", "In-Reply-To": "<20200902130846.55910-1-jacopo@jmondi.org>", "References": "<20200902130846.55910-1-jacopo@jmondi.org>", "MIME-Version": "1.0", "Subject": "[libcamera-devel] [RFC 5/7] android: camera_device: Rework\n\tCameraStream handling", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "Cc": "tfiga@google.com, hiroh@google.com", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "7bit", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "The CameraDevice::streams_ vector of CameraStream instances is\ncurrently mostly accessed by index. The current implementation\ncreates all the CameraStream during the first loop that inspects the\ncamera3_stream instances, and the update the index of the\nStreamConfiguration associated with the CameraStream during a second\nloop that inspects MJPEG streams. A third loop creates the JPEG encoder\nassociated with CameraStreams that produce MJPEG format.\n\nAs the index-based association is hard to follow and rather fragile,\nrework the creation and handling of CameraStream:\n\n1) Make the StreamConfiguration index a constructor parameter and a\n private struct member. This disallow the creation of CameraStream\n without a StreamConfiguration index assigned.\n\n2) Create CameraStream only after the associated StreamConfiguration\n has been identified. The first loop creates CameraStream for non-JPEG\n streams, the second for the JPEG ones after having identified the\n associated StreamConfiguration. Since we have just created the\n CameraStream, create the JPEG encoder at the same time instead of\n deferring it.\n\nThis change removes most accesses by index to the CameraDevice::streams_\nvector.\n\nNo functional changes intended, but this change aims to make the code\neasier to follow and more robust.\n\nSigned-off-by: Jacopo Mondi <jacopo@jmondi.org>\n---\n src/android/camera_device.cpp | 73 +++++++++++++++++++----------------\n src/android/camera_device.h | 18 +++++----\n 2 files changed, 50 insertions(+), 41 deletions(-)", "diff": "diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex a917404016e7..15dc12556cf5 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -168,8 +168,8 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n \t}\n }\n \n-CameraStream::CameraStream(PixelFormat f, Size s)\n-\t: index(-1), format(f), size(s), jpeg(nullptr)\n+CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)\n+\t: format(f), size(s), jpeg(nullptr), index_(i)\n {\n }\n \n@@ -1212,30 +1212,28 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n \t\tif (!format.isValid())\n \t\t\treturn -EINVAL;\n \n-\t\tstreams_.emplace_back(format, size);\n-\t\tstream->priv = static_cast<void *>(&streams_[i]);\n-\n \t\t/* Defer handling of MJPEG streams until all others are known. */\n \t\tif (stream->format == HAL_PIXEL_FORMAT_BLOB)\n \t\t\tcontinue;\n \n \t\tStreamConfiguration streamConfiguration;\n-\n \t\tstreamConfiguration.size = size;\n \t\tstreamConfiguration.pixelFormat = format;\n \n-\t\tstreams_[i].index = config_->addConfiguration(streamConfiguration);\n+\t\tunsigned int index = config_->addConfiguration(streamConfiguration);\n+\t\tCameraStream &cameraStream = streams_.emplace_back(format, size, index);\n+\t\tstream->priv = static_cast<void *>(&cameraStream);\n \t}\n \n \t/* Now handle MJPEG streams, adding a new stream if required. */\n \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n \t\tcamera3_stream_t *stream = stream_list->streams[i];\n-\t\tbool match = false;\n-\n \t\tif (stream->format != HAL_PIXEL_FORMAT_BLOB)\n \t\t\tcontinue;\n \n-\t\t/* Search for a compatible stream */\n+\t\t/* Search for a compatible stream in the non-JPEG ones. */\n+\t\tbool match = false;\n+\t\tunsigned int index;\n \t\tfor (unsigned int j = 0; j < config_->size(); j++) {\n \t\t\tStreamConfiguration &cfg = config_->at(j);\n \n@@ -1243,13 +1241,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n \t\t\t * \\todo The PixelFormat must also be compatible with\n \t\t\t * the encoder.\n \t\t\t */\n-\t\t\tif (cfg.size == streams_[i].size) {\n-\t\t\t\tLOG(HAL, Info) << \"Stream \" << i\n-\t\t\t\t\t << \" using libcamera stream \" << j;\n+\t\t\tif (cfg.size.width != stream->width ||\n+\t\t\t cfg.size.height != stream->height)\n+\t\t\t\tcontinue;\n \n-\t\t\t\tmatch = true;\n-\t\t\t\tstreams_[i].index = j;\n-\t\t\t}\n+\t\t\tLOG(HAL, Info) << \"Stream \" << i\n+\t\t\t\t << \" using libcamera stream \" << j;\n+\n+\t\t\tindex = j;\n+\t\t\tmatch = true;\n \t\t}\n \n \t\t/*\n@@ -1272,7 +1272,26 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n \t\t\tLOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n \t\t\t\t << \" for MJPEG support\";\n \n-\t\t\tstreams_[i].index = config_->addConfiguration(streamConfiguration);\n+\t\t\tindex = config_->addConfiguration(streamConfiguration);\n+\t\t}\n+\n+\t\tStreamConfiguration &cfg = config_->at(index);\n+\t\tPixelFormat format = formats::MJPEG;\n+\t\tSize size = cfg.size;\n+\n+\t\tCameraStream &cameraStream = streams_.emplace_back(format, size, index);\n+\t\tstream->priv = static_cast<void *>(&cameraStream);\n+\n+\t\t/*\n+\t\t * Construct a software encoder for MJPEG streams from the\n+\t\t * chosen libcamera source stream.\n+\t\t */\n+\t\tcameraStream.jpeg = new EncoderLibJpeg();\n+\t\tint ret = cameraStream.jpeg->configure(cfg);\n+\t\tif (ret) {\n+\t\t\tLOG(HAL, Error)\n+\t\t\t\t<< \"Failed to configure encoder\";\n+\t\t\treturn ret;\n \t\t}\n \t}\n \n@@ -1296,24 +1315,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n \t\tcamera3_stream_t *stream = stream_list->streams[i];\n \t\tCameraStream *cameraStream = &streams_[i];\n-\t\tStreamConfiguration &cfg = config_->at(cameraStream->index);\n+\t\tStreamConfiguration &cfg = config_->at(cameraStream->index());\n \n \t\t/* Use the bufferCount confirmed by the validation process. */\n \t\tstream->max_buffers = cfg.bufferCount;\n-\n-\t\t/*\n-\t\t * Construct a software encoder for MJPEG streams from the\n-\t\t * chosen libcamera source stream.\n-\t\t */\n-\t\tif (cameraStream->format == formats::MJPEG) {\n-\t\t\tcameraStream->jpeg = new EncoderLibJpeg();\n-\t\t\tint ret = cameraStream->jpeg->configure(cfg);\n-\t\t\tif (ret) {\n-\t\t\t\tLOG(HAL, Error)\n-\t\t\t\t\t<< \"Failed to configure encoder\";\n-\t\t\t\treturn ret;\n-\t\t\t}\n-\t\t}\n \t}\n \n \t/*\n@@ -1427,7 +1432,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t\t}\n \t\tdescriptor->frameBuffers.emplace_back(buffer);\n \n-\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n+\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n \t\tStream *stream = streamConfiguration->stream();\n \n \t\trequest->addBuffer(stream, buffer);\n@@ -1482,7 +1487,7 @@ void CameraDevice::requestComplete(Request *request)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n+\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n \t\tStream *stream = streamConfiguration->stream();\n \t\tFrameBuffer *buffer = request->findBuffer(stream);\n \t\tif (!buffer) {\ndiff --git a/src/android/camera_device.h b/src/android/camera_device.h\nindex 230e89b011e6..975c312c1c12 100644\n--- a/src/android/camera_device.h\n+++ b/src/android/camera_device.h\n@@ -28,20 +28,24 @@\n class CameraMetadata;\n \n struct CameraStream {\n-\tCameraStream(libcamera::PixelFormat, libcamera::Size);\n+public:\n+\tCameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n \t~CameraStream();\n \n-\t/*\n-\t * The index of the libcamera StreamConfiguration as added during\n-\t * configureStreams(). A single libcamera Stream may be used to deliver\n-\t * one or more streams to the Android framework.\n-\t */\n-\tunsigned int index;\n+\tunsigned int index() const { return index_; }\n \n \tlibcamera::PixelFormat format;\n \tlibcamera::Size size;\n \n \tEncoder *jpeg;\n+\n+private:\n+\t/*\n+\t * The index of the libcamera StreamConfiguration as added during\n+\t * configureStreams(). A single libcamera Stream may be used to deliver\n+\t * one or more streams to the Android framework.\n+\t */\n+\tunsigned int index_;\n };\n \n class CameraDevice : protected libcamera::Loggable\n", "prefixes": [ "libcamera-devel", "RFC", "5/7" ] }