[{"id":12532,"web_url":"https://patchwork.libcamera.org/comment/12532/","msgid":"<20200915225427.GH3998@pendragon.ideasonboard.com>","date":"2020-09-15T22:54:27","subject":"Re: [libcamera-devel] [PATCH v4 08/10] android: camera_device:\n\tRework CameraStream handling","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 Sat, Sep 12, 2020 at 12:11:27PM +0200, Jacopo Mondi wrote:\n> The CameraDevice::streams_ vector of CameraStream instances is\n> currently mostly accessed by index. The current implementation\n> creates all the CameraStream during the first loop that inspects the\n> camera3_stream instances, and the update the index of the\n\ns/the/then/\n\n> StreamConfiguration associated with the CameraStream during a second\n> loop that inspects MJPEG streams. A third loop creates the JPEG encoder\n> associated with CameraStreams that produce MJPEG format.\n\ns/CameraStreams/camera streams/ (or CameraStream instances)\n\n> As the index-based association is hard to follow and rather fragile,\n> rework the creation and handling of CameraStream:\n> \n> 1) Make the StreamConfiguration index a constructor parameter and a\n> private struct member. This disallow the creation of CameraStream\n\ns/ This disallow/This disallows/\n\n> without a StreamConfiguration index assigned.\n> \n> 2) 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> \n> This change removes all accesses by index to the CameraDevice::streams_\n> vector.\n> \n> No functional changes intended, but this change aims to make the code\n> easier to follow and more robust.\n> \n> Reviewed-by: Hirokazu Honda \n> Reviewed-by: Kieran Bingham \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 91 ++++++++++++++++++-----------------\n> src/android/camera_device.h | 18 ++++---\n> 2 files changed, 57 insertions(+), 52 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 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(&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\nI would add\n\n\t\t\tif (jpegStream) {\n\t\t\t\tLOG(HAL, Error)\n\t\t\t\t\t<< \"Multiple JPEG streams are not supported\";\n\t\t\t\treturn -EINVAL;\n\t\t\t}\n\nor something along those lines.\n\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(&streams_.back());\n> \t}\n> \n> \t/* Now handle MJPEG streams, adding a new stream if required. */\n\ns/MJPEG streams/the MPJEG stream/\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\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\nI would have searched in streams_ instead of config_, but the result\nshould be equivalent.\n\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\ns/on stream/to libcamera stream/\n\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(&cameraStream);\n> +\n> +\t\t/*\n> +\t\t * Construct a software encoder for MJPEG streams from the\n\ns/MJPEG streams/the MJPEG stream/\n\nReviewed-by: Laurent Pinchart \n\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(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) {\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 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","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 447B4BF01C\n\tfor ;\n\tTue, 15 Sep 2020 22:54:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BABBD62E13;\n\tWed, 16 Sep 2020 00:54:58 +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 652C562C8C\n\tfor ;\n\tWed, 16 Sep 2020 00:54:57 +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 BB144FD8;\n\tWed, 16 Sep 2020 00:54:56 +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=\"QSJiQOGF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600210497;\n\tbh=0dxIFZlMTeRPwjX2Fept8WbHbrsYcu7hbPGzwNOvo5U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QSJiQOGFGDXwR99BXhZKWC2XplnHaQWDBqnAYXSLy5RR+EVDyI7OgySAXoYWy5uYI\n\tmZbH5z4a2Pn+XuuXh/694eiZJVVs4MubMH7O6Zj37UNSm2Ce+R6mn1wgr0FuGoQkHM\n\tcFPbzh+3/6V+jYKzlSBcUxerHF/FgeonKhpX/YCc=","Date":"Wed, 16 Sep 2020 01:54:27 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200915225427.GH3998@pendragon.ideasonboard.com>","References":"<20200912101129.12625-1-jacopo@jmondi.org>\n\t<20200912101129.12625-9-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200912101129.12625-9-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 08/10] android: camera_device:\n\tRework CameraStream handling","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]