[{"id":12323,"web_url":"https://patchwork.libcamera.org/comment/12323/","msgid":"<20200905220947.GN6319@pendragon.ideasonboard.com>","date":"2020-09-05T22:09:47","subject":"Re: [libcamera-devel] [PATCH v2 11/12] android: camera_device: Set\n\tEncoder at construction","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 Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote:\n> Make the CameraStream Encoder * a private struct member and require its\n> initialization at construction time.\n> \n> This change dis-allow creating a CameraStream and set the Encoder later,\n\ns/dis-allow/disallows/\n\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\nThinking ahead in the future, do you think this will be a good match\nwhen we'll add other post-processing than JPEG encoding (I'm thinking\nabout scaling, rotation and/or format conversion) ? I don't see any\nparticular issue, but a second opinion would be useful.\n\n> Reviewed-by: Kieran Bingham \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/android/camera_device.cpp | 23 ++++++++++++-----------\n> src/android/camera_device.h | 7 ++++---\n> 2 files changed, 16 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 9bcd1d993c17..28d8e081eab0 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -168,14 +168,14 @@ 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> +\t: format(f), size(s), index_(i), encoder_(encoder)\n> {\n> }\n> \n> CameraStream::~CameraStream()\n> {\n> -\tdelete jpeg;\n> +\tdelete encoder_;\n> };\n> \n> /*\n> @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\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(&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)\n> -\t\t\t\t<< \"Failed to configure encoder\";\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\tCameraStream &cameraStream = streams_.emplace_back(format, size,\n> +\t\t\t\t\t\t\t\t index, encoder);\n> +\t\tstream->priv = static_cast(&cameraStream);\n> \t}\n> \n> \tswitch (config_->validate()) {\n> @@ -1481,7 +1482,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..3c57ffec265d 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -29,16 +29,16 @@ class CameraMetadata;\n> \n> struct CameraStream {\n> public:\n> -\tCameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> +\tCameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,\n> +\t\t Encoder *encoder = nullptr);\n> \t~CameraStream();\n> \n> \tunsigned int index() const { return index_; }\n> +\tEncoder *encoder() const { return encoder_; }\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 +46,7 @@ private:\n> \t * one or more streams to the Android framework.\n> \t */\n> \tunsigned int index_;\n> +\tEncoder *encoder_;\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 DDA3ABDB1D\n\tfor ;\n\tSat, 5 Sep 2020 22:10:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7C47362B5B;\n\tSun, 6 Sep 2020 00:10:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AB0BC60599\n\tfor ;\n\tSun, 6 Sep 2020 00:10:12 +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 173BE335;\n\tSun, 6 Sep 2020 00:10:11 +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=\"R+3h89Nq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599343811;\n\tbh=5L1g7bJqIxP3rRCm5l1vzgetajrL7HZnIwIqros8jT8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R+3h89NqW5UOaWDf//t956DpClI8mEDD74AcKvbhgsOJBTTThgaPAhUzfjYNAKosa\n\tiDbdWNt3j5uFdhNE6FjNtpb5k5iArOjtkGpYrqQLF/jIiZ1ntckrOpbDRhBDLIJRBH\n\tdHs5tmZda5GEhYzlB0/opHOvg0SGJkfkXUymmCDg=","Date":"Sun, 6 Sep 2020 01:09:47 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200905220947.GN6319@pendragon.ideasonboard.com>","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-12-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200902152236.69770-12-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 11/12] 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":"tfiga@google.com, libcamera-devel@lists.libcamera.org, 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\" "}},{"id":12350,"web_url":"https://patchwork.libcamera.org/comment/12350/","msgid":"<20200907085610.eh6bcewyn5ef3cnf@uno.localdomain>","date":"2020-09-07T08:56:10","subject":"Re: [libcamera-devel] [PATCH v2 11/12] 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":"On Sun, Sep 06, 2020 at 01:09:47AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote:\n> > Make the CameraStream Encoder * a private struct member and require its\n> > initialization at construction time.\n> >\n> > This change dis-allow creating a CameraStream and set the Encoder later,\n>\n> s/dis-allow/disallows/\n>\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> Thinking ahead in the future, do you think this will be a good match\n> when we'll add other post-processing than JPEG encoding (I'm thinking\n> about scaling, rotation and/or format conversion) ? I don't see any\n> particular issue, but a second opinion would be useful.\n\nAs discussed with Kieran and briefly with Tomasz, I think we'll have\nto define how to establish which Encoder (or whatever name it will\nbe give) interface derived class to instantiate here.\n\nI think this part will need to be greatly expanded, and I'm not yet\nsure to which direction (I would likely say a factory of some kind,\nbut I think establishing the criteria for selecting which derived\nclass to use is more pressing than the implementation itself).\n\nThanks\n j\n\n>\n> > Reviewed-by: Kieran Bingham \n> > Signed-off-by: Jacopo Mondi \n>\n> Reviewed-by: Laurent Pinchart \n>\n> > ---\n> > src/android/camera_device.cpp | 23 ++++++++++++-----------\n> > src/android/camera_device.h | 7 ++++---\n> > 2 files changed, 16 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 9bcd1d993c17..28d8e081eab0 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -168,14 +168,14 @@ 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> > +\t: format(f), size(s), index_(i), encoder_(encoder)\n> > {\n> > }\n> >\n> > CameraStream::~CameraStream()\n> > {\n> > -\tdelete jpeg;\n> > +\tdelete encoder_;\n> > };\n> >\n> > /*\n> > @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\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(&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)\n> > -\t\t\t\t<< \"Failed to configure encoder\";\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\tCameraStream &cameraStream = streams_.emplace_back(format, size,\n> > +\t\t\t\t\t\t\t\t index, encoder);\n> > +\t\tstream->priv = static_cast(&cameraStream);\n> > \t}\n> >\n> > \tswitch (config_->validate()) {\n> > @@ -1481,7 +1482,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..3c57ffec265d 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -29,16 +29,16 @@ class CameraMetadata;\n> >\n> > struct CameraStream {\n> > public:\n> > -\tCameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> > +\tCameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,\n> > +\t\t Encoder *encoder = nullptr);\n> > \t~CameraStream();\n> >\n> > \tunsigned int index() const { return index_; }\n> > +\tEncoder *encoder() const { return encoder_; }\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 +46,7 @@ private:\n> > \t * one or more streams to the Android framework.\n> > \t */\n> > \tunsigned int index_;\n> > +\tEncoder *encoder_;\n> > };\n> >\n> > class CameraDevice : protected libcamera::Loggable\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 C8149BDB1C\n\tfor ;\n\tMon, 7 Sep 2020 08:52:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 535FC62B82;\n\tMon, 7 Sep 2020 10:52:25 +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 D7EFD629B6\n\tfor ;\n\tMon, 7 Sep 2020 10:52:23 +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 EC65D240013;\n\tMon, 7 Sep 2020 08:52:22 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 7 Sep 2020 10:56:10 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200907085610.eh6bcewyn5ef3cnf@uno.localdomain>","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-12-jacopo@jmondi.org>\n\t<20200905220947.GN6319@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200905220947.GN6319@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 11/12] 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":"tfiga@google.com, libcamera-devel@lists.libcamera.org, 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\" "}},{"id":12364,"web_url":"https://patchwork.libcamera.org/comment/12364/","msgid":"","date":"2020-09-08T05:21:20","subject":"Re: [libcamera-devel] [PATCH v2 11/12] 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 Mon, Sep 7, 2020 at 5:52 PM Jacopo Mondi wrote:\n>\n> On Sun, Sep 06, 2020 at 01:09:47AM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote:\n> > > Make the CameraStream Encoder * a private struct member and require its\n> > > initialization at construction time.\n> > >\n> > > This change dis-allow creating a CameraStream and set the Encoder later,\n> >\n> > s/dis-allow/disallows/\n> >\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> > Thinking ahead in the future, do you think this will be a good match\n> > when we'll add other post-processing than JPEG encoding (I'm thinking\n> > about scaling, rotation and/or format conversion) ? I don't see any\n> > particular issue, but a second opinion would be useful.\n>\n> As discussed with Kieran and briefly with Tomasz, I think we'll have\n> to define how to establish which Encoder (or whatever name it will\n> be give) interface derived class to instantiate here.\n>\n> I think this part will need to be greatly expanded, and I'm not yet\n> sure to which direction (I would likely say a factory of some kind,\n> but I think establishing the criteria for selecting which derived\n> class to use is more pressing than the implementation itself).\n>\n> Thanks\n> j\n>\n> >\n> > > Reviewed-by: Kieran Bingham \n> > > Signed-off-by: Jacopo Mondi \n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > ---\n> > > src/android/camera_device.cpp | 23 ++++++++++++-----------\n> > > src/android/camera_device.h | 7 ++++---\n> > > 2 files changed, 16 insertions(+), 14 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 9bcd1d993c17..28d8e081eab0 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -168,14 +168,14 @@ 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> > > + : format(f), size(s), index_(i), encoder_(encoder)\n> > > {\n> > > }\n> > >\n> > > CameraStream::~CameraStream()\n> > > {\n> > > - delete jpeg;\n> > > + delete encoder_;\n> > > };\n> > >\n> > > /*\n> > > @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > PixelFormat format = formats::MJPEG;\n> > > Size size = cfg.size;\n> > >\n> > > - CameraStream &cameraStream = streams_.emplace_back(format, size, index);\n> > > - stream->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)\n> > > - << \"Failed to configure encoder\";\n> > > + LOG(HAL, Error) << \"Failed to configure encoder\";\n> > > + delete encoder;\n> > > return ret;\n> > > }\n> > > +\n> > > + CameraStream &cameraStream = streams_.emplace_back(format, size,\n> > > + index, encoder);\n> > > + stream->priv = static_cast(&cameraStream);\n> > > }\n> > >\n> > > switch (config_->validate()) {\n> > > @@ -1481,7 +1482,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..3c57ffec265d 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -29,16 +29,16 @@ class CameraMetadata;\n> > >\n> > > struct CameraStream {\n> > > public:\n> > > - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> > > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,\n> > > + Encoder *encoder = nullptr);\n> > > ~CameraStream();\n> > >\n> > > unsigned int index() const { return index_; }\n> > > + Encoder *encoder() const { return encoder_; }\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 +46,7 @@ private:\n> > > * one or more streams to the Android framework.\n> > > */\n> > > unsigned int index_;\n> > > + Encoder *encoder_;\n> > > };\n> > >\n> > > class CameraDevice : protected libcamera::Loggable\n> >\n\nI would like to manage encoder_ with std::unique_ptr<> in CameraStream.\n\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 8DB49BDB1C\n\tfor ;\n\tTue, 8 Sep 2020 05:21:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 252AD62BAA;\n\tTue, 8 Sep 2020 07:21:33 +0200 (CEST)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A3CC7603EE\n\tfor ;\n\tTue, 8 Sep 2020 07:21:31 +0200 (CEST)","by mail-ed1-x52a.google.com with SMTP id n13so14631871edo.10\n\tfor ;\n\tMon, 07 Sep 2020 22:21:31 -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=\"n5Z2aXzc\"; 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=u3+i8PjqsagNU6L+q59xYHY4KcowPVa+a0cDAwJYLQI=;\n\tb=n5Z2aXzc7fm0/fT3i0e+W560UyjRxyvK0J0aj1pJPR1+QSf2wZ3PBXPWdrTPcwOsWK\n\tQumOVi0gHkggSwCBWsYdOHb14lCYRwrDcmZvhxcBMuMOjMsZAR+dIuev93xSKJ112tQc\n\tZfrWVncqLWqpivh8dm1tuf2C5EP2dSELn94VA=","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=u3+i8PjqsagNU6L+q59xYHY4KcowPVa+a0cDAwJYLQI=;\n\tb=a2ezXn4bgHchcpwqwH8+kCK18RyT5bCO+5urIo4WrR4AAawTZtyCAgcYRMI5C5rswV\n\tt9PHch3Y4beNLMGGKM0wjixrrYFlIcolFKYkwE2K1LOa545Kszc+XahK94qzHRV4oNxn\n\tGAHQG4rIY8jdSHn8I75co9bQovTUHE0fE3jJ61ZAwa2YHynsERxVimsv9zvNm3wAgj8o\n\tuFcVEswj9tAFtyBnmPwtM5SYBjfhHl00f2CUnA/etkBRbqd8xzU0Zl4kEkmBM9mRhM9F\n\t61uT+3gnbjuD+0WJcNGxEHJN1egQkkIRaMXvCB7hYQEshawRywcHcLjE0kn3Q2G4id7D\n\tVTLw==","X-Gm-Message-State":"AOAM5324f54X/C9ev9ry17LpXOrcrMDZvhD+Zvpv9NFf4mBWJTL9VEGe\n\tSKuy03fYA6AgfeIJfNtKkCuenS+ZmtaAEnimBVZQn2a6MnA=","X-Google-Smtp-Source":"ABdhPJzc1B2tblIw1gOiojJEjkqnvOVByUFYb1a38+Drls6ULve3oJISePz59Y8yPukRitU6jeVWVL59/A4TauZZqns=","X-Received":"by 2002:aa7:d296:: with SMTP id\n\tw22mr25227149edq.327.1599542491259; \n\tMon, 07 Sep 2020 22:21:31 -0700 (PDT)","MIME-Version":"1.0","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-12-jacopo@jmondi.org>\n\t<20200905220947.GN6319@pendragon.ideasonboard.com>\n\t<20200907085610.eh6bcewyn5ef3cnf@uno.localdomain>","In-Reply-To":"<20200907085610.eh6bcewyn5ef3cnf@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 8 Sep 2020 14:21:20 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 11/12] 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":"Tomasz Figa , libcamera-devel@lists.libcamera.org,\n\tHirokazu Honda ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12366,"web_url":"https://patchwork.libcamera.org/comment/12366/","msgid":"","date":"2020-09-08T05:59:06","subject":"Re: [libcamera-devel] [PATCH v2 11/12] 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 Tue, Sep 8, 2020 at 2:21 PM Hirokazu Honda wrote:\n>\n> On Mon, Sep 7, 2020 at 5:52 PM Jacopo Mondi wrote:\n> >\n> > On Sun, Sep 06, 2020 at 01:09:47AM +0300, Laurent Pinchart wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote:\n> > > > Make the CameraStream Encoder * a private struct member and require its\n> > > > initialization at construction time.\n> > > >\n> > > > This change dis-allow creating a CameraStream and set the Encoder later,\n> > >\n> > > s/dis-allow/disallows/\n> > >\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> > > Thinking ahead in the future, do you think this will be a good match\n> > > when we'll add other post-processing than JPEG encoding (I'm thinking\n> > > about scaling, rotation and/or format conversion) ? I don't see any\n> > > particular issue, but a second opinion would be useful.\n> >\n> > As discussed with Kieran and briefly with Tomasz, I think we'll have\n> > to define how to establish which Encoder (or whatever name it will\n> > be give) interface derived class to instantiate here.\n> >\n\nI am thinking of introducing PostProcessor interface and regard\nJpegEncoder as one of frame conversions.\nI would remove the existing Encoder interface.\n\nPostProcessor\n +-- FrameConverter (for format conversion)\n | +-- JpegEncoder.\n | |\n | +-- JpegDecoder.\n |\n +-- FrameScaler (for frame scaling and cropping)\n +-- YUVScaler\n\nWhat do you think?\n\n> > I think this part will need to be greatly expanded, and I'm not yet\n> > sure to which direction (I would likely say a factory of some kind,\n> > but I think establishing the criteria for selecting which derived\n> > class to use is more pressing than the implementation itself).\n> >\n> > Thanks\n> > j\n> >\n> > >\n> > > > Reviewed-by: Kieran Bingham \n> > > > Signed-off-by: Jacopo Mondi \n> > >\n> > > Reviewed-by: Laurent Pinchart \n> > >\n> > > > ---\n> > > > src/android/camera_device.cpp | 23 ++++++++++++-----------\n> > > > src/android/camera_device.h | 7 ++++---\n> > > > 2 files changed, 16 insertions(+), 14 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index 9bcd1d993c17..28d8e081eab0 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -168,14 +168,14 @@ 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> > > > + : format(f), size(s), index_(i), encoder_(encoder)\n> > > > {\n> > > > }\n> > > >\n> > > > CameraStream::~CameraStream()\n> > > > {\n> > > > - delete jpeg;\n> > > > + delete encoder_;\n> > > > };\n> > > >\n> > > > /*\n> > > > @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > PixelFormat format = formats::MJPEG;\n> > > > Size size = cfg.size;\n> > > >\n> > > > - CameraStream &cameraStream = streams_.emplace_back(format, size, index);\n> > > > - stream->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)\n> > > > - << \"Failed to configure encoder\";\n> > > > + LOG(HAL, Error) << \"Failed to configure encoder\";\n> > > > + delete encoder;\n> > > > return ret;\n> > > > }\n> > > > +\n> > > > + CameraStream &cameraStream = streams_.emplace_back(format, size,\n> > > > + index, encoder);\n> > > > + stream->priv = static_cast(&cameraStream);\n> > > > }\n> > > >\n> > > > switch (config_->validate()) {\n> > > > @@ -1481,7 +1482,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..3c57ffec265d 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -29,16 +29,16 @@ class CameraMetadata;\n> > > >\n> > > > struct CameraStream {\n> > > > public:\n> > > > - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> > > > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,\n> > > > + Encoder *encoder = nullptr);\n> > > > ~CameraStream();\n> > > >\n> > > > unsigned int index() const { return index_; }\n> > > > + Encoder *encoder() const { return encoder_; }\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 +46,7 @@ private:\n> > > > * one or more streams to the Android framework.\n> > > > */\n> > > > unsigned int index_;\n> > > > + Encoder *encoder_;\n> > > > };\n> > > >\n> > > > class CameraDevice : protected libcamera::Loggable\n> > >\n>\n> I would like to manage encoder_ with std::unique_ptr<> in CameraStream.\n>\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\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 68BF9BDB1C\n\tfor ;\n\tTue, 8 Sep 2020 05:59:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CF23962BAA;\n\tTue, 8 Sep 2020 07:59:18 +0200 (CEST)","from mail-ej1-x636.google.com (mail-ej1-x636.google.com\n\t[IPv6:2a00:1450:4864:20::636])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C2A2A603EE\n\tfor ;\n\tTue, 8 Sep 2020 07:59:17 +0200 (CEST)","by mail-ej1-x636.google.com with SMTP id i26so20725397ejb.12\n\tfor ;\n\tMon, 07 Sep 2020 22:59:17 -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=\"VkRRHdm8\"; 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=9ist4hBJ7Tpo9hCYhjuPbQxEtQFUE5S4T3AqWXGaS9A=;\n\tb=VkRRHdm8mY5sxH/+pKd0CCYsTR9tmgPOnH5zIxINcYC6fiLb1cTgpCFoXe7sS8e1Qx\n\tVXRADE+t0doK2xIsJcuiFlYJRWRNgpWq33CH8/EKSbbuyBA3OanuM/55uLW7jLBay7Hw\n\tsNdCNYix99+1sjyi0cwQE1tuaoFI+lyjZ9J7E=","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=9ist4hBJ7Tpo9hCYhjuPbQxEtQFUE5S4T3AqWXGaS9A=;\n\tb=HdjwIX+t6Wp+WGEYXLu5WLRrg2Xaouom1K6E4GFVCZYdwdlqGFHDkPn4KoyWutWF+0\n\t32nhL+hNgIZz3mxRo2GnFrue5TsiyJDPFHJPC5LoEtyHse1UElyO95Vy5dOXbjsZo5Kf\n\tGHhD+PSAsVT1PBbby/wo94yb/dXwCvd3LGI6Ng1YYSLF2Sr3OAlILU1HgDgthaBmC4J1\n\taR+H0YJhx4/x85chSsnCtwDplzZ253J3g68ppNZUK+icRDOp4qkgCKmB4QrUiLV72FuB\n\tdqHH6uMFcK76/XyGUKUFKVPGz21scJ3qtvuhmylzsCUUl/8hrK/2azLicOSb6oQeAyRV\n\tzdTQ==","X-Gm-Message-State":"AOAM530ww66OA5LU2jodDBAg/WWbEyTt7Is1k+k521CcfXMpyKe8r6me\n\tfUVdUcOl80Qj0KkAIOTM3y1YNbOFT1uqLaq2bSfR/g==","X-Google-Smtp-Source":"ABdhPJyMM9jr+r+APX6Hl90LH/nqiYZutvA/kH/kNaBaMMvuh2C6i1CbDFp1+66gPbhb8o+zlgW0WNfgkYXXYyvfx50=","X-Received":"by 2002:a17:906:3506:: with SMTP id\n\tr6mr24145521eja.55.1599544757364; \n\tMon, 07 Sep 2020 22:59:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-12-jacopo@jmondi.org>\n\t<20200905220947.GN6319@pendragon.ideasonboard.com>\n\t<20200907085610.eh6bcewyn5ef3cnf@uno.localdomain>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 8 Sep 2020 14:59:06 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 11/12] 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":"Tomasz Figa , libcamera-devel@lists.libcamera.org,\n\tHirokazu Honda ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12367,"web_url":"https://patchwork.libcamera.org/comment/12367/","msgid":"<20200908063612.GV6047@pendragon.ideasonboard.com>","date":"2020-09-08T06:36:12","subject":"Re: [libcamera-devel] [PATCH v2 11/12] android: camera_device: Set\n\tEncoder at construction","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro-san,\n\nOn Tue, Sep 08, 2020 at 02:59:06PM +0900, Hirokazu Honda wrote:\n> On Tue, Sep 8, 2020 at 2:21 PM Hirokazu Honda wrote:\n> > On Mon, Sep 7, 2020 at 5:52 PM Jacopo Mondi wrote:\n> >> On Sun, Sep 06, 2020 at 01:09:47AM +0300, Laurent Pinchart wrote:\n> >>> Hi Jacopo,\n> >>>\n> >>> Thank you for the patch.\n> >>>\n> >>> On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote:\n> >>>> Make the CameraStream Encoder * a private struct member and require its\n> >>>> initialization at construction time.\n> >>>>\n> >>>> This change dis-allow creating a CameraStream and set the Encoder later,\n> >>>\n> >>> s/dis-allow/disallows/\n> >>>\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> >>> Thinking ahead in the future, do you think this will be a good match\n> >>> when we'll add other post-processing than JPEG encoding (I'm thinking\n> >>> about scaling, rotation and/or format conversion) ? I don't see any\n> >>> particular issue, but a second opinion would be useful.\n> >>\n> >> As discussed with Kieran and briefly with Tomasz, I think we'll have\n> >> to define how to establish which Encoder (or whatever name it will\n> >> be give) interface derived class to instantiate here.\n> \n> I am thinking of introducing PostProcessor interface and regard\n> JpegEncoder as one of frame conversions.\n> I would remove the existing Encoder interface.\n> \n> PostProcessor\n> +-- FrameConverter (for format conversion)\n> | +-- JpegEncoder.\n> | |\n> | +-- JpegDecoder.\n> |\n> +-- FrameScaler (for frame scaling and cropping)\n> +-- YUVScaler\n> \n> What do you think?\n\nI agree, I think all post-processing steps should have the same\ninterface. I'm not sure if the intermediate FrameConverter and\nFrameScaler classes are needed though. It may be more efficient in some\ncases to perform both scaling and format conversion in one go (for\ninstance if we use a JPEG hardware encoder that includes a scaler, or if\nwe need to scale and convert from YUV to RGB).\n\n> >> I think this part will need to be greatly expanded, and I'm not yet\n> >> sure to which direction (I would likely say a factory of some kind,\n> >> but I think establishing the criteria for selecting which derived\n> >> class to use is more pressing than the implementation itself).\n> >>\n> >> Thanks\n> >> j\n> >>\n> >>>\n> >>>> Reviewed-by: Kieran Bingham \n> >>>> Signed-off-by: Jacopo Mondi \n> >>>\n> >>> Reviewed-by: Laurent Pinchart \n> >>>\n> >>>> ---\n> >>>> src/android/camera_device.cpp | 23 ++++++++++++-----------\n> >>>> src/android/camera_device.h | 7 ++++---\n> >>>> 2 files changed, 16 insertions(+), 14 deletions(-)\n> >>>>\n> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>>> index 9bcd1d993c17..28d8e081eab0 100644\n> >>>> --- a/src/android/camera_device.cpp\n> >>>> +++ b/src/android/camera_device.cpp\n> >>>> @@ -168,14 +168,14 @@ 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> >>>> + : format(f), size(s), index_(i), encoder_(encoder)\n> >>>> {\n> >>>> }\n> >>>>\n> >>>> CameraStream::~CameraStream()\n> >>>> {\n> >>>> - delete jpeg;\n> >>>> + delete encoder_;\n> >>>> };\n> >>>>\n> >>>> /*\n> >>>> @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> >>>> PixelFormat format = formats::MJPEG;\n> >>>> Size size = cfg.size;\n> >>>>\n> >>>> - CameraStream &cameraStream = streams_.emplace_back(format, size, index);\n> >>>> - stream->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)\n> >>>> - << \"Failed to configure encoder\";\n> >>>> + LOG(HAL, Error) << \"Failed to configure encoder\";\n> >>>> + delete encoder;\n> >>>> return ret;\n> >>>> }\n> >>>> +\n> >>>> + CameraStream &cameraStream = streams_.emplace_back(format, size,\n> >>>> + index, encoder);\n> >>>> + stream->priv = static_cast(&cameraStream);\n> >>>> }\n> >>>>\n> >>>> switch (config_->validate()) {\n> >>>> @@ -1481,7 +1482,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..3c57ffec265d 100644\n> >>>> --- a/src/android/camera_device.h\n> >>>> +++ b/src/android/camera_device.h\n> >>>> @@ -29,16 +29,16 @@ class CameraMetadata;\n> >>>>\n> >>>> struct CameraStream {\n> >>>> public:\n> >>>> - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> >>>> + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,\n> >>>> + Encoder *encoder = nullptr);\n> >>>> ~CameraStream();\n> >>>>\n> >>>> unsigned int index() const { return index_; }\n> >>>> + Encoder *encoder() const { return encoder_; }\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 +46,7 @@ private:\n> >>>> * one or more streams to the Android framework.\n> >>>> */\n> >>>> unsigned int index_;\n> >>>> + Encoder *encoder_;\n> >>>> };\n> >>>>\n> >>>> class CameraDevice : protected libcamera::Loggable\n> >>>\n> >\n> > I would like to manage encoder_ with std::unique_ptr<> in CameraStream.","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 D16CFBDB1D\n\tfor ;\n\tTue, 8 Sep 2020 06:36:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 533E162BCE;\n\tTue, 8 Sep 2020 08:36:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED1EB603EE\n\tfor ;\n\tTue, 8 Sep 2020 08:36:37 +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 5FFB935;\n\tTue, 8 Sep 2020 08:36:37 +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=\"lw8mTPCw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599546997;\n\tbh=fmxu8FOgTOdhpJLGG7x8RDP2RcZbS0dKGh8LP+qqbiQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lw8mTPCw2v3fmxYfzzCK8LMMcsa0uhA3B09A76Tei8upcohFwso+ZIIBsJ1LvQ2Qt\n\tI14Dzfom0rN81ZCTBU6VCaCAT6dT7SaeREYTRooClE2CYjbxbkc1rdIm3oGcEkzhGj\n\tzwFkCSYwlHQ2S+mpAzhTaFGCW+XwexKoSYeZl2AE=","Date":"Tue, 8 Sep 2020 09:36:12 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"<20200908063612.GV6047@pendragon.ideasonboard.com>","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-12-jacopo@jmondi.org>\n\t<20200905220947.GN6319@pendragon.ideasonboard.com>\n\t<20200907085610.eh6bcewyn5ef3cnf@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 11/12] 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":"Tomasz Figa , libcamera-devel@lists.libcamera.org,\n\tHirokazu Honda ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12382,"web_url":"https://patchwork.libcamera.org/comment/12382/","msgid":"<20200908130220.ckfpinue5t3cnhqi@uno.localdomain>","date":"2020-09-08T13:02:20","subject":"Re: [libcamera-devel] [PATCH v2 11/12] 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, Laurent,\n\nOn Tue, Sep 08, 2020 at 09:36:12AM +0300, Laurent Pinchart wrote:\n> Hi Hiro-san,\n>\n> On Tue, Sep 08, 2020 at 02:59:06PM +0900, Hirokazu Honda wrote:\n> > On Tue, Sep 8, 2020 at 2:21 PM Hirokazu Honda wrote:\n> > > On Mon, Sep 7, 2020 at 5:52 PM Jacopo Mondi wrote:\n> > >> On Sun, Sep 06, 2020 at 01:09:47AM +0300, Laurent Pinchart wrote:\n> > >>> Hi Jacopo,\n> > >>>\n> > >>> Thank you for the patch.\n> > >>>\n> > >>> On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote:\n> > >>>> Make the CameraStream Encoder * a private struct member and require its\n> > >>>> initialization at construction time.\n> > >>>>\n> > >>>> This change dis-allow creating a CameraStream and set the Encoder later,\n> > >>>\n> > >>> s/dis-allow/disallows/\n> > >>>\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> > >>> Thinking ahead in the future, do you think this will be a good match\n> > >>> when we'll add other post-processing than JPEG encoding (I'm thinking\n> > >>> about scaling, rotation and/or format conversion) ? I don't see any\n> > >>> particular issue, but a second opinion would be useful.\n> > >>\n> > >> As discussed with Kieran and briefly with Tomasz, I think we'll have\n> > >> to define how to establish which Encoder (or whatever name it will\n> > >> be give) interface derived class to instantiate here.\n> >\n> > I am thinking of introducing PostProcessor interface and regard\n> > JpegEncoder as one of frame conversions.\n> > I would remove the existing Encoder interface.\n> >\n> > PostProcessor\n> > +-- FrameConverter (for format conversion)\n> > | +-- JpegEncoder.\n> > | |\n> > | +-- JpegDecoder.\n> > |\n> > +-- FrameScaler (for frame scaling and cropping)\n> > +-- YUVScaler\n> >\n> > What do you think?\n>\n> I agree, I think all post-processing steps should have the same\n> interface. I'm not sure if the intermediate FrameConverter and\n> FrameScaler classes are needed though. It may be more efficient in some\n> cases to perform both scaling and format conversion in one go (for\n> instance if we use a JPEG hardware encoder that includes a scaler, or if\n> we need to scale and convert from YUV to RGB).\n\nI don't have an idea clear enough to express myself on the layout of\nthe class hierarchy we'll end up implementing at this time.\n\nBut I agree this is the place where we'll need to instantiate the\nright Encoder derived class, based on criteria which I like to define\nsoon.\n\nProposals are welcome, also based on the experience on how this is\nhandled in CrOS.\n\nThanks\n j\n\n>\n> > >> I think this part will need to be greatly expanded, and I'm not yet\n> > >> sure to which direction (I would likely say a factory of some kind,\n> > >> but I think establishing the criteria for selecting which derived\n> > >> class to use is more pressing than the implementation itself).\n> > >>\n> > >> Thanks\n> > >> j\n> > >>\n> > >>>\n> > >>>> Reviewed-by: Kieran Bingham \n> > >>>> Signed-off-by: Jacopo Mondi \n> > >>>\n> > >>> Reviewed-by: Laurent Pinchart \n> > >>>\n> > >>>> ---\n> > >>>> src/android/camera_device.cpp | 23 ++++++++++++-----------\n> > >>>> src/android/camera_device.h | 7 ++++---\n> > >>>> 2 files changed, 16 insertions(+), 14 deletions(-)\n> > >>>>\n> > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > >>>> index 9bcd1d993c17..28d8e081eab0 100644\n> > >>>> --- a/src/android/camera_device.cpp\n> > >>>> +++ b/src/android/camera_device.cpp\n> > >>>> @@ -168,14 +168,14 @@ 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> > >>>> + : format(f), size(s), index_(i), encoder_(encoder)\n> > >>>> {\n> > >>>> }\n> > >>>>\n> > >>>> CameraStream::~CameraStream()\n> > >>>> {\n> > >>>> - delete jpeg;\n> > >>>> + delete encoder_;\n> > >>>> };\n> > >>>>\n> > >>>> /*\n> > >>>> @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > >>>> PixelFormat format = formats::MJPEG;\n> > >>>> Size size = cfg.size;\n> > >>>>\n> > >>>> - CameraStream &cameraStream = streams_.emplace_back(format, size, index);\n> > >>>> - stream->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)\n> > >>>> - << \"Failed to configure encoder\";\n> > >>>> + LOG(HAL, Error) << \"Failed to configure encoder\";\n> > >>>> + delete encoder;\n> > >>>> return ret;\n> > >>>> }\n> > >>>> +\n> > >>>> + CameraStream &cameraStream = streams_.emplace_back(format, size,\n> > >>>> + index, encoder);\n> > >>>> + stream->priv = static_cast(&cameraStream);\n> > >>>> }\n> > >>>>\n> > >>>> switch (config_->validate()) {\n> > >>>> @@ -1481,7 +1482,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..3c57ffec265d 100644\n> > >>>> --- a/src/android/camera_device.h\n> > >>>> +++ b/src/android/camera_device.h\n> > >>>> @@ -29,16 +29,16 @@ class CameraMetadata;\n> > >>>>\n> > >>>> struct CameraStream {\n> > >>>> public:\n> > >>>> - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> > >>>> + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,\n> > >>>> + Encoder *encoder = nullptr);\n> > >>>> ~CameraStream();\n> > >>>>\n> > >>>> unsigned int index() const { return index_; }\n> > >>>> + Encoder *encoder() const { return encoder_; }\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 +46,7 @@ private:\n> > >>>> * one or more streams to the Android framework.\n> > >>>> */\n> > >>>> unsigned int index_;\n> > >>>> + Encoder *encoder_;\n> > >>>> };\n> > >>>>\n> > >>>> class CameraDevice : protected libcamera::Loggable\n> > >>>\n> > >\n> > > I would like to manage encoder_ with std::unique_ptr<> in CameraStream.\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 5DFCABDB1D\n\tfor ;\n\tTue, 8 Sep 2020 12:58:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC62962C4B;\n\tTue, 8 Sep 2020 14:58:34 +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 5045C62C47\n\tfor ;\n\tTue, 8 Sep 2020 14:58:33 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 1E4441BF206;\n\tTue, 8 Sep 2020 12:58:31 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 8 Sep 2020 15:02:20 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200908130220.ckfpinue5t3cnhqi@uno.localdomain>","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-12-jacopo@jmondi.org>\n\t<20200905220947.GN6319@pendragon.ideasonboard.com>\n\t<20200907085610.eh6bcewyn5ef3cnf@uno.localdomain>\n\t\n\t\n\t<20200908063612.GV6047@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200908063612.GV6047@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 11/12] 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":"Tomasz Figa , libcamera-devel@lists.libcamera.org,\n\tHirokazu Honda ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12386,"web_url":"https://patchwork.libcamera.org/comment/12386/","msgid":"","date":"2020-09-08T14:37:59","subject":"Re: [libcamera-devel] [PATCH v2 11/12] android: camera_device: Set\n\tEncoder at construction","submitter":{"id":48,"url":"https://patchwork.libcamera.org/api/people/48/","name":"Tomasz Figa","email":"tfiga@google.com"},"content":"On Tue, Sep 8, 2020 at 8:36 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro-san,\n>\n> On Tue, Sep 08, 2020 at 02:59:06PM +0900, Hirokazu Honda wrote:\n> > On Tue, Sep 8, 2020 at 2:21 PM Hirokazu Honda wrote:\n> > > On Mon, Sep 7, 2020 at 5:52 PM Jacopo Mondi wrote:\n> > >> On Sun, Sep 06, 2020 at 01:09:47AM +0300, Laurent Pinchart wrote:\n> > >>> Hi Jacopo,\n> > >>>\n> > >>> Thank you for the patch.\n> > >>>\n> > >>> On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote:\n> > >>>> Make the CameraStream Encoder * a private struct member and require its\n> > >>>> initialization at construction time.\n> > >>>>\n> > >>>> This change dis-allow creating a CameraStream and set the Encoder later,\n> > >>>\n> > >>> s/dis-allow/disallows/\n> > >>>\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> > >>> Thinking ahead in the future, do you think this will be a good match\n> > >>> when we'll add other post-processing than JPEG encoding (I'm thinking\n> > >>> about scaling, rotation and/or format conversion) ? I don't see any\n> > >>> particular issue, but a second opinion would be useful.\n> > >>\n> > >> As discussed with Kieran and briefly with Tomasz, I think we'll have\n> > >> to define how to establish which Encoder (or whatever name it will\n> > >> be give) interface derived class to instantiate here.\n> >\n> > I am thinking of introducing PostProcessor interface and regard\n> > JpegEncoder as one of frame conversions.\n> > I would remove the existing Encoder interface.\n> >\n> > PostProcessor\n> > +-- FrameConverter (for format conversion)\n> > | +-- JpegEncoder.\n> > | |\n> > | +-- JpegDecoder.\n> > |\n> > +-- FrameScaler (for frame scaling and cropping)\n> > +-- YUVScaler\n> >\n> > What do you think?\n>\n> I agree, I think all post-processing steps should have the same\n> interface. I'm not sure if the intermediate FrameConverter and\n> FrameScaler classes are needed though. It may be more efficient in some\n> cases to perform both scaling and format conversion in one go (for\n> instance if we use a JPEG hardware encoder that includes a scaler, or if\n> we need to scale and convert from YUV to RGB).\n\nFrameConverter and FrameScaler could be interfaces, so both could be\nimplemented at the same time. On the other hand, if we end up with\nsomething like V4L2PostProcessor, the same class would likely\nimplement support for post processors which could perform\nscale+conversion, scale only or conversion only, so maybe such\ndistinction isn't necessary in the end. I guess we may need to dig\ndeeper into the actual implementation idea to see what's the best\nchoice. :)\n\nBest regards,\nTomasz\n\n>\n> > >> I think this part will need to be greatly expanded, and I'm not yet\n> > >> sure to which direction (I would likely say a factory of some kind,\n> > >> but I think establishing the criteria for selecting which derived\n> > >> class to use is more pressing than the implementation itself).\n> > >>\n> > >> Thanks\n> > >> j\n> > >>\n> > >>>\n> > >>>> Reviewed-by: Kieran Bingham \n> > >>>> Signed-off-by: Jacopo Mondi \n> > >>>\n> > >>> Reviewed-by: Laurent Pinchart \n> > >>>\n> > >>>> ---\n> > >>>> src/android/camera_device.cpp | 23 ++++++++++++-----------\n> > >>>> src/android/camera_device.h | 7 ++++---\n> > >>>> 2 files changed, 16 insertions(+), 14 deletions(-)\n> > >>>>\n> > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > >>>> index 9bcd1d993c17..28d8e081eab0 100644\n> > >>>> --- a/src/android/camera_device.cpp\n> > >>>> +++ b/src/android/camera_device.cpp\n> > >>>> @@ -168,14 +168,14 @@ 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> > >>>> + : format(f), size(s), index_(i), encoder_(encoder)\n> > >>>> {\n> > >>>> }\n> > >>>>\n> > >>>> CameraStream::~CameraStream()\n> > >>>> {\n> > >>>> - delete jpeg;\n> > >>>> + delete encoder_;\n> > >>>> };\n> > >>>>\n> > >>>> /*\n> > >>>> @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > >>>> PixelFormat format = formats::MJPEG;\n> > >>>> Size size = cfg.size;\n> > >>>>\n> > >>>> - CameraStream &cameraStream = streams_.emplace_back(format, size, index);\n> > >>>> - stream->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)\n> > >>>> - << \"Failed to configure encoder\";\n> > >>>> + LOG(HAL, Error) << \"Failed to configure encoder\";\n> > >>>> + delete encoder;\n> > >>>> return ret;\n> > >>>> }\n> > >>>> +\n> > >>>> + CameraStream &cameraStream = streams_.emplace_back(format, size,\n> > >>>> + index, encoder);\n> > >>>> + stream->priv = static_cast(&cameraStream);\n> > >>>> }\n> > >>>>\n> > >>>> switch (config_->validate()) {\n> > >>>> @@ -1481,7 +1482,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..3c57ffec265d 100644\n> > >>>> --- a/src/android/camera_device.h\n> > >>>> +++ b/src/android/camera_device.h\n> > >>>> @@ -29,16 +29,16 @@ class CameraMetadata;\n> > >>>>\n> > >>>> struct CameraStream {\n> > >>>> public:\n> > >>>> - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> > >>>> + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,\n> > >>>> + Encoder *encoder = nullptr);\n> > >>>> ~CameraStream();\n> > >>>>\n> > >>>> unsigned int index() const { return index_; }\n> > >>>> + Encoder *encoder() const { return encoder_; }\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 +46,7 @@ private:\n> > >>>> * one or more streams to the Android framework.\n> > >>>> */\n> > >>>> unsigned int index_;\n> > >>>> + Encoder *encoder_;\n> > >>>> };\n> > >>>>\n> > >>>> class CameraDevice : protected libcamera::Loggable\n> > >>>\n> > >\n> > > I would like to manage encoder_ with std::unique_ptr<> in CameraStream.\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 D6F93BDB1C\n\tfor ;\n\tTue, 8 Sep 2020 14:38:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 64AC262C37;\n\tTue, 8 Sep 2020 16:38:27 +0200 (CEST)","from mail-wr1-x433.google.com (mail-wr1-x433.google.com\n\t[IPv6:2a00:1450:4864:20::433])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 797896036E\n\tfor ;\n\tTue, 8 Sep 2020 16:38:25 +0200 (CEST)","by mail-wr1-x433.google.com with SMTP id k15so19342621wrn.10\n\tfor ;\n\tTue, 08 Sep 2020 07:38:25 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"gKKZlu0I\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=BYaBURtiMwPmW4TrG5XszC1lDo5IRUrMcFLYNAr3X2s=;\n\tb=gKKZlu0IGol6MxN+LkQbMFqwXIoC4lvX6xgnrKCCqMzXFPnH+GqQV+h5ugOvfj103H\n\tw6/U3zXVEQzPs2n38ilNL/3nrFwqKfHbTIDrgKALZxKU16e+fNTlKvUldKjirqM1ReRY\n\twwaC78k0FEo2Xd6Cl8FMPWf1OY8uzb3ZUJ6iyNrdPzQOO8cfeJJlZJzZG6Qmm0IIOd9J\n\tON8Ep+kjS/tSbrS1C9qATVTYtdJvzog0MFTkD01EwvIL7l5LPc/xGTyZtFrFyEiqQg38\n\tv/Ue8Xk+6jOvpmckGA1i+Lfw18qEEjKxpCHxqVFIIacGe+XKcmL/u4QFiITUv5m48Yg7\n\tTnzg==","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=BYaBURtiMwPmW4TrG5XszC1lDo5IRUrMcFLYNAr3X2s=;\n\tb=bqa1XrKb+8fGb1hapGRRR7ybKAYfRT7bT1lGMiVznL/z+ibiaHlc2cysuODWY7R1Bz\n\tbysC4BMsFTy1NtIxJNwf1KR8I8fu92wksl2iWijT3g6qCvPsINx2NF1N36xmDNt+21oN\n\tzrz391xkF/EmEcWVNs1ut6S2d93JVE6pSYK53i72FDQDVC2zcN3gfF52YCJbcaPLRPLT\n\t1ucC2ThMvEEsXk0HTiZkQMzUahpwd5H1UEVXMOsvWhox03FqY0JEEvfQZU3ZKbitgLlj\n\tTgK9QoWEP/9qwuFx2Bmni6c7KqWsdGY816YwwvCUpg/RNev77kivRLYNwk3fjJgxk+ZB\n\tPFpA==","X-Gm-Message-State":"AOAM532pU8XCNlDM06/C8UfkDXsyOHizQ7+jMZOsqGsDRJdcjMgun/7g\n\tcsySfg/oF+GRBqhjcV1XQkAjio4m3GLmhaYLAZ4xXQ==","X-Google-Smtp-Source":"ABdhPJz/qMN84O3HoYBl65g5Kezb1VZQjezTAx3oV/MiZ+6Kdy58dOT2XvwBfTM4W6Y/rmms2p+pRIyrfCdMyOjmPXo=","X-Received":"by 2002:adf:ff90:: with SMTP id\n\tj16mr29476145wrr.105.1599575904841; \n\tTue, 08 Sep 2020 07:38:24 -0700 (PDT)","MIME-Version":"1.0","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-12-jacopo@jmondi.org>\n\t<20200905220947.GN6319@pendragon.ideasonboard.com>\n\t<20200907085610.eh6bcewyn5ef3cnf@uno.localdomain>\n\t\n\t\n\t<20200908063612.GV6047@pendragon.ideasonboard.com>","In-Reply-To":"<20200908063612.GV6047@pendragon.ideasonboard.com>","From":"Tomasz Figa ","Date":"Tue, 8 Sep 2020 16:37:59 +0200","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 11/12] 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":"libcamera devel ,\n\tHirokazu Honda ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]