Patch Detail
Show a patch.
GET /api/1.1/patches/9588/?format=api
{ "id": 9588, "url": "https://patchwork.libcamera.org/api/1.1/patches/9588/?format=api", "web_url": "https://patchwork.libcamera.org/patch/9588/", "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": "<20200912101129.12625-9-jacopo@jmondi.org>", "date": "2020-09-12T10:11:27", "name": "[libcamera-devel,v4,08/10] android: camera_device: Rework CameraStream handling", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "2d643d0541de5260c1de1c8a6bc7912027b85a05", "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/9588/mbox/", "series": [ { "id": 1287, "url": "https://patchwork.libcamera.org/api/1.1/series/1287/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=1287", "date": "2020-09-12T10:11:19", "name": "android: camera_device: Fix JPEG/RAW sizes", "version": 4, "mbox": "https://patchwork.libcamera.org/series/1287/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/9588/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/9588/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 293F1C3B5B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 12 Sep 2020 10:08:10 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01C8262DB4;\n\tSat, 12 Sep 2020 12:08:10 +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 582C162D27\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 12 Sep 2020 12:08:08 +0200 (CEST)", "from uno.lan (2-224-242-101.ip172.fastwebnet.it [2.224.242.101])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id DD6A51BF203;\n\tSat, 12 Sep 2020 10:08:06 +0000 (UTC)" ], "X-Originating-IP": "2.224.242.101", "From": "Jacopo Mondi <jacopo@jmondi.org>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Sat, 12 Sep 2020 12:11:27 +0200", "Message-Id": "<20200912101129.12625-9-jacopo@jmondi.org>", "X-Mailer": "git-send-email 2.28.0", "In-Reply-To": "<20200912101129.12625-1-jacopo@jmondi.org>", "References": "<20200912101129.12625-1-jacopo@jmondi.org>", "MIME-Version": "1.0", "Subject": "[libcamera-devel] [PATCH v4 08/10] 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": "hanlinchen@chromium.org", "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 all 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\nReviewed-by: Hirokazu Honda <hiroh@chromium.org>\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\nSigned-off-by: Jacopo Mondi <jacopo@jmondi.org>\n---\n src/android/camera_device.cpp | 91 ++++++++++++++++++-----------------\n src/android/camera_device.h | 18 ++++---\n 2 files changed, 57 insertions(+), 52 deletions(-)", "diff": "diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex af2905007b28..59acfd762a89 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -169,8 +169,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@@ -1191,6 +1191,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n \tstreams_.reserve(stream_list->num_streams);\n \n \t/* First handle all non-MJPEG streams. */\n+\tcamera3_stream_t *jpegStream = nullptr;\n \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n \t\tcamera3_stream_t *stream = stream_list->streams[i];\n \t\tSize size(stream->width, stream->height);\n@@ -1207,52 +1208,50 @@ 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\tif (stream->format == HAL_PIXEL_FORMAT_BLOB) {\n+\t\t\tjpegStream = stream;\n \t\t\tcontinue;\n+\t\t}\n \n \t\tStreamConfiguration streamConfiguration;\n-\n \t\tstreamConfiguration.size = size;\n \t\tstreamConfiguration.pixelFormat = format;\n \n \t\tconfig_->addConfiguration(streamConfiguration);\n-\t\tstreams_[i].index = config_->size() - 1;\n+\t\tunsigned int index = config_->size() - 1;\n+\t\tstreams_.emplace_back(format, size, index);\n+\t\tstream->priv = static_cast<void *>(&streams_.back());\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+\tif (jpegStream) {\n+\t\tint index = -1;\n \n-\t\t/* Search for a compatible stream */\n-\t\tfor (unsigned int j = 0; j < config_->size(); j++) {\n-\t\t\tStreamConfiguration &cfg = config_->at(j);\n+\t\t/* Search for a compatible stream in the non-JPEG ones. */\n+\t\tfor (unsigned int i = 0; i < config_->size(); i++) {\n+\t\t\tStreamConfiguration &cfg = config_->at(i);\n \n \t\t\t/*\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 != jpegStream->width ||\n+\t\t\t cfg.size.height != jpegStream->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)\n+\t\t\t\t<< \"Android JPEG stream mapped on stream \" << i;\n+\n+\t\t\tindex = i;\n+\t\t\tbreak;\n \t\t}\n \n \t\t/*\n \t\t * Without a compatible match for JPEG encoding we must\n \t\t * introduce a new stream to satisfy the request requirements.\n \t\t */\n-\t\tif (!match) {\n+\t\tif (index < 0) {\n \t\t\tStreamConfiguration streamConfiguration;\n \n \t\t\t/*\n@@ -1261,15 +1260,31 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n \t\t\t * handled, and should be considered as part of any\n \t\t\t * stream configuration reworks.\n \t\t\t */\n-\t\t\tstreamConfiguration.size.width = stream->width;\n-\t\t\tstreamConfiguration.size.height = stream->height;\n+\t\t\tstreamConfiguration.size.width = jpegStream->width;\n+\t\t\tstreamConfiguration.size.height = jpegStream->height;\n \t\t\tstreamConfiguration.pixelFormat = formats::NV12;\n \n \t\t\tLOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n \t\t\t\t << \" for MJPEG support\";\n \n \t\t\tconfig_->addConfiguration(streamConfiguration);\n-\t\t\tstreams_[i].index = config_->size() - 1;\n+\t\t\tindex = config_->size() - 1;\n+\t\t}\n+\n+\t\tStreamConfiguration &cfg = config_->at(index);\n+\t\tCameraStream &cameraStream =\n+\t\t\tstreams_.emplace_back(formats::MJPEG, cfg.size, index);\n+\t\tjpegStream->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) << \"Failed to configure encoder\";\n+\t\t\treturn ret;\n \t\t}\n \t}\n \n@@ -1292,25 +1307,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n \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\tCameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);\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@@ -1424,7 +1425,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@@ -1479,7 +1480,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 c0732fb8ed7f..376d001ea7d7 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", "v4", "08/10" ] }