[{"id":14122,"web_url":"https://patchwork.libcamera.org/comment/14122/","msgid":"","date":"2020-12-08T03:40:08","subject":"Re: [libcamera-devel] [PATCH v3 2/3] android: camera_device: Use\n\tCamera3StreamConfig in configureStreams()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Mon, Dec 7, 2020 at 7:24 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Dec 07, 2020 at 08:42:17AM +0000, Hirokazu Honda wrote:\n> > This uses Camera3StreamConfig in CameraDevice::configureStreams().\n>\n> I would be slightly more verbose.\n>\n> \"Use the newly introduced Camera3StreamConfig to associate the\n> Android requested streams with the associated StreamConfiguration\n> in a vector of configurations.\n>\n> This change prepares to sort the vector of configuration before using\n> it to configure the Camera and populate the streams_ vector.\n>\n> No functional changes intended.\"\n>\n> And the signed-off-by of course.\n> > ---\n> > src/android/camera_device.cpp | 52 +++++++++++++++++++++--------------\n> > 1 file changed, 31 insertions(+), 21 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 09269d95..b7bf3d88 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1240,6 +1240,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > streams_.clear();\n> > streams_.reserve(stream_list->num_streams);\n> >\n> > + std::vector streamConfigs;\n> > + streamConfigs.reserve(stream_list->num_streams);\n> > +\n> > /* First handle all non-MJPEG streams. */\n> > camera3_stream_t *jpegStream = nullptr;\n> > for (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> > @@ -1270,14 +1273,12 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > continue;\n> > }\n> >\n> > - StreamConfiguration streamConfiguration;\n> > - streamConfiguration.size = size;\n> > - streamConfiguration.pixelFormat = format;\n> > -\n> > - config_->addConfiguration(streamConfiguration);\n> > - streams_.emplace_back(this, CameraStream::Type::Direct,\n> > - stream, config_->size() - 1);\n> > - stream->priv = static_cast(&streams_.back());\n> > + Camera3StreamConfig streamConfig;\n> > + streamConfig.streams = { stream };\n> > + streamConfig.types = { CameraStream::Type::Direct };\n> > + streamConfig.config.size = size;\n> > + streamConfig.config.pixelFormat = format;\n> > + streamConfigs.push_back(std::move(streamConfig));\n>\n> Nit: does this move help in any way with memory handling ? I mean,\n> I do expect the auto-generated move constructor of Camera3StreamConfig\n> not being able to magically move bits between the local variable and\n> the vector storage ? Am I wrong or what move() actually does is to\n> invalidate the local variable but copies data anyway in this case ?\n>\n> We're talking about a tiny number of operations not in an hot-path, so\n> this i very minor, but I wonder if emplace() wouldn't do better (or\n> just a push_back() it it's equivalent to move()).\n>\n\nSince std::move(streamConfig) move()s std::vectors (i.e. streams and\ntypes) to the new instance, it should be efficient than without\nstd::move.\nemplace_back(std::move(streamConfig)) is equivalent to\npush_back(std::move(streamConfig)).\ncf.) https://stackoverflow.com/a/26861516\n\nRegards,\n-Hiro\n\n\n> > }\n> >\n> > /* Now handle the MJPEG streams, adding a new stream if required. */\n> > @@ -1286,9 +1287,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > int index = -1;\n> >\n> > /* Search for a compatible stream in the non-JPEG ones. */\n> > - for (unsigned int i = 0; i < config_->size(); i++) {\n> > - StreamConfiguration &cfg = config_->at(i);\n> > -\n> > + for (size_t i = 0; i < streamConfigs.size(); ++i) {\n> > + const auto &cfg = streamConfigs[i].config;\n> > /*\n> > * \\todo The PixelFormat must also be compatible with\n> > * the encoder.\n> > @@ -1310,28 +1310,38 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > * introduce a new stream to satisfy the request requirements.\n> > */\n> > if (index < 0) {\n> > - StreamConfiguration streamConfiguration;\n> > -\n> > /*\n> > * \\todo The pixelFormat should be a 'best-fit' choice\n> > * and may require a validation cycle. This is not yet\n> > * handled, and should be considered as part of any\n> > * stream configuration reworks.\n> > */\n> > - streamConfiguration.size.width = jpegStream->width;\n> > - streamConfiguration.size.height = jpegStream->height;\n> > - streamConfiguration.pixelFormat = formats::NV12;\n> > + Camera3StreamConfig streamConfig;\n> > + streamConfig.config.size.width = jpegStream->width;\n> > + streamConfig.config.size.height = jpegStream->height;\n> > + streamConfig.config.pixelFormat = formats::NV12;\n> > + streamConfigs.push_back(std::move(streamConfig));\n> >\n> > - LOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n> > + LOG(HAL, Info) << \"Adding \" << streamConfig.config.toString()\n> > << \" for MJPEG support\";\n> >\n> > type = CameraStream::Type::Internal;\n> > - config_->addConfiguration(streamConfiguration);\n> > - index = config_->size() - 1;\n> > + index = streamConfigs.size() - 1;\n> > }\n> >\n> > - streams_.emplace_back(this, type, jpegStream, index);\n> > - jpegStream->priv = static_cast(&streams_.back());\n> > + streamConfigs[index].streams.push_back(jpegStream);\n> > + streamConfigs[index].types.push_back(type);\n> > + }\n> > +\n> > + for (const auto &streamConfig : streamConfigs) {\n> > + config_->addConfiguration(streamConfig.config);\n> > + for (size_t i = 0; i < streamConfig.streams.size(); ++i) {\n> > + camera3_stream_t *stream = streamConfig.streams[i];\n> > + const CameraStream::Type type = streamConfig.types[i];\n> > + streams_.emplace_back(this, type,\n> > + stream, config_->size() - 1);\n> > + stream->priv = static_cast(&streams_.back());\n> > + }\n>\n> I like where this is going!\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\n> > }\n> >\n> > switch (config_->validate()) {\n> > --\n> > 2.29.2.576.ga3fc446d84-goog","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 D1A7ABDB1F\n\tfor ;\n\tTue, 8 Dec 2020 03:40:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 588C667E6C;\n\tTue, 8 Dec 2020 04:40:22 +0100 (CET)","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 E93AF60323\n\tfor ;\n\tTue, 8 Dec 2020 04:40:20 +0100 (CET)","by mail-ed1-x543.google.com with SMTP id cw27so16093225edb.5\n\tfor ;\n\tMon, 07 Dec 2020 19:40:20 -0800 (PST)"],"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=\"l0P0ofag\"; 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; bh=kbMoleXUPchvjHzg91McVI+NvXD3k9HkC8ibD1k/EMc=;\n\tb=l0P0ofagIdHZyFYiSVnqpkOgy1Nb45y7oHTCdioxPBOhaKbXE8se6RJzCJxsLykhZx\n\t+Pb0x1h0feFz+HOrOmWnuqUND3nFaGb0VEgbCtgr28Bn1I4B5vi7QAjONnxOENpZCdZX\n\tFjBEOwTJqnVmqvPisRh3eTggzSa7PoovDuoGA=","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;\n\tbh=kbMoleXUPchvjHzg91McVI+NvXD3k9HkC8ibD1k/EMc=;\n\tb=bKfkIpTcOPJp+AYmK4vR2v9DdqfWprqQELcBL9CPWpRRm1YNXc74hx9VuasIrJLT7A\n\twcxYhT4u3i036mZbUzEQuiAE76coUkEuWau66gmvlCG0n3DzVIh4rHa2CgnyRaudZ7vZ\n\to9v50Wt+kbWEifD8nsJE+8M3KjoD8EM8gDTZiW0QbG8y8jnYcIqe9HlvdoOJSqygLx5i\n\t6xpg6EQFK8lvSE/qIykLMutoAL+jPz1m4t9BMLUapYP/d3uK5TwF8f+4FX1C5JeS7NvD\n\tvHyUi8A6c1Otnudv8j9WH6QFkQlftSJ4ezm4AeasxdRT3HtcBBfNlMXsNYHePuloe97d\n\tNBew==","X-Gm-Message-State":"AOAM533wS1H+lenCjHmK1WU1EwusvyHVJkkH0TG1b9UmoAbiT2UsNXrZ\n\tUEFwYoqkJtaJN1lR4GHAiondEBhYMzrfxlVS2sx74Js4LBB+tA==","X-Google-Smtp-Source":"ABdhPJzZ1sGuzLd2aM0BKmJyq4TJctCLpiQLlqOjjqaB6NkPa27YsjyBuaBu3/fjGPMsX7FP36ufGTOw+E3z6B7NyF4=","X-Received":"by 2002:a05:6402:13d1:: with SMTP id\n\ta17mr19447591edx.202.1607398820526; \n\tMon, 07 Dec 2020 19:40:20 -0800 (PST)","MIME-Version":"1.0","References":"<20201207084218.2307410-1-hiroh@chromium.org>\n\t<20201207084218.2307410-2-hiroh@chromium.org>\n\t<20201207102445.yv36ozjubctjvslj@uno.localdomain>","In-Reply-To":"<20201207102445.yv36ozjubctjvslj@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 8 Dec 2020 12:40:08 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] android: camera_device: Use\n\tCamera3StreamConfig in configureStreams()","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\" "}}]