[{"id":12413,"web_url":"https://patchwork.libcamera.org/comment/12413/","msgid":"<20200910110605.GL4095624@oden.dyn.berto.se>","date":"2020-09-10T11:06:05","subject":"Re: [libcamera-devel] [PATCH v3 10/11] android: camera_device: Set\n\tEncoder at construction","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-08 15:41:41 +0200, Jacopo Mondi wrote:\n> Make the CameraStream encoder a private unique pointer and require its\n> initialization at construction time. This ties the encoder lifetime to\n> the CameraStream it has been created with, allowing to remove the\n> CameraStream destructor.\n> \n> This change dis-allow creating a CameraStream and set the Encoder later,\n> which shall not happen now that we create CameraStream once we have all\n> the required information in place.\n> \n> No functional changes intended but this change aims to make the code\n> more robust enforcing a stricter CameraStream interface.\n> \n> Reviewed-by: Laurent Pinchart \n> Reviewed-by: Kieran Bingham \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 22 +++++++++-------------\n> src/android/camera_device.h | 8 ++++----\n> 2 files changed, 13 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index e0260c92246c..4c1416913d00 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -168,16 +168,11 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> \t}\n> }\n> \n> -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)\n> -\t: format(f), size(s), jpeg(nullptr), index_(i)\n> +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder)\n\nI would either name the argument 'e' or expand the other arguments to \ntheir full names mixing does not look nice.\n\n> +\t: format(f), size(s), index_(i), encoder_(encoder)\n> {\n> }\n> \n> -CameraStream::~CameraStream()\n> -{\n> -\tdelete jpeg;\n> -};\n> -\n> /*\n> * \\struct Camera3RequestDescriptor\n> *\n> @@ -1271,20 +1266,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\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> \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\tEncoder *encoder = new EncoderLibJpeg();\n> +\t\tint ret = encoder->configure(cfg);\n> \t\tif (ret) {\n> \t\t\tLOG(HAL, Error) << \"Failed to configure encoder\";\n> +\t\t\tdelete encoder;\n> \t\t\treturn ret;\n> \t\t}\n> +\n> +\t\tstreams_.emplace_back(formats::MJPEG, cfg.size, index, encoder);\n> +\t\tjpegStream->priv = static_cast(&streams_.back());\n\nSame comment as in previous patch about pointer to vector member. As \nthat should be sorted in that patch and I do like this change, with the \narguments comment addressed above,\n\nReviewed-by: Niklas Söderlund \n\n> \t}\n> \n> \tswitch (config_->validate()) {\n> @@ -1473,7 +1469,7 @@ void CameraDevice::requestComplete(Request *request)\n> \t\tif (cameraStream->format != formats::MJPEG)\n> \t\t\tcontinue;\n> \n> -\t\tEncoder *encoder = cameraStream->jpeg;\n> +\t\tEncoder *encoder = cameraStream->encoder();\n> \t\tif (!encoder) {\n> \t\t\tLOG(HAL, Error) << \"Failed to identify encoder\";\n> \t\t\tcontinue;\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index f8f237203ce9..5ac8f05e5a07 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -29,16 +29,15 @@ class CameraMetadata;\n> \n> struct CameraStream {\n> public:\n> -\tCameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> -\t~CameraStream();\n> +\tCameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,\n> +\t\t Encoder *encoder = nullptr);\n> \n> \tunsigned int index() const { return index_; }\n> +\tEncoder *encoder() const { return encoder_.get(); }\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> @@ -46,6 +45,7 @@ private:\n> \t * one or more streams to the Android framework.\n> \t */\n> \tunsigned int index_;\n> +\tstd::unique_ptr encoder_;\n> };\n> \n> class CameraDevice : protected libcamera::Loggable\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 466DEBDB1D\n\tfor ;\n\tThu, 10 Sep 2020 11:06:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B869862D72;\n\tThu, 10 Sep 2020 13:06:07 +0200 (CEST)","from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CF46462CE1\n\tfor ;\n\tThu, 10 Sep 2020 13:06:06 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id u4so7562724ljd.10\n\tfor ;\n\tThu, 10 Sep 2020 04:06:06 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\t144sm1268600lfj.35.2020.09.10.04.06.05\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 10 Sep 2020 04:06:05 -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=\"TXvHV3RI\"; 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=vvKinpXM+lb0s/QZt9FZgdOO50ZjnZXKFN/AzEYzE+w=;\n\tb=TXvHV3RI0vyMfru4w0Q0aTjEbiu02Pj2V+lqG8lFxZes3VTr8PqaAiATnETCyr7kxm\n\t16puKwtMOVeH1jyP/BH1Xmuupi16/lkAWwogAugmEziVFPkedeKBJT/9qXmdGzgZykTm\n\t0M+uyZsoXlnU8vyjFFF9zkh2jSYVZPlDlDjsUQoOyfT+J565U5u42U/gcVvrpPkRr+pV\n\tq/9SjlxwBkJcqyqSaMkMKkYQ2Z4HiFhwU9aXeiEKBG6KPrk/6KvgVKwXeOFoKmnn+kIH\n\tGZDhRlxR5IUgvnMg7y/8GBN/PtBK5RWKA4zVtKtVR2X7pwIb44ZwXw3DDkKF7IIZdvG+\n\txhGQ==","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=vvKinpXM+lb0s/QZt9FZgdOO50ZjnZXKFN/AzEYzE+w=;\n\tb=IGS0XJTmA5RGksa2J9+o2U//TrpAq4sLcAjOxka4pItcaIGb1O9Kq2vfc5PkwyJemP\n\tjRCVvH5xdOrP98LtK3oWCl7RJs0DURsMGYk30Il20Clt3Ai01y4azR7CVjsltIJQVtKm\n\tRsdKWKlfX/xDVatWxOz8Meba6FqsfvXp150mlGc35OPBGcn3+Q7ywQ7kXSf/euao5ytp\n\t05zEQ/mXmsffVrvebC8iYtRETgPFuxV8fDuW4DU4PEL7GcVQtNVgQDtijIxLbYtaTS5N\n\tM9I1jnkVjrK0icrP59nU5YV0b8/MWkpnNJ8mCsBYuxIZO6yEUWWqJX/SEtdAMOmqBdrE\n\tpjcw==","X-Gm-Message-State":"AOAM531+dIyD/wZOXcTSel27/RFtaG4ooxc3iPmOy2gOZY+oEOUVgPdJ\n\tU3fT05UIFqcjHtmBd6CZx+b+ZVskfs7QbQ==","X-Google-Smtp-Source":"ABdhPJxuqToYDYuLqZHCsFrVLPYE7hqs2txGCeAq/35Yh9XzTd/K/s44w+VX5UEwhvA/4fG989MlmA==","X-Received":"by 2002:a2e:9c9a:: with SMTP id x26mr4258370lji.56.1599735966240;\n\tThu, 10 Sep 2020 04:06:06 -0700 (PDT)","Date":"Thu, 10 Sep 2020 13:06:05 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200910110605.GL4095624@oden.dyn.berto.se>","References":"<20200908134142.27470-1-jacopo@jmondi.org>\n\t<20200908134142.27470-11-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200908134142.27470-11-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 10/11] android: camera_device: Set\n\tEncoder at construction","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":12417,"web_url":"https://patchwork.libcamera.org/comment/12417/","msgid":"<20200910111547.u6rzqchrqpvalzl2@uno.localdomain>","date":"2020-09-10T11:15:47","subject":"Re: [libcamera-devel] [PATCH v3 10/11] android: camera_device: Set\n\tEncoder at construction","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:06:05PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2020-09-08 15:41:41 +0200, Jacopo Mondi wrote:\n> > Make the CameraStream encoder a private unique pointer and require its\n> > initialization at construction time. This ties the encoder lifetime to\n> > the CameraStream it has been created with, allowing to remove the\n> > CameraStream destructor.\n> >\n> > This change dis-allow creating a CameraStream and set the Encoder later,\n> > which shall not happen now that we create CameraStream once we have all\n> > the required information in place.\n> >\n> > No functional changes intended but this change aims to make the code\n> > more robust enforcing a stricter CameraStream interface.\n> >\n> > Reviewed-by: Laurent Pinchart \n> > Reviewed-by: Kieran Bingham \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 22 +++++++++-------------\n> > src/android/camera_device.h | 8 ++++----\n> > 2 files changed, 13 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index e0260c92246c..4c1416913d00 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -168,16 +168,11 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > \t}\n> > }\n> >\n> > -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)\n> > -\t: format(f), size(s), jpeg(nullptr), index_(i)\n> > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder)\n>\n> I would either name the argument 'e' or expand the other arguments to\n> their full names mixing does not look nice.\n\nArguments names are expanded in the last patch of the series.\n\n>\n> > +\t: format(f), size(s), index_(i), encoder_(encoder)\n> > {\n> > }\n> >\n> > -CameraStream::~CameraStream()\n> > -{\n> > -\tdelete jpeg;\n> > -};\n> > -\n> > /*\n> > * \\struct Camera3RequestDescriptor\n> > *\n> > @@ -1271,20 +1266,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\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> > \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\tEncoder *encoder = new EncoderLibJpeg();\n> > +\t\tint ret = encoder->configure(cfg);\n> > \t\tif (ret) {\n> > \t\t\tLOG(HAL, Error) << \"Failed to configure encoder\";\n> > +\t\t\tdelete encoder;\n> > \t\t\treturn ret;\n> > \t\t}\n> > +\n> > +\t\tstreams_.emplace_back(formats::MJPEG, cfg.size, index, encoder);\n> > +\t\tjpegStream->priv = static_cast(&streams_.back());\n>\n> Same comment as in previous patch about pointer to vector member. As\n> that should be sorted in that patch and I do like this change, with the\n> arguments comment addressed above,\n>\n> Reviewed-by: Niklas Söderlund \n>\n> > \t}\n> >\n> > \tswitch (config_->validate()) {\n> > @@ -1473,7 +1469,7 @@ void CameraDevice::requestComplete(Request *request)\n> > \t\tif (cameraStream->format != formats::MJPEG)\n> > \t\t\tcontinue;\n> >\n> > -\t\tEncoder *encoder = cameraStream->jpeg;\n> > +\t\tEncoder *encoder = cameraStream->encoder();\n> > \t\tif (!encoder) {\n> > \t\t\tLOG(HAL, Error) << \"Failed to identify encoder\";\n> > \t\t\tcontinue;\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index f8f237203ce9..5ac8f05e5a07 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -29,16 +29,15 @@ class CameraMetadata;\n> >\n> > struct CameraStream {\n> > public:\n> > -\tCameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> > -\t~CameraStream();\n> > +\tCameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,\n> > +\t\t Encoder *encoder = nullptr);\n> >\n> > \tunsigned int index() const { return index_; }\n> > +\tEncoder *encoder() const { return encoder_.get(); }\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> > @@ -46,6 +45,7 @@ private:\n> > \t * one or more streams to the Android framework.\n> > \t */\n> > \tunsigned int index_;\n> > +\tstd::unique_ptr encoder_;\n> > };\n> >\n> > class CameraDevice : protected libcamera::Loggable\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 BDD6BC3B5B\n\tfor ;\n\tThu, 10 Sep 2020 11:12:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 99C5F62D27;\n\tThu, 10 Sep 2020 13:12:00 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5762E62D04\n\tfor ;\n\tThu, 10 Sep 2020 13:11:59 +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 relay1-d.mail.gandi.net (Postfix) with ESMTPSA id BA8ED240003;\n\tThu, 10 Sep 2020 11:11:58 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 10 Sep 2020 13:15:47 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200910111547.u6rzqchrqpvalzl2@uno.localdomain>","References":"<20200908134142.27470-1-jacopo@jmondi.org>\n\t<20200908134142.27470-11-jacopo@jmondi.org>\n\t<20200910110605.GL4095624@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200910110605.GL4095624@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v3 10/11] android: camera_device: Set\n\tEncoder at construction","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":12439,"web_url":"https://patchwork.libcamera.org/comment/12439/","msgid":"","date":"2020-09-11T02:40:57","subject":"Re: [libcamera-devel] [PATCH v3 10/11] android: camera_device: Set\n\tEncoder at construction","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 8:12 PM Jacopo Mondi wrote:\n>\n> Hi Niklas,\n>\n> On Thu, Sep 10, 2020 at 01:06:05PM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2020-09-08 15:41:41 +0200, Jacopo Mondi wrote:\n> > > Make the CameraStream encoder a private unique pointer and require its\n> > > initialization at construction time. This ties the encoder lifetime to\n> > > the CameraStream it has been created with, allowing to remove the\n> > > CameraStream destructor.\n> > >\n> > > This change dis-allow creating a CameraStream and set the Encoder later,\n> > > which shall not happen now that we create CameraStream once we have all\n> > > the required information in place.\n> > >\n> > > No functional changes intended but this change aims to make the code\n> > > more robust enforcing a stricter CameraStream interface.\n> > >\n> > > Reviewed-by: Laurent Pinchart \n> > > Reviewed-by: Kieran Bingham \n> > > Signed-off-by: Jacopo Mondi \n\n\nReviewed-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_device.cpp | 22 +++++++++-------------\n> > > src/android/camera_device.h | 8 ++++----\n> > > 2 files changed, 13 insertions(+), 17 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index e0260c92246c..4c1416913d00 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -168,16 +168,11 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > > }\n> > > }\n> > >\n> > > -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)\n> > > - : format(f), size(s), jpeg(nullptr), index_(i)\n> > > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder)\n> >\n> > I would either name the argument 'e' or expand the other arguments to\n> > their full names mixing does not look nice.\n>\n> Arguments names are expanded in the last patch of the series.\n>\n> >\n> > > + : format(f), size(s), index_(i), encoder_(encoder)\n> > > {\n> > > }\n> > >\n> > > -CameraStream::~CameraStream()\n> > > -{\n> > > - delete jpeg;\n> > > -};\n> > > -\n> > > /*\n> > > * \\struct Camera3RequestDescriptor\n> > > *\n> > > @@ -1271,20 +1266,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > }\n> > >\n> > > StreamConfiguration &cfg = config_->at(index);\n> > > - CameraStream &cameraStream =\n> > > - streams_.emplace_back(formats::MJPEG, cfg.size, index);\n> > > - jpegStream->priv = static_cast(&cameraStream);\n> > >\n> > > /*\n> > > * Construct a software encoder for MJPEG streams from the\n> > > * chosen libcamera source stream.\n> > > */\n> > > - cameraStream.jpeg = new EncoderLibJpeg();\n> > > - int ret = cameraStream.jpeg->configure(cfg);\n> > > + Encoder *encoder = new EncoderLibJpeg();\n> > > + int ret = encoder->configure(cfg);\n> > > if (ret) {\n> > > LOG(HAL, Error) << \"Failed to configure encoder\";\n> > > + delete encoder;\n> > > return ret;\n> > > }\n> > > +\n> > > + streams_.emplace_back(formats::MJPEG, cfg.size, index, encoder);\n> > > + jpegStream->priv = static_cast(&streams_.back());\n> >\n> > Same comment as in previous patch about pointer to vector member. As\n> > that should be sorted in that patch and I do like this change, with the\n> > arguments comment addressed above,\n> >\n> > Reviewed-by: Niklas Söderlund \n> >\n> > > }\n> > >\n> > > switch (config_->validate()) {\n> > > @@ -1473,7 +1469,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > if (cameraStream->format != formats::MJPEG)\n> > > continue;\n> > >\n> > > - Encoder *encoder = cameraStream->jpeg;\n> > > + Encoder *encoder = cameraStream->encoder();\n> > > if (!encoder) {\n> > > LOG(HAL, Error) << \"Failed to identify encoder\";\n> > > continue;\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index f8f237203ce9..5ac8f05e5a07 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -29,16 +29,15 @@ class CameraMetadata;\n> > >\n> > > struct CameraStream {\n> > > public:\n> > > - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> > > - ~CameraStream();\n> > > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,\n> > > + Encoder *encoder = nullptr);\n> > >\n> > > unsigned int index() const { return index_; }\n> > > + Encoder *encoder() const { return encoder_.get(); }\n> > >\n> > > libcamera::PixelFormat format;\n> > > libcamera::Size size;\n> > >\n> > > - Encoder *jpeg;\n> > > -\n> > > private:\n> > > /*\n> > > * The index of the libcamera StreamConfiguration as added during\n> > > @@ -46,6 +45,7 @@ private:\n> > > * one or more streams to the Android framework.\n> > > */\n> > > unsigned int index_;\n> > > + std::unique_ptr encoder_;\n> > > };\n> > >\n> > > class CameraDevice : protected libcamera::Loggable\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 C103CC3B5B\n\tfor ;\n\tFri, 11 Sep 2020 02:41:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C30962D4B;\n\tFri, 11 Sep 2020 04:41:09 +0200 (CEST)","from mail-ej1-x641.google.com (mail-ej1-x641.google.com\n\t[IPv6:2a00:1450:4864:20::641])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 457A66037B\n\tfor ;\n\tFri, 11 Sep 2020 04:41:08 +0200 (CEST)","by mail-ej1-x641.google.com with SMTP id e23so11736152eja.3\n\tfor ;\n\tThu, 10 Sep 2020 19:41:08 -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=\"mq5gP7mn\"; 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=n0nNEu7WTWWXTtELFgyuDyzF0cIsKtqL7Cm/nTMCkZw=;\n\tb=mq5gP7mnvgyySJpN8XQhH4iJzMesHD5uGEe7JIfpZdw9h1RQ1rbTI02kCNOIiqm8vP\n\t04DetmPetT6x+0pIsljR1imnbpGNxSRaTghpu7PqvfkCk7KFMj0s5CRcRuS6kHznNKvY\n\tbtl7gkJDpv512WJitpaKMHlw2+6rKXKV29Caw=","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=n0nNEu7WTWWXTtELFgyuDyzF0cIsKtqL7Cm/nTMCkZw=;\n\tb=FgXMT+p2pDwMMl2v+Q+ITvUezD5FFFRruUZom3iIhVXJPziw7TRB7D7s4oAhB4bG8J\n\tbLJDXzKF7QE0Gz3SLHH5oYusa74zTFd51Q/sNetXWB9+lZYfQHuOz33UR1+ruwvcYeoY\n\tQdW/8uuCYJuNFLgfQeFy23O7D4jT566itPyRwUXixMxnyk0ge3pcJuiCXhOMAH7xRXA7\n\tzfr2bNCToaeJ1gZq2grW901awNZ90bLR6mOcMiWohTFAf8Y12GNAI8k/NKbLlgQmOx5f\n\tPnoFJgb7tthexKN+XW2ClWvUvQFYuc64UzjJIDlEjmqyLT76VbhpOT0M3u7/eBuVNa/y\n\t8ASg==","X-Gm-Message-State":"AOAM5320LPViJwRsz0DgBVy8ELYWn0cn4f0PwRet/3HpaG2oyG1WbYuN\n\teMJRH6ICptMRslBZx6rs2MBT0rfrbswAoLHH/KFqPQ==","X-Google-Smtp-Source":"ABdhPJxxBN4gtixAB7yr9s2zYlI8HQGKFehVQOME9L3smYCd7Zh7XbC1c0pOXFlVBokFd+Er5GuUfEMOQ++Hwb8VLc4=","X-Received":"by 2002:a17:906:4d4d:: with SMTP id\n\tb13mr11614217ejv.221.1599792067904; \n\tThu, 10 Sep 2020 19:41:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20200908134142.27470-1-jacopo@jmondi.org>\n\t<20200908134142.27470-11-jacopo@jmondi.org>\n\t<20200910110605.GL4095624@oden.dyn.berto.se>\n\t<20200910111547.u6rzqchrqpvalzl2@uno.localdomain>","In-Reply-To":"<20200910111547.u6rzqchrqpvalzl2@uno.localdomain>","From":"Hirokazu Honda ","Date":"Fri, 11 Sep 2020 11:40:57 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 10/11] android: camera_device: Set\n\tEncoder at construction","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":12440,"web_url":"https://patchwork.libcamera.org/comment/12440/","msgid":"","date":"2020-09-11T02:42:34","subject":"Re: [libcamera-devel] [PATCH v3 10/11] android: camera_device: Set\n\tEncoder at construction","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Fri, Sep 11, 2020 at 11:40 AM Hirokazu Honda wrote:\n>\n> On Thu, Sep 10, 2020 at 8:12 PM Jacopo Mondi wrote:\n> >\n> > Hi Niklas,\n> >\n> > On Thu, Sep 10, 2020 at 01:06:05PM +0200, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2020-09-08 15:41:41 +0200, Jacopo Mondi wrote:\n> > > > Make the CameraStream encoder a private unique pointer and require its\n> > > > initialization at construction time. This ties the encoder lifetime to\n> > > > the CameraStream it has been created with, allowing to remove the\n> > > > CameraStream destructor.\n> > > >\n> > > > This change dis-allow creating a CameraStream and set the Encoder later,\n> > > > which shall not happen now that we create CameraStream once we have all\n> > > > the required information in place.\n> > > >\n> > > > No functional changes intended but this change aims to make the code\n> > > > more robust enforcing a stricter CameraStream interface.\n> > > >\n> > > > Reviewed-by: Laurent Pinchart \n> > > > Reviewed-by: Kieran Bingham \n> > > > Signed-off-by: Jacopo Mondi \n>\n>\n> Reviewed-by: Hirokazu Honda \n> > > > ---\n> > > > src/android/camera_device.cpp | 22 +++++++++-------------\n> > > > src/android/camera_device.h | 8 ++++----\n> > > > 2 files changed, 13 insertions(+), 17 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index e0260c92246c..4c1416913d00 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -168,16 +168,11 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > > > }\n> > > > }\n> > > >\n> > > > -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)\n> > > > - : format(f), size(s), jpeg(nullptr), index_(i)\n> > > > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder)\n> > >\n\nTo make sure we get the ownership,\ns/Encoder */std::unique_ptr/\n\n\n> > > I would either name the argument 'e' or expand the other arguments to\n> > > their full names mixing does not look nice.\n> >\n> > Arguments names are expanded in the last patch of the series.\n> >\n> > >\n> > > > + : format(f), size(s), index_(i), encoder_(encoder)\n> > > > {\n> > > > }\n> > > >\n> > > > -CameraStream::~CameraStream()\n> > > > -{\n> > > > - delete jpeg;\n> > > > -};\n> > > > -\n> > > > /*\n> > > > * \\struct Camera3RequestDescriptor\n> > > > *\n> > > > @@ -1271,20 +1266,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > }\n> > > >\n> > > > StreamConfiguration &cfg = config_->at(index);\n> > > > - CameraStream &cameraStream =\n> > > > - streams_.emplace_back(formats::MJPEG, cfg.size, index);\n> > > > - jpegStream->priv = static_cast(&cameraStream);\n> > > >\n> > > > /*\n> > > > * Construct a software encoder for MJPEG streams from the\n> > > > * chosen libcamera source stream.\n> > > > */\n> > > > - cameraStream.jpeg = new EncoderLibJpeg();\n> > > > - int ret = cameraStream.jpeg->configure(cfg);\n> > > > + Encoder *encoder = new EncoderLibJpeg();\n> > > > + int ret = encoder->configure(cfg);\n> > > > if (ret) {\n> > > > LOG(HAL, Error) << \"Failed to configure encoder\";\n> > > > + delete encoder;\n> > > > return ret;\n> > > > }\n> > > > +\n> > > > + streams_.emplace_back(formats::MJPEG, cfg.size, index, encoder);\n> > > > + jpegStream->priv = static_cast(&streams_.back());\n> > >\n> > > Same comment as in previous patch about pointer to vector member. As\n> > > that should be sorted in that patch and I do like this change, with the\n> > > arguments comment addressed above,\n> > >\n> > > Reviewed-by: Niklas Söderlund \n> > >\n> > > > }\n> > > >\n> > > > switch (config_->validate()) {\n> > > > @@ -1473,7 +1469,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > > if (cameraStream->format != formats::MJPEG)\n> > > > continue;\n> > > >\n> > > > - Encoder *encoder = cameraStream->jpeg;\n> > > > + Encoder *encoder = cameraStream->encoder();\n> > > > if (!encoder) {\n> > > > LOG(HAL, Error) << \"Failed to identify encoder\";\n> > > > continue;\n> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > index f8f237203ce9..5ac8f05e5a07 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -29,16 +29,15 @@ class CameraMetadata;\n> > > >\n> > > > struct CameraStream {\n> > > > public:\n> > > > - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> > > > - ~CameraStream();\n> > > > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,\n> > > > + Encoder *encoder = nullptr);\n> > > >\n> > > > unsigned int index() const { return index_; }\n> > > > + Encoder *encoder() const { return encoder_.get(); }\n> > > >\n> > > > libcamera::PixelFormat format;\n> > > > libcamera::Size size;\n> > > >\n> > > > - Encoder *jpeg;\n> > > > -\n> > > > private:\n> > > > /*\n> > > > * The index of the libcamera StreamConfiguration as added during\n> > > > @@ -46,6 +45,7 @@ private:\n> > > > * one or more streams to the Android framework.\n> > > > */\n> > > > unsigned int index_;\n> > > > + std::unique_ptr encoder_;\n> > > > };\n> > > >\n> > > > class CameraDevice : protected libcamera::Loggable\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 4C595C3B5B\n\tfor ;\n\tFri, 11 Sep 2020 02:42:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1548962D60;\n\tFri, 11 Sep 2020 04:42:47 +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 2B97662D27\n\tfor ;\n\tFri, 11 Sep 2020 04:42:45 +0200 (CEST)","by mail-ed1-x543.google.com with SMTP id b12so8404914edz.11\n\tfor ;\n\tThu, 10 Sep 2020 19:42:45 -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=\"NW66E2JJ\"; 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=G33kToHs8uzoUYOHJ5pg4FugpqpE8wsJ3zDv+zyWf7o=;\n\tb=NW66E2JJ9EwAMrtDIJM2HeoQ3ClLJDqu/hRwB3EINXrdnVk1IzY7tGNzNYb4vGavCU\n\t/UM4WarISrKMGsUrWh1yQYWDbUtrJU0PbQH+l0woLJ9XBeFBsvUuxBo7ZRHq2Bz2eeUI\n\tR1YSBYfKaX3PYPb7p/jfFrTsfwNYJ4D2YYOdU=","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=G33kToHs8uzoUYOHJ5pg4FugpqpE8wsJ3zDv+zyWf7o=;\n\tb=mlOmTSNeI1n54zM5/Gn2oca287p9VZDMyrv9tE26u8ZSr8uS/9tC4rXzY0zavGgDpy\n\tt7aNYqOQ4g5OQPVOqH3kSrLwQWZNJbMT1WtDq8SMsa/mWvL650HGXaWTXqJ+Iv1KbLFC\n\tfnzaFTUWDkKjaD0kCP2ZkVLTbUpZAz7AewLiE1OdfVo+luB47Xz6SVOUf6cVfdRfwKL0\n\t7EtkWkigB8RkiBhJcO5jEGsiYzSr6FX7aIVE2zDnlmh3oBep8xpsgbePoskWTxs55E8C\n\tmYATOkO3nuVI0LCBQQl1rQ9ZvDlHRJ4qbit+wsguRojmD6sUBcsEN16mKIngeT8bxDiS\n\t4yNg==","X-Gm-Message-State":"AOAM531gvcKPh/RcYfKelNEeOsuH9X4x671LjjTcu3m1Q447XFQ+/L8I\n\tQKIlN1AKbQQ96hhBuXpH58oU9m2ufYkJquDRPWa7ng==","X-Google-Smtp-Source":"ABdhPJyLtX9GFO3oKVeE5MPm8bTgl1DLCxIYknpGIv04gbvJywH/dtyiQfxGsdDpJ6u963BURjfBeeBXmGAZ6fY7fyY=","X-Received":"by 2002:aa7:d296:: with SMTP id\n\tw22mr12596804edq.327.1599792164790; \n\tThu, 10 Sep 2020 19:42:44 -0700 (PDT)","MIME-Version":"1.0","References":"<20200908134142.27470-1-jacopo@jmondi.org>\n\t<20200908134142.27470-11-jacopo@jmondi.org>\n\t<20200910110605.GL4095624@oden.dyn.berto.se>\n\t<20200910111547.u6rzqchrqpvalzl2@uno.localdomain>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 11 Sep 2020 11:42:34 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 10/11] android: camera_device: Set\n\tEncoder at construction","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":12457,"web_url":"https://patchwork.libcamera.org/comment/12457/","msgid":"<20200911163203.paulihc6lzcitbsz@uno.localdomain>","date":"2020-09-11T16:32:03","subject":"Re: [libcamera-devel] [PATCH v3 10/11] android: camera_device: Set\n\tEncoder at construction","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 11:42:34AM +0900, Hirokazu Honda wrote:\n> On Fri, Sep 11, 2020 at 11:40 AM Hirokazu Honda wrote:\n> >\n> > On Thu, Sep 10, 2020 at 8:12 PM Jacopo Mondi wrote:\n> > >\n> > > Hi Niklas,\n> > >\n> > > On Thu, Sep 10, 2020 at 01:06:05PM +0200, Niklas Söderlund wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thanks for your work.\n> > > >\n> > > > On 2020-09-08 15:41:41 +0200, Jacopo Mondi wrote:\n> > > > > Make the CameraStream encoder a private unique pointer and require its\n> > > > > initialization at construction time. This ties the encoder lifetime to\n> > > > > the CameraStream it has been created with, allowing to remove the\n> > > > > CameraStream destructor.\n> > > > >\n> > > > > This change dis-allow creating a CameraStream and set the Encoder later,\n> > > > > which shall not happen now that we create CameraStream once we have all\n> > > > > the required information in place.\n> > > > >\n> > > > > No functional changes intended but this change aims to make the code\n> > > > > more robust enforcing a stricter CameraStream interface.\n> > > > >\n> > > > > Reviewed-by: Laurent Pinchart \n> > > > > Reviewed-by: Kieran Bingham \n> > > > > Signed-off-by: Jacopo Mondi \n> >\n> >\n> > Reviewed-by: Hirokazu Honda \n> > > > > ---\n> > > > > src/android/camera_device.cpp | 22 +++++++++-------------\n> > > > > src/android/camera_device.h | 8 ++++----\n> > > > > 2 files changed, 13 insertions(+), 17 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index e0260c92246c..4c1416913d00 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -168,16 +168,11 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)\n> > > > > - : format(f), size(s), jpeg(nullptr), index_(i)\n> > > > > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder)\n> > > >\n>\n> To make sure we get the ownership,\n> s/Encoder */std::unique_ptr/\n>\n\nBut then I would have to move() at construction.. Is it worth it ?\nI'll check the usage pattern and see how it looks like.\n\nThanks\n j\n\n>\n> > > > I would either name the argument 'e' or expand the other arguments to\n> > > > their full names mixing does not look nice.\n> > >\n> > > Arguments names are expanded in the last patch of the series.\n> > >\n> > > >\n> > > > > + : format(f), size(s), index_(i), encoder_(encoder)\n> > > > > {\n> > > > > }\n> > > > >\n> > > > > -CameraStream::~CameraStream()\n> > > > > -{\n> > > > > - delete jpeg;\n> > > > > -};\n> > > > > -\n> > > > > /*\n> > > > > * \\struct Camera3RequestDescriptor\n> > > > > *\n> > > > > @@ -1271,20 +1266,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > > }\n> > > > >\n> > > > > StreamConfiguration &cfg = config_->at(index);\n> > > > > - CameraStream &cameraStream =\n> > > > > - streams_.emplace_back(formats::MJPEG, cfg.size, index);\n> > > > > - jpegStream->priv = static_cast(&cameraStream);\n> > > > >\n> > > > > /*\n> > > > > * Construct a software encoder for MJPEG streams from the\n> > > > > * chosen libcamera source stream.\n> > > > > */\n> > > > > - cameraStream.jpeg = new EncoderLibJpeg();\n> > > > > - int ret = cameraStream.jpeg->configure(cfg);\n> > > > > + Encoder *encoder = new EncoderLibJpeg();\n> > > > > + int ret = encoder->configure(cfg);\n> > > > > if (ret) {\n> > > > > LOG(HAL, Error) << \"Failed to configure encoder\";\n> > > > > + delete encoder;\n> > > > > return ret;\n> > > > > }\n> > > > > +\n> > > > > + streams_.emplace_back(formats::MJPEG, cfg.size, index, encoder);\n> > > > > + jpegStream->priv = static_cast(&streams_.back());\n> > > >\n> > > > Same comment as in previous patch about pointer to vector member. As\n> > > > that should be sorted in that patch and I do like this change, with the\n> > > > arguments comment addressed above,\n> > > >\n> > > > Reviewed-by: Niklas Söderlund \n> > > >\n> > > > > }\n> > > > >\n> > > > > switch (config_->validate()) {\n> > > > > @@ -1473,7 +1469,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > > > if (cameraStream->format != formats::MJPEG)\n> > > > > continue;\n> > > > >\n> > > > > - Encoder *encoder = cameraStream->jpeg;\n> > > > > + Encoder *encoder = cameraStream->encoder();\n> > > > > if (!encoder) {\n> > > > > LOG(HAL, Error) << \"Failed to identify encoder\";\n> > > > > continue;\n> > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > > index f8f237203ce9..5ac8f05e5a07 100644\n> > > > > --- a/src/android/camera_device.h\n> > > > > +++ b/src/android/camera_device.h\n> > > > > @@ -29,16 +29,15 @@ class CameraMetadata;\n> > > > >\n> > > > > struct CameraStream {\n> > > > > public:\n> > > > > - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> > > > > - ~CameraStream();\n> > > > > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,\n> > > > > + Encoder *encoder = nullptr);\n> > > > >\n> > > > > unsigned int index() const { return index_; }\n> > > > > + Encoder *encoder() const { return encoder_.get(); }\n> > > > >\n> > > > > libcamera::PixelFormat format;\n> > > > > libcamera::Size size;\n> > > > >\n> > > > > - Encoder *jpeg;\n> > > > > -\n> > > > > private:\n> > > > > /*\n> > > > > * The index of the libcamera StreamConfiguration as added during\n> > > > > @@ -46,6 +45,7 @@ private:\n> > > > > * one or more streams to the Android framework.\n> > > > > */\n> > > > > unsigned int index_;\n> > > > > + std::unique_ptr encoder_;\n> > > > > };\n> > > > >\n> > > > > class CameraDevice : protected libcamera::Loggable\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 E9EA5C3B5B\n\tfor ;\n\tFri, 11 Sep 2020 16:28:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C39AD62DAB;\n\tFri, 11 Sep 2020 18:28:15 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E097A60534\n\tfor ;\n\tFri, 11 Sep 2020 18:28:14 +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 relay2-d.mail.gandi.net (Postfix) with ESMTPSA id F1D0140006;\n\tFri, 11 Sep 2020 16:28:13 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 11 Sep 2020 18:32:03 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20200911163203.paulihc6lzcitbsz@uno.localdomain>","References":"<20200908134142.27470-1-jacopo@jmondi.org>\n\t<20200908134142.27470-11-jacopo@jmondi.org>\n\t<20200910110605.GL4095624@oden.dyn.berto.se>\n\t<20200910111547.u6rzqchrqpvalzl2@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 10/11] android: camera_device: Set\n\tEncoder at construction","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\" "}}]