[{"id":12322,"web_url":"https://patchwork.libcamera.org/comment/12322/","msgid":"<20200905220524.GM6319@pendragon.ideasonboard.com>","date":"2020-09-05T22:05:24","subject":"Re: [libcamera-devel] [PATCH v2 10/12] android: camera_device:\n\tRework CameraStream handling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Sep 02, 2020 at 05:22:34PM +0200, Jacopo Mondi wrote:\n> The CameraDevice::streams_ vector of CameraStream instances is\n> currently mostly accessed by index. The current implementation\n> creates all the CameraStream during the first loop that inspects the\n> camera3_stream instances, and the update the index of the\n> StreamConfiguration associated with the CameraStream during a second\n> loop that inspects MJPEG streams. A third loop creates the JPEG encoder\n> associated with CameraStreams that produce MJPEG format.\n> \n> As the index-based association is hard to follow and rather fragile,\n> rework the creation and handling of CameraStream:\n> \n> 1) Make the StreamConfiguration index a constructor parameter and a\n>    private struct member.  This disallow the creation of CameraStream\n>    without a StreamConfiguration index assigned.\n> \n> 2) Create CameraStream only after the associated StreamConfiguration\n>    has been identified. The first loop creates CameraStream for non-JPEG\n>    streams, the second for the JPEG ones after having identified the\n>    associated StreamConfiguration. Since we have just created the\n>    CameraStream, create the JPEG encoder at the same time instead of\n>    deferring it.\n> \n> This change removes all accesses by index to the CameraDevice::streams_\n> vector.\n> \n> No functional changes intended, but this change aims to make the code\n> easier to follow and more robust.\n> \n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/android/camera_device.cpp | 75 +++++++++++++++++++----------------\n>  src/android/camera_device.h   | 18 +++++----\n>  2 files changed, 51 insertions(+), 42 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 3630e87e8814..9bcd1d993c17 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -168,8 +168,8 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n>  \t}\n>  }\n>  \n> -CameraStream::CameraStream(PixelFormat f, Size s)\n> -\t: index(-1), format(f), size(s), jpeg(nullptr)\n> +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)\n> +\t: format(f), size(s), jpeg(nullptr), index_(i)\n>  {\n>  }\n>  \n> @@ -1212,30 +1212,28 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>  \t\tif (!format.isValid())\n>  \t\t\treturn -EINVAL;\n>  \n> -\t\tstreams_.emplace_back(format, size);\n> -\t\tstream->priv = static_cast<void *>(&streams_[i]);\n> -\n>  \t\t/* Defer handling of MJPEG streams until all others are known. */\n>  \t\tif (stream->format == HAL_PIXEL_FORMAT_BLOB)\n>  \t\t\tcontinue;\n>  \n>  \t\tStreamConfiguration streamConfiguration;\n> -\n>  \t\tstreamConfiguration.size = size;\n>  \t\tstreamConfiguration.pixelFormat = format;\n>  \n> -\t\tstreams_[i].index = config_->addConfiguration(streamConfiguration);\n> +\t\tunsigned int index = config_->addConfiguration(streamConfiguration);\n> +\t\tCameraStream &cameraStream = streams_.emplace_back(format, size, index);\n> +\t\tstream->priv = static_cast<void *>(&cameraStream);\n\nThis is problematic. std::vector::emplace_back may cause a reallocation,\nwhich invalidates all iterators, rendering the pointer invalid. One\noption would be to turn streams_ into an std::list, another option to\nfill the priv pointers in a separate loop after populating streams_.\n\n>  \t}\n>  \n>  \t/* Now handle MJPEG streams, adding a new stream if required. */\n>  \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n>  \t\tcamera3_stream_t *stream = stream_list->streams[i];\n> -\t\tbool match = false;\n> -\n>  \t\tif (stream->format != HAL_PIXEL_FORMAT_BLOB)\n>  \t\t\tcontinue;\n\nDrive-by comment for a possible future simplification, can't we rely on\nthe fact that only a single JPEG stream will be requested ? If so, we\ncould store a pointer to the JPEG camera3_stream_configuration_t in the\nprevious loop, and have a\n\n\tif (jpegStream) {\n\t\t...\n\t}\n\nhere instead of a loop.\n\n>  \n> -\t\t/* Search for a compatible stream */\n> +\t\t/* Search for a compatible stream in the non-JPEG ones. */\n> +\t\tbool match = false;\n> +\t\tunsigned int index;\n>  \t\tfor (unsigned int j = 0; j < config_->size(); j++) {\n>  \t\t\tStreamConfiguration &cfg = config_->at(j);\n>  \n> @@ -1243,13 +1241,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>  \t\t\t * \\todo The PixelFormat must also be compatible with\n>  \t\t\t * the encoder.\n>  \t\t\t */\n> -\t\t\tif (cfg.size == streams_[i].size) {\n> -\t\t\t\tLOG(HAL, Info) << \"Stream \" << i\n> -\t\t\t\t\t       << \" using libcamera stream \" << j;\n> +\t\t\tif (cfg.size.width != stream->width ||\n> +\t\t\t    cfg.size.height != stream->height)\n> +\t\t\t\tcontinue;\n>  \n> -\t\t\t\tmatch = true;\n> -\t\t\t\tstreams_[i].index = j;\n> -\t\t\t}\n> +\t\t\tLOG(HAL, Info) << \"Stream \" << i\n\ns/Stream/Android stream/ ?\n\n> +\t\t\t\t       << \" using libcamera stream \" << j;\n> +\n> +\t\t\tindex = j;\n> +\t\t\tmatch = true;\n\nShould you break ?\n\n>  \t\t}\n>  \n>  \t\t/*\n> @@ -1272,7 +1272,26 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>  \t\t\tLOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n>  \t\t\t\t       << \" for MJPEG support\";\n>  \n> -\t\t\tstreams_[i].index = config_->addConfiguration(streamConfiguration);\n> +\t\t\tindex = config_->addConfiguration(streamConfiguration);\n> +\t\t}\n> +\n> +\t\tStreamConfiguration &cfg = config_->at(index);\n> +\t\tPixelFormat format = formats::MJPEG;\n> +\t\tSize size = cfg.size;\n> +\n> +\t\tCameraStream &cameraStream = streams_.emplace_back(format, size, index);\n\nNo need for the format and size local variables, you can write\n\n\t\tCameraStream &cameraStream =\n\t\t\tstreams_.emplace_back(formats::MJPEG, cfg.size, index);\n\n> +\t\tstream->priv = static_cast<void *>(&cameraStream);\n> +\n> +\t\t/*\n> +\t\t * Construct a software encoder for MJPEG streams from the\n> +\t\t * chosen libcamera source stream.\n> +\t\t */\n> +\t\tcameraStream.jpeg = new EncoderLibJpeg();\n> +\t\tint ret = cameraStream.jpeg->configure(cfg);\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error)\n> +\t\t\t\t<< \"Failed to configure encoder\";\n\nThis holds on a single line.\n\n\t\t\tLOG(HAL, Error) << \"Failed to configure encoder\";\n\n> +\t\t\treturn ret;\n>  \t\t}\n>  \t}\n>  \n> @@ -1295,25 +1314,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>  \n>  \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n>  \t\tcamera3_stream_t *stream = stream_list->streams[i];\n> -\t\tCameraStream *cameraStream = &streams_[i];\n> -\t\tStreamConfiguration &cfg = config_->at(cameraStream->index);\n> +\t\tCameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);\n> +\t\tStreamConfiguration &cfg = config_->at(cameraStream->index());\n>  \n>  \t\t/* Use the bufferCount confirmed by the validation process. */\n>  \t\tstream->max_buffers = cfg.bufferCount;\n> -\n> -\t\t/*\n> -\t\t * Construct a software encoder for MJPEG streams from the\n> -\t\t * chosen libcamera source stream.\n> -\t\t */\n> -\t\tif (cameraStream->format == formats::MJPEG) {\n> -\t\t\tcameraStream->jpeg = new EncoderLibJpeg();\n> -\t\t\tint ret = cameraStream->jpeg->configure(cfg);\n> -\t\t\tif (ret) {\n> -\t\t\t\tLOG(HAL, Error)\n> -\t\t\t\t\t<< \"Failed to configure encoder\";\n> -\t\t\t\treturn ret;\n> -\t\t\t}\n> -\t\t}\n>  \t}\n>  \n>  \t/*\n> @@ -1427,7 +1432,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>  \t\t}\n>  \t\tdescriptor->frameBuffers.emplace_back(buffer);\n>  \n> -\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n> +\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n>  \t\tStream *stream = streamConfiguration->stream();\n>  \n>  \t\trequest->addBuffer(stream, buffer);\n> @@ -1482,7 +1487,7 @@ void CameraDevice::requestComplete(Request *request)\n>  \t\t\tcontinue;\n>  \t\t}\n>  \n> -\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n> +\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n>  \t\tStream *stream = streamConfiguration->stream();\n>  \t\tFrameBuffer *buffer = request->findBuffer(stream);\n>  \t\tif (!buffer) {\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index dc0ee664d443..f8f237203ce9 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -28,20 +28,24 @@\n>  class CameraMetadata;\n>  \n>  struct CameraStream {\n\nCan you turn it into a class while at it ?\n\nAlso clearly not a candidate for this patch, do you think it would be\nuseful to rename the HAL-related classes with a HAL prefix (e.g.\nHALCamera instead of CameraDevice, HALStream instead of CameraStream) to\navoid confusing them with the libcamera classes ? We're familiar with\nthis code so it's probably not too confusing for us, but it may be for a\nnewcomer.\n\n> -\tCameraStream(libcamera::PixelFormat, libcamera::Size);\n> +public:\n> +\tCameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n\ni is not a very clear name, index would be better. I would also name the\nexisting parameters (that could be a separate change).\n\n>  \t~CameraStream();\n>  \n> -\t/*\n> -\t * The index of the libcamera StreamConfiguration as added during\n> -\t * configureStreams(). A single libcamera Stream may be used to deliver\n> -\t * one or more streams to the Android framework.\n> -\t */\n> -\tunsigned int index;\n> +\tunsigned int index() const { return index_; }\n>  \n>  \tlibcamera::PixelFormat format;\n>  \tlibcamera::Size size;\n>  \n>  \tEncoder *jpeg;\n> +\n> +private:\n> +\t/*\n> +\t * The index of the libcamera StreamConfiguration as added during\n> +\t * configureStreams(). A single libcamera Stream may be used to deliver\n> +\t * one or more streams to the Android framework.\n> +\t */\n\nShould this be moved to the .cpp file ? Documentation rules are a bit\nmore relaxed here though, as it's internal doc.\n\n> +\tunsigned int index_;\n>  };\n>  \n>  class CameraDevice : protected libcamera::Loggable","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 38E86BDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat,  5 Sep 2020 22:05:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C483962B5B;\n\tSun,  6 Sep 2020 00:05:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A21D960599\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  6 Sep 2020 00:05:48 +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 1DF56335;\n\tSun,  6 Sep 2020 00:05:48 +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=\"BAOX3aFz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599343548;\n\tbh=DkqGTXsUJle/2tXwOZWIfdKRfdKIL1VNSx8LpWe2j/g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BAOX3aFzwPx3eRsmAA0WdupfYJEVuPetI16jhv5LS/8FeDq2Zi6V9LZUturw05k+b\n\tR7mzuuHTVTFNo4jw206Q13OaP1ctl0/IAHvWMqleryqG3tlQ6dpwRiSkHw21GVRjAU\n\tnR9DsCorfleiIC00qvnRG+3FiNNY8JF48aU7GCBc=","Date":"Sun, 6 Sep 2020 01:05:24 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20200905220524.GM6319@pendragon.ideasonboard.com>","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-11-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200902152236.69770-11-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 10/12] android: camera_device:\n\tRework CameraStream handling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12349,"web_url":"https://patchwork.libcamera.org/comment/12349/","msgid":"<20200907085333.wly5lcay7hzc6bfq@uno.localdomain>","date":"2020-09-07T08:53:33","subject":"Re: [libcamera-devel] [PATCH v2 10/12] android: camera_device:\n\tRework CameraStream handling","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Sep 06, 2020 at 01:05:24AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 02, 2020 at 05:22:34PM +0200, Jacopo Mondi wrote:\n> > The CameraDevice::streams_ vector of CameraStream instances is\n> > currently mostly accessed by index. The current implementation\n> > creates all the CameraStream during the first loop that inspects the\n> > camera3_stream instances, and the update the index of the\n> > StreamConfiguration associated with the CameraStream during a second\n> > loop that inspects MJPEG streams. A third loop creates the JPEG encoder\n> > associated with CameraStreams that produce MJPEG format.\n> >\n> > As the index-based association is hard to follow and rather fragile,\n> > rework the creation and handling of CameraStream:\n> >\n> > 1) Make the StreamConfiguration index a constructor parameter and a\n> >    private struct member.  This disallow the creation of CameraStream\n> >    without a StreamConfiguration index assigned.\n> >\n> > 2) Create CameraStream only after the associated StreamConfiguration\n> >    has been identified. The first loop creates CameraStream for non-JPEG\n> >    streams, the second for the JPEG ones after having identified the\n> >    associated StreamConfiguration. Since we have just created the\n> >    CameraStream, create the JPEG encoder at the same time instead of\n> >    deferring it.\n> >\n> > This change removes all accesses by index to the CameraDevice::streams_\n> > vector.\n> >\n> > No functional changes intended, but this change aims to make the code\n> > easier to follow and more robust.\n> >\n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  src/android/camera_device.cpp | 75 +++++++++++++++++++----------------\n> >  src/android/camera_device.h   | 18 +++++----\n> >  2 files changed, 51 insertions(+), 42 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 3630e87e8814..9bcd1d993c17 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -168,8 +168,8 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> >  \t}\n> >  }\n> >\n> > -CameraStream::CameraStream(PixelFormat f, Size s)\n> > -\t: index(-1), format(f), size(s), jpeg(nullptr)\n> > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)\n> > +\t: format(f), size(s), jpeg(nullptr), index_(i)\n> >  {\n> >  }\n> >\n> > @@ -1212,30 +1212,28 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> >  \t\tif (!format.isValid())\n> >  \t\t\treturn -EINVAL;\n> >\n> > -\t\tstreams_.emplace_back(format, size);\n> > -\t\tstream->priv = static_cast<void *>(&streams_[i]);\n> > -\n> >  \t\t/* Defer handling of MJPEG streams until all others are known. */\n> >  \t\tif (stream->format == HAL_PIXEL_FORMAT_BLOB)\n> >  \t\t\tcontinue;\n> >\n> >  \t\tStreamConfiguration streamConfiguration;\n> > -\n> >  \t\tstreamConfiguration.size = size;\n> >  \t\tstreamConfiguration.pixelFormat = format;\n> >\n> > -\t\tstreams_[i].index = config_->addConfiguration(streamConfiguration);\n> > +\t\tunsigned int index = config_->addConfiguration(streamConfiguration);\n> > +\t\tCameraStream &cameraStream = streams_.emplace_back(format, size, index);\n> > +\t\tstream->priv = static_cast<void *>(&cameraStream);\n>\n> This is problematic. std::vector::emplace_back may cause a reallocation,\n\nWe have reserved space for all the possible cameraStream instances\nbefore entering this loop, didn't we ? It should save all relocations,\nor don't you think it's enough ?\n\n> which invalidates all iterators, rendering the pointer invalid. One\n> option would be to turn streams_ into an std::list, another option to\n> fill the priv pointers in a separate loop after populating streams_.\n>\n> >  \t}\n> >\n> >  \t/* Now handle MJPEG streams, adding a new stream if required. */\n> >  \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> >  \t\tcamera3_stream_t *stream = stream_list->streams[i];\n> > -\t\tbool match = false;\n> > -\n> >  \t\tif (stream->format != HAL_PIXEL_FORMAT_BLOB)\n> >  \t\t\tcontinue;\n>\n> Drive-by comment for a possible future simplification, can't we rely on\n> the fact that only a single JPEG stream will be requested ? If so, we\n> could store a pointer to the JPEG camera3_stream_configuration_t in the\n> previous loop, and have a\n>\n> \tif (jpegStream) {\n> \t\t...\n> \t}\n>\n> here instead of a loop.\n>\n\nAh yes, we can save this external loop indeed!\nThanks ;)\n\n> >\n> > -\t\t/* Search for a compatible stream */\n> > +\t\t/* Search for a compatible stream in the non-JPEG ones. */\n> > +\t\tbool match = false;\n> > +\t\tunsigned int index;\n> >  \t\tfor (unsigned int j = 0; j < config_->size(); j++) {\n> >  \t\t\tStreamConfiguration &cfg = config_->at(j);\n> >\n> > @@ -1243,13 +1241,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> >  \t\t\t * \\todo The PixelFormat must also be compatible with\n> >  \t\t\t * the encoder.\n> >  \t\t\t */\n> > -\t\t\tif (cfg.size == streams_[i].size) {\n> > -\t\t\t\tLOG(HAL, Info) << \"Stream \" << i\n> > -\t\t\t\t\t       << \" using libcamera stream \" << j;\n> > +\t\t\tif (cfg.size.width != stream->width ||\n> > +\t\t\t    cfg.size.height != stream->height)\n> > +\t\t\t\tcontinue;\n> >\n> > -\t\t\t\tmatch = true;\n> > -\t\t\t\tstreams_[i].index = j;\n> > -\t\t\t}\n> > +\t\t\tLOG(HAL, Info) << \"Stream \" << i\n>\n> s/Stream/Android stream/ ?\n>\n> > +\t\t\t\t       << \" using libcamera stream \" << j;\n> > +\n> > +\t\t\tindex = j;\n> > +\t\t\tmatch = true;\n>\n> Should you break ?\n>\n\nUps, I should indeed\n\n> >  \t\t}\n> >\n> >  \t\t/*\n> > @@ -1272,7 +1272,26 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> >  \t\t\tLOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n> >  \t\t\t\t       << \" for MJPEG support\";\n> >\n> > -\t\t\tstreams_[i].index = config_->addConfiguration(streamConfiguration);\n> > +\t\t\tindex = config_->addConfiguration(streamConfiguration);\n> > +\t\t}\n> > +\n> > +\t\tStreamConfiguration &cfg = config_->at(index);\n> > +\t\tPixelFormat format = formats::MJPEG;\n> > +\t\tSize size = cfg.size;\n> > +\n> > +\t\tCameraStream &cameraStream = streams_.emplace_back(format, size, index);\n>\n> No need for the format and size local variables, you can write\n>\n> \t\tCameraStream &cameraStream =\n> \t\t\tstreams_.emplace_back(formats::MJPEG, cfg.size, index);\n>\n\nKieran had a similar comment iirc. I'll change this!\n\n> > +\t\tstream->priv = static_cast<void *>(&cameraStream);\n> > +\n> > +\t\t/*\n> > +\t\t * Construct a software encoder for MJPEG streams from the\n> > +\t\t * chosen libcamera source stream.\n> > +\t\t */\n> > +\t\tcameraStream.jpeg = new EncoderLibJpeg();\n> > +\t\tint ret = cameraStream.jpeg->configure(cfg);\n> > +\t\tif (ret) {\n> > +\t\t\tLOG(HAL, Error)\n> > +\t\t\t\t<< \"Failed to configure encoder\";\n>\n> This holds on a single line.\n>\n> \t\t\tLOG(HAL, Error) << \"Failed to configure encoder\";\n\nRight!\n\n>\n> > +\t\t\treturn ret;\n> >  \t\t}\n> >  \t}\n> >\n> > @@ -1295,25 +1314,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> >\n> >  \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> >  \t\tcamera3_stream_t *stream = stream_list->streams[i];\n> > -\t\tCameraStream *cameraStream = &streams_[i];\n> > -\t\tStreamConfiguration &cfg = config_->at(cameraStream->index);\n> > +\t\tCameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);\n> > +\t\tStreamConfiguration &cfg = config_->at(cameraStream->index());\n> >\n> >  \t\t/* Use the bufferCount confirmed by the validation process. */\n> >  \t\tstream->max_buffers = cfg.bufferCount;\n> > -\n> > -\t\t/*\n> > -\t\t * Construct a software encoder for MJPEG streams from the\n> > -\t\t * chosen libcamera source stream.\n> > -\t\t */\n> > -\t\tif (cameraStream->format == formats::MJPEG) {\n> > -\t\t\tcameraStream->jpeg = new EncoderLibJpeg();\n> > -\t\t\tint ret = cameraStream->jpeg->configure(cfg);\n> > -\t\t\tif (ret) {\n> > -\t\t\t\tLOG(HAL, Error)\n> > -\t\t\t\t\t<< \"Failed to configure encoder\";\n> > -\t\t\t\treturn ret;\n> > -\t\t\t}\n> > -\t\t}\n> >  \t}\n> >\n> >  \t/*\n> > @@ -1427,7 +1432,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >  \t\t}\n> >  \t\tdescriptor->frameBuffers.emplace_back(buffer);\n> >\n> > -\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n> > +\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n> >  \t\tStream *stream = streamConfiguration->stream();\n> >\n> >  \t\trequest->addBuffer(stream, buffer);\n> > @@ -1482,7 +1487,7 @@ void CameraDevice::requestComplete(Request *request)\n> >  \t\t\tcontinue;\n> >  \t\t}\n> >\n> > -\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n> > +\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n> >  \t\tStream *stream = streamConfiguration->stream();\n> >  \t\tFrameBuffer *buffer = request->findBuffer(stream);\n> >  \t\tif (!buffer) {\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index dc0ee664d443..f8f237203ce9 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -28,20 +28,24 @@\n> >  class CameraMetadata;\n> >\n> >  struct CameraStream {\n>\n> Can you turn it into a class while at it ?\n>\n\nSee the end of the series  :)\n\n> Also clearly not a candidate for this patch, do you think it would be\n> useful to rename the HAL-related classes with a HAL prefix (e.g.\n> HALCamera instead of CameraDevice, HALStream instead of CameraStream) to\n> avoid confusing them with the libcamera classes ? We're familiar with\n> this code so it's probably not too confusing for us, but it may be for a\n> newcomer.\n\nIndeed, I'm looking forward to get to a point where we can break\ncamera_device.cpp in multiple files, and possible re-name them as\nwell.\n\nI tried using the camera3 prefix for all Android-things to keep them\nseparate from our ones, but indeed we have confusing names for some\nclasses.\n\n>\n> > -\tCameraStream(libcamera::PixelFormat, libcamera::Size);\n> > +public:\n> > +\tCameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n>\n> i is not a very clear name, index would be better. I would also name the\n> existing parameters (that could be a separate change).\n>\n\nI think this happens later all in one go\n\n> >  \t~CameraStream();\n> >\n> > -\t/*\n> > -\t * The index of the libcamera StreamConfiguration as added during\n> > -\t * configureStreams(). A single libcamera Stream may be used to deliver\n> > -\t * one or more streams to the Android framework.\n> > -\t */\n> > -\tunsigned int index;\n> > +\tunsigned int index() const { return index_; }\n> >\n> >  \tlibcamera::PixelFormat format;\n> >  \tlibcamera::Size size;\n> >\n> >  \tEncoder *jpeg;\n> > +\n> > +private:\n> > +\t/*\n> > +\t * The index of the libcamera StreamConfiguration as added during\n> > +\t * configureStreams(). A single libcamera Stream may be used to deliver\n> > +\t * one or more streams to the Android framework.\n> > +\t */\n>\n> Should this be moved to the .cpp file ? Documentation rules are a bit\n> more relaxed here though, as it's internal doc.\n>\n\nwell, having them in the cpp file makes most sense for the purpose of\ngenerating documentation, which doesn't happen for the HAL. This\ncomment alone, not anchored to the field declaration loses a bit of\nvalue.\n\nI thinks going forward we want to document more formally this part\nthough.\n\n> > +\tunsigned int index_;\n> >  };\n> >\n> >  class CameraDevice : protected libcamera::Loggable\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 387DDBDB1D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  7 Sep 2020 08:49:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC60562B82;\n\tMon,  7 Sep 2020 10:49:47 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8BB2B629B6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  7 Sep 2020 10:49:46 +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 relay12.mail.gandi.net (Postfix) with ESMTPSA id 954E4200018;\n\tMon,  7 Sep 2020 08:49:45 +0000 (UTC)"],"Date":"Mon, 7 Sep 2020 10:53:33 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20200907085333.wly5lcay7hzc6bfq@uno.localdomain>","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-11-jacopo@jmondi.org>\n\t<20200905220524.GM6319@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200905220524.GM6319@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 10/12] android: camera_device:\n\tRework CameraStream handling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12358,"web_url":"https://patchwork.libcamera.org/comment/12358/","msgid":"<20200907132806.GE6047@pendragon.ideasonboard.com>","date":"2020-09-07T13:28:06","subject":"Re: [libcamera-devel] [PATCH v2 10/12] android: camera_device:\n\tRework CameraStream handling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Sep 07, 2020 at 10:53:33AM +0200, Jacopo Mondi wrote:\n> On Sun, Sep 06, 2020 at 01:05:24AM +0300, Laurent Pinchart wrote:\n> > On Wed, Sep 02, 2020 at 05:22:34PM +0200, Jacopo Mondi wrote:\n> > > The CameraDevice::streams_ vector of CameraStream instances is\n> > > currently mostly accessed by index. The current implementation\n> > > creates all the CameraStream during the first loop that inspects the\n> > > camera3_stream instances, and the update the index of the\n> > > StreamConfiguration associated with the CameraStream during a second\n> > > loop that inspects MJPEG streams. A third loop creates the JPEG encoder\n> > > associated with CameraStreams that produce MJPEG format.\n> > >\n> > > As the index-based association is hard to follow and rather fragile,\n> > > rework the creation and handling of CameraStream:\n> > >\n> > > 1) Make the StreamConfiguration index a constructor parameter and a\n> > >    private struct member.  This disallow the creation of CameraStream\n> > >    without a StreamConfiguration index assigned.\n> > >\n> > > 2) Create CameraStream only after the associated StreamConfiguration\n> > >    has been identified. The first loop creates CameraStream for non-JPEG\n> > >    streams, the second for the JPEG ones after having identified the\n> > >    associated StreamConfiguration. Since we have just created the\n> > >    CameraStream, create the JPEG encoder at the same time instead of\n> > >    deferring it.\n> > >\n> > > This change removes all accesses by index to the CameraDevice::streams_\n> > > vector.\n> > >\n> > > No functional changes intended, but this change aims to make the code\n> > > easier to follow and more robust.\n> > >\n> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > ---\n> > >  src/android/camera_device.cpp | 75 +++++++++++++++++++----------------\n> > >  src/android/camera_device.h   | 18 +++++----\n> > >  2 files changed, 51 insertions(+), 42 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 3630e87e8814..9bcd1d993c17 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -168,8 +168,8 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > >  \t}\n> > >  }\n> > >\n> > > -CameraStream::CameraStream(PixelFormat f, Size s)\n> > > -\t: index(-1), format(f), size(s), jpeg(nullptr)\n> > > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)\n> > > +\t: format(f), size(s), jpeg(nullptr), index_(i)\n> > >  {\n> > >  }\n> > >\n> > > @@ -1212,30 +1212,28 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > >  \t\tif (!format.isValid())\n> > >  \t\t\treturn -EINVAL;\n> > >\n> > > -\t\tstreams_.emplace_back(format, size);\n> > > -\t\tstream->priv = static_cast<void *>(&streams_[i]);\n> > > -\n> > >  \t\t/* Defer handling of MJPEG streams until all others are known. */\n> > >  \t\tif (stream->format == HAL_PIXEL_FORMAT_BLOB)\n> > >  \t\t\tcontinue;\n> > >\n> > >  \t\tStreamConfiguration streamConfiguration;\n> > > -\n> > >  \t\tstreamConfiguration.size = size;\n> > >  \t\tstreamConfiguration.pixelFormat = format;\n> > >\n> > > -\t\tstreams_[i].index = config_->addConfiguration(streamConfiguration);\n> > > +\t\tunsigned int index = config_->addConfiguration(streamConfiguration);\n> > > +\t\tCameraStream &cameraStream = streams_.emplace_back(format, size, index);\n> > > +\t\tstream->priv = static_cast<void *>(&cameraStream);\n> >\n> > This is problematic. std::vector::emplace_back may cause a reallocation,\n> \n> We have reserved space for all the possible cameraStream instances\n> before entering this loop, didn't we ? It should save all relocations,\n> or don't you think it's enough ?\n\nYou're right, I had forgotten about that.\n\n> > which invalidates all iterators, rendering the pointer invalid. One\n> > option would be to turn streams_ into an std::list, another option to\n> > fill the priv pointers in a separate loop after populating streams_.\n> >\n> > >  \t}\n> > >\n> > >  \t/* Now handle MJPEG streams, adding a new stream if required. */\n> > >  \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> > >  \t\tcamera3_stream_t *stream = stream_list->streams[i];\n> > > -\t\tbool match = false;\n> > > -\n> > >  \t\tif (stream->format != HAL_PIXEL_FORMAT_BLOB)\n> > >  \t\t\tcontinue;\n> >\n> > Drive-by comment for a possible future simplification, can't we rely on\n> > the fact that only a single JPEG stream will be requested ? If so, we\n> > could store a pointer to the JPEG camera3_stream_configuration_t in the\n> > previous loop, and have a\n> >\n> > \tif (jpegStream) {\n> > \t\t...\n> > \t}\n> >\n> > here instead of a loop.\n> \n> Ah yes, we can save this external loop indeed!\n> Thanks ;)\n> \n> > > -\t\t/* Search for a compatible stream */\n> > > +\t\t/* Search for a compatible stream in the non-JPEG ones. */\n> > > +\t\tbool match = false;\n> > > +\t\tunsigned int index;\n> > >  \t\tfor (unsigned int j = 0; j < config_->size(); j++) {\n> > >  \t\t\tStreamConfiguration &cfg = config_->at(j);\n> > >\n> > > @@ -1243,13 +1241,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > >  \t\t\t * \\todo The PixelFormat must also be compatible with\n> > >  \t\t\t * the encoder.\n> > >  \t\t\t */\n> > > -\t\t\tif (cfg.size == streams_[i].size) {\n> > > -\t\t\t\tLOG(HAL, Info) << \"Stream \" << i\n> > > -\t\t\t\t\t       << \" using libcamera stream \" << j;\n> > > +\t\t\tif (cfg.size.width != stream->width ||\n> > > +\t\t\t    cfg.size.height != stream->height)\n> > > +\t\t\t\tcontinue;\n> > >\n> > > -\t\t\t\tmatch = true;\n> > > -\t\t\t\tstreams_[i].index = j;\n> > > -\t\t\t}\n> > > +\t\t\tLOG(HAL, Info) << \"Stream \" << i\n> >\n> > s/Stream/Android stream/ ?\n> >\n> > > +\t\t\t\t       << \" using libcamera stream \" << j;\n> > > +\n> > > +\t\t\tindex = j;\n> > > +\t\t\tmatch = true;\n> >\n> > Should you break ?\n> \n> Ups, I should indeed\n> \n> > >  \t\t}\n> > >\n> > >  \t\t/*\n> > > @@ -1272,7 +1272,26 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > >  \t\t\tLOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n> > >  \t\t\t\t       << \" for MJPEG support\";\n> > >\n> > > -\t\t\tstreams_[i].index = config_->addConfiguration(streamConfiguration);\n> > > +\t\t\tindex = config_->addConfiguration(streamConfiguration);\n> > > +\t\t}\n> > > +\n> > > +\t\tStreamConfiguration &cfg = config_->at(index);\n> > > +\t\tPixelFormat format = formats::MJPEG;\n> > > +\t\tSize size = cfg.size;\n> > > +\n> > > +\t\tCameraStream &cameraStream = streams_.emplace_back(format, size, index);\n> >\n> > No need for the format and size local variables, you can write\n> >\n> > \t\tCameraStream &cameraStream =\n> > \t\t\tstreams_.emplace_back(formats::MJPEG, cfg.size, index);\n> \n> Kieran had a similar comment iirc. I'll change this!\n> \n> > > +\t\tstream->priv = static_cast<void *>(&cameraStream);\n> > > +\n> > > +\t\t/*\n> > > +\t\t * Construct a software encoder for MJPEG streams from the\n> > > +\t\t * chosen libcamera source stream.\n> > > +\t\t */\n> > > +\t\tcameraStream.jpeg = new EncoderLibJpeg();\n> > > +\t\tint ret = cameraStream.jpeg->configure(cfg);\n> > > +\t\tif (ret) {\n> > > +\t\t\tLOG(HAL, Error)\n> > > +\t\t\t\t<< \"Failed to configure encoder\";\n> >\n> > This holds on a single line.\n> >\n> > \t\t\tLOG(HAL, Error) << \"Failed to configure encoder\";\n> \n> Right!\n> \n> > > +\t\t\treturn ret;\n> > >  \t\t}\n> > >  \t}\n> > >\n> > > @@ -1295,25 +1314,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > >\n> > >  \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> > >  \t\tcamera3_stream_t *stream = stream_list->streams[i];\n> > > -\t\tCameraStream *cameraStream = &streams_[i];\n> > > -\t\tStreamConfiguration &cfg = config_->at(cameraStream->index);\n> > > +\t\tCameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);\n> > > +\t\tStreamConfiguration &cfg = config_->at(cameraStream->index());\n> > >\n> > >  \t\t/* Use the bufferCount confirmed by the validation process. */\n> > >  \t\tstream->max_buffers = cfg.bufferCount;\n> > > -\n> > > -\t\t/*\n> > > -\t\t * Construct a software encoder for MJPEG streams from the\n> > > -\t\t * chosen libcamera source stream.\n> > > -\t\t */\n> > > -\t\tif (cameraStream->format == formats::MJPEG) {\n> > > -\t\t\tcameraStream->jpeg = new EncoderLibJpeg();\n> > > -\t\t\tint ret = cameraStream->jpeg->configure(cfg);\n> > > -\t\t\tif (ret) {\n> > > -\t\t\t\tLOG(HAL, Error)\n> > > -\t\t\t\t\t<< \"Failed to configure encoder\";\n> > > -\t\t\t\treturn ret;\n> > > -\t\t\t}\n> > > -\t\t}\n> > >  \t}\n> > >\n> > >  \t/*\n> > > @@ -1427,7 +1432,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >  \t\t}\n> > >  \t\tdescriptor->frameBuffers.emplace_back(buffer);\n> > >\n> > > -\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n> > > +\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n> > >  \t\tStream *stream = streamConfiguration->stream();\n> > >\n> > >  \t\trequest->addBuffer(stream, buffer);\n> > > @@ -1482,7 +1487,7 @@ void CameraDevice::requestComplete(Request *request)\n> > >  \t\t\tcontinue;\n> > >  \t\t}\n> > >\n> > > -\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n> > > +\t\tStreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n> > >  \t\tStream *stream = streamConfiguration->stream();\n> > >  \t\tFrameBuffer *buffer = request->findBuffer(stream);\n> > >  \t\tif (!buffer) {\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index dc0ee664d443..f8f237203ce9 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -28,20 +28,24 @@\n> > >  class CameraMetadata;\n> > >\n> > >  struct CameraStream {\n> >\n> > Can you turn it into a class while at it ?\n> \n> See the end of the series  :)\n> \n> > Also clearly not a candidate for this patch, do you think it would be\n> > useful to rename the HAL-related classes with a HAL prefix (e.g.\n> > HALCamera instead of CameraDevice, HALStream instead of CameraStream) to\n> > avoid confusing them with the libcamera classes ? We're familiar with\n> > this code so it's probably not too confusing for us, but it may be for a\n> > newcomer.\n> \n> Indeed, I'm looking forward to get to a point where we can break\n> camera_device.cpp in multiple files, and possible re-name them as\n> well.\n> \n> I tried using the camera3 prefix for all Android-things to keep them\n> separate from our ones, but indeed we have confusing names for some\n> classes.\n> \n> > > -\tCameraStream(libcamera::PixelFormat, libcamera::Size);\n> > > +public:\n> > > +\tCameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> >\n> > i is not a very clear name, index would be better. I would also name the\n> > existing parameters (that could be a separate change).\n> \n> I think this happens later all in one go\n> \n> > >  \t~CameraStream();\n> > >\n> > > -\t/*\n> > > -\t * The index of the libcamera StreamConfiguration as added during\n> > > -\t * configureStreams(). A single libcamera Stream may be used to deliver\n> > > -\t * one or more streams to the Android framework.\n> > > -\t */\n> > > -\tunsigned int index;\n> > > +\tunsigned int index() const { return index_; }\n> > >\n> > >  \tlibcamera::PixelFormat format;\n> > >  \tlibcamera::Size size;\n> > >\n> > >  \tEncoder *jpeg;\n> > > +\n> > > +private:\n> > > +\t/*\n> > > +\t * The index of the libcamera StreamConfiguration as added during\n> > > +\t * configureStreams(). A single libcamera Stream may be used to deliver\n> > > +\t * one or more streams to the Android framework.\n> > > +\t */\n> >\n> > Should this be moved to the .cpp file ? Documentation rules are a bit\n> > more relaxed here though, as it's internal doc.\n> \n> well, having them in the cpp file makes most sense for the purpose of\n> generating documentation, which doesn't happen for the HAL. This\n> comment alone, not anchored to the field declaration loses a bit of\n> value.\n> \n> I thinks going forward we want to document more formally this part\n> though.\n> \n> > > +\tunsigned int index_;\n> > >  };\n> > >\n> > >  class CameraDevice : protected libcamera::Loggable","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7F12FBDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  7 Sep 2020 13:28:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A70562B82;\n\tMon,  7 Sep 2020 15:28:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2C2F66036E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  7 Sep 2020 15:28:33 +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 48BFC3E;\n\tMon,  7 Sep 2020 15:28:31 +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=\"mm/aE6XG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599485311;\n\tbh=9zr3lTzlB8r5mFhnEcH3sIrrRptjkMNBqpIEWatL4LY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mm/aE6XGb35K1iCvaLI6s54hL2hu9xAuOS0I5c/h87JX5lkNMwUL4FcwdKQE5/MFI\n\tHDb9RxbZXh9FxXeC7N6G6Hv5UryIZtRVMTO1i+dTB0Ux44s9zp+K/6uw9JXqB7yaw7\n\t35a6jbPbpSbREk4beYAMZZA76/JDSAyNlX135WEs=","Date":"Mon, 7 Sep 2020 16:28:06 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20200907132806.GE6047@pendragon.ideasonboard.com>","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-11-jacopo@jmondi.org>\n\t<20200905220524.GM6319@pendragon.ideasonboard.com>\n\t<20200907085333.wly5lcay7hzc6bfq@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200907085333.wly5lcay7hzc6bfq@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 10/12] android: camera_device:\n\tRework CameraStream handling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12363,"web_url":"https://patchwork.libcamera.org/comment/12363/","msgid":"<CAO5uPHMw-KGAPzQ80UkxXUHgs96qHKEarDaggrQEV1wFEv2k+g@mail.gmail.com>","date":"2020-09-08T05:17:46","subject":"Re: [libcamera-devel] [PATCH v2 10/12] android: camera_device:\n\tRework CameraStream handling","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 10:28 PM Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi Jacopo,\n>\n> On Mon, Sep 07, 2020 at 10:53:33AM +0200, Jacopo Mondi wrote:\n> > On Sun, Sep 06, 2020 at 01:05:24AM +0300, Laurent Pinchart wrote:\n> > > On Wed, Sep 02, 2020 at 05:22:34PM +0200, Jacopo Mondi wrote:\n> > > > The CameraDevice::streams_ vector of CameraStream instances is\n> > > > currently mostly accessed by index. The current implementation\n> > > > creates all the CameraStream during the first loop that inspects the\n> > > > camera3_stream instances, and the update the index of the\n> > > > StreamConfiguration associated with the CameraStream during a second\n> > > > loop that inspects MJPEG streams. A third loop creates the JPEG encoder\n> > > > associated with CameraStreams that produce MJPEG format.\n> > > >\n> > > > As the index-based association is hard to follow and rather fragile,\n> > > > rework the creation and handling of CameraStream:\n> > > >\n> > > > 1) Make the StreamConfiguration index a constructor parameter and a\n> > > >    private struct member.  This disallow the creation of CameraStream\n> > > >    without a StreamConfiguration index assigned.\n> > > >\n> > > > 2) Create CameraStream only after the associated StreamConfiguration\n> > > >    has been identified. The first loop creates CameraStream for non-JPEG\n> > > >    streams, the second for the JPEG ones after having identified the\n> > > >    associated StreamConfiguration. Since we have just created the\n> > > >    CameraStream, create the JPEG encoder at the same time instead of\n> > > >    deferring it.\n> > > >\n> > > > This change removes all accesses by index to the CameraDevice::streams_\n> > > > vector.\n> > > >\n> > > > No functional changes intended, but this change aims to make the code\n> > > > easier to follow and more robust.\n> > > >\n> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > > ---\n> > > >  src/android/camera_device.cpp | 75 +++++++++++++++++++----------------\n> > > >  src/android/camera_device.h   | 18 +++++----\n> > > >  2 files changed, 51 insertions(+), 42 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index 3630e87e8814..9bcd1d993c17 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -168,8 +168,8 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > > >   }\n> > > >  }\n> > > >\n> > > > -CameraStream::CameraStream(PixelFormat f, Size s)\n> > > > - : index(-1), format(f), size(s), jpeg(nullptr)\n> > > > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)\n> > > > + : format(f), size(s), jpeg(nullptr), index_(i)\n> > > >  {\n> > > >  }\n> > > >\n> > > > @@ -1212,30 +1212,28 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > >           if (!format.isValid())\n> > > >                   return -EINVAL;\n> > > >\n> > > > -         streams_.emplace_back(format, size);\n> > > > -         stream->priv = static_cast<void *>(&streams_[i]);\n> > > > -\n> > > >           /* Defer handling of MJPEG streams until all others are known. */\n> > > >           if (stream->format == HAL_PIXEL_FORMAT_BLOB)\n> > > >                   continue;\n> > > >\n> > > >           StreamConfiguration streamConfiguration;\n> > > > -\n> > > >           streamConfiguration.size = size;\n> > > >           streamConfiguration.pixelFormat = format;\n> > > >\n> > > > -         streams_[i].index = config_->addConfiguration(streamConfiguration);\n> > > > +         unsigned int index = config_->addConfiguration(streamConfiguration);\n> > > > +         CameraStream &cameraStream = streams_.emplace_back(format, size, index);\n> > > > +         stream->priv = static_cast<void *>(&cameraStream);\n> > >\n> > > This is problematic. std::vector::emplace_back may cause a reallocation,\n> >\n> > We have reserved space for all the possible cameraStream instances\n> > before entering this loop, didn't we ? It should save all relocations,\n> > or don't you think it's enough ?\n>\n> You're right, I had forgotten about that.\n>\n\nHuge nit: [This is my preference, please feel free to ignore]\nI would write\nstreams_.push_back(format, size, index);\nstream->priv = static_cast<void*>(&streams_.back());\n\n> > > which invalidates all iterators, rendering the pointer invalid. One\n> > > option would be to turn streams_ into an std::list, another option to\n> > > fill the priv pointers in a separate loop after populating streams_.\n> > >\n> > > >   }\n> > > >\n> > > >   /* Now handle MJPEG streams, adding a new stream if required. */\n> > > >   for (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> > > >           camera3_stream_t *stream = stream_list->streams[i];\n> > > > -         bool match = false;\n> > > > -\n> > > >           if (stream->format != HAL_PIXEL_FORMAT_BLOB)\n> > > >                   continue;\n> > >\n> > > Drive-by comment for a possible future simplification, can't we rely on\n> > > the fact that only a single JPEG stream will be requested ? If so, we\n> > > could store a pointer to the JPEG camera3_stream_configuration_t in the\n> > > previous loop, and have a\n> > >\n> > >     if (jpegStream) {\n> > >             ...\n> > >     }\n> > >\n> > > here instead of a loop.\n> >\n> > Ah yes, we can save this external loop indeed!\n> > Thanks ;)\n> >\n> > > > -         /* Search for a compatible stream */\n> > > > +         /* Search for a compatible stream in the non-JPEG ones. */\n> > > > +         bool match = false;\n> > > > +         unsigned int index;\n> > > >           for (unsigned int j = 0; j < config_->size(); j++) {\n> > > >                   StreamConfiguration &cfg = config_->at(j);\n> > > >\n> > > > @@ -1243,13 +1241,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > >                    * \\todo The PixelFormat must also be compatible with\n> > > >                    * the encoder.\n> > > >                    */\n> > > > -                 if (cfg.size == streams_[i].size) {\n> > > > -                         LOG(HAL, Info) << \"Stream \" << i\n> > > > -                                        << \" using libcamera stream \" << j;\n> > > > +                 if (cfg.size.width != stream->width ||\n> > > > +                     cfg.size.height != stream->height)\n> > > > +                         continue;\n> > > >\n> > > > -                         match = true;\n> > > > -                         streams_[i].index = j;\n> > > > -                 }\n> > > > +                 LOG(HAL, Info) << \"Stream \" << i\n> > >\n> > > s/Stream/Android stream/ ?\n> > >\n> > > > +                                << \" using libcamera stream \" << j;\n> > > > +\n> > > > +                 index = j;\n> > > > +                 match = true;\n> > >\n> > > Should you break ?\n> >\n> > Ups, I should indeed\n> >\n\nNit: I would save match by setting index to -1 initially, or use std::optional.\n\n> > > >           }\n> > > >\n> > > >           /*\n> > > > @@ -1272,7 +1272,26 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > >                   LOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n> > > >                                  << \" for MJPEG support\";\n> > > >\n> > > > -                 streams_[i].index = config_->addConfiguration(streamConfiguration);\n> > > > +                 index = config_->addConfiguration(streamConfiguration);\n> > > > +         }\n> > > > +\n> > > > +         StreamConfiguration &cfg = config_->at(index);\n> > > > +         PixelFormat format = formats::MJPEG;\n> > > > +         Size size = cfg.size;\n> > > > +\n> > > > +         CameraStream &cameraStream = streams_.emplace_back(format, size, index);\n> > >\n> > > No need for the format and size local variables, you can write\n> > >\n> > >             CameraStream &cameraStream =\n> > >                     streams_.emplace_back(formats::MJPEG, cfg.size, index);\n> >\n> > Kieran had a similar comment iirc. I'll change this!\n> >\n> > > > +         stream->priv = static_cast<void *>(&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> > > > +         if (ret) {\n> > > > +                 LOG(HAL, Error)\n> > > > +                         << \"Failed to configure encoder\";\n> > >\n> > > This holds on a single line.\n> > >\n> > >                     LOG(HAL, Error) << \"Failed to configure encoder\";\n> >\n> > Right!\n> >\n> > > > +                 return ret;\n> > > >           }\n> > > >   }\n> > > >\n> > > > @@ -1295,25 +1314,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > >\n> > > >   for (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> > > >           camera3_stream_t *stream = stream_list->streams[i];\n> > > > -         CameraStream *cameraStream = &streams_[i];\n> > > > -         StreamConfiguration &cfg = config_->at(cameraStream->index);\n> > > > +         CameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);\n> > > > +         StreamConfiguration &cfg = config_->at(cameraStream->index());\n> > > >\n> > > >           /* Use the bufferCount confirmed by the validation process. */\n> > > >           stream->max_buffers = cfg.bufferCount;\n> > > > -\n> > > > -         /*\n> > > > -          * Construct a software encoder for MJPEG streams from the\n> > > > -          * chosen libcamera source stream.\n> > > > -          */\n> > > > -         if (cameraStream->format == formats::MJPEG) {\n> > > > -                 cameraStream->jpeg = new EncoderLibJpeg();\n> > > > -                 int ret = cameraStream->jpeg->configure(cfg);\n> > > > -                 if (ret) {\n> > > > -                         LOG(HAL, Error)\n> > > > -                                 << \"Failed to configure encoder\";\n> > > > -                         return ret;\n> > > > -                 }\n> > > > -         }\n> > > >   }\n> > > >\n> > > >   /*\n> > > > @@ -1427,7 +1432,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > >           }\n> > > >           descriptor->frameBuffers.emplace_back(buffer);\n> > > >\n> > > > -         StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n> > > > +         StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n> > > >           Stream *stream = streamConfiguration->stream();\n> > > >\n> > > >           request->addBuffer(stream, buffer);\n> > > > @@ -1482,7 +1487,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > >                   continue;\n> > > >           }\n> > > >\n> > > > -         StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);\n> > > > +         StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());\n> > > >           Stream *stream = streamConfiguration->stream();\n> > > >           FrameBuffer *buffer = request->findBuffer(stream);\n> > > >           if (!buffer) {\n> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > index dc0ee664d443..f8f237203ce9 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -28,20 +28,24 @@\n> > > >  class CameraMetadata;\n> > > >\n> > > >  struct CameraStream {\n> > >\n> > > Can you turn it into a class while at it ?\n> >\n> > See the end of the series  :)\n> >\n> > > Also clearly not a candidate for this patch, do you think it would be\n> > > useful to rename the HAL-related classes with a HAL prefix (e.g.\n> > > HALCamera instead of CameraDevice, HALStream instead of CameraStream) to\n> > > avoid confusing them with the libcamera classes ? We're familiar with\n> > > this code so it's probably not too confusing for us, but it may be for a\n> > > newcomer.\n> >\n> > Indeed, I'm looking forward to get to a point where we can break\n> > camera_device.cpp in multiple files, and possible re-name them as\n> > well.\n> >\n> > I tried using the camera3 prefix for all Android-things to keep them\n> > separate from our ones, but indeed we have confusing names for some\n> > classes.\n> >\n> > > > - CameraStream(libcamera::PixelFormat, libcamera::Size);\n> > > > +public:\n> > > > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);\n> > >\n> > > i is not a very clear name, index would be better. I would also name the\n> > > existing parameters (that could be a separate change).\n> >\n> > I think this happens later all in one go\n> >\n> > > >   ~CameraStream();\n> > > >\n> > > > - /*\n> > > > -  * The index of the libcamera StreamConfiguration as added during\n> > > > -  * configureStreams(). A single libcamera Stream may be used to deliver\n> > > > -  * one or more streams to the Android framework.\n> > > > -  */\n> > > > - unsigned int index;\n> > > > + unsigned int index() const { return index_; }\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> > > > +  * configureStreams(). A single libcamera Stream may be used to deliver\n> > > > +  * one or more streams to the Android framework.\n> > > > +  */\n> > >\n> > > Should this be moved to the .cpp file ? Documentation rules are a bit\n> > > more relaxed here though, as it's internal doc.\n> >\n> > well, having them in the cpp file makes most sense for the purpose of\n> > generating documentation, which doesn't happen for the HAL. This\n> > comment alone, not anchored to the field declaration loses a bit of\n> > value.\n> >\n> > I thinks going forward we want to document more formally this part\n> > though.\n> >\n> > > > + unsigned int index_;\n> > > >  };\n> > > >\n> > > >  class CameraDevice : protected libcamera::Loggable\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":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 009DBBDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  8 Sep 2020 05:17:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 811AF62BAA;\n\tTue,  8 Sep 2020 07:17:59 +0200 (CEST)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CA397603EE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  8 Sep 2020 07:17:57 +0200 (CEST)","by mail-ed1-x534.google.com with SMTP id w1so14667120edr.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 07 Sep 2020 22:17:57 -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=\"BQMlinfI\"; 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=zK/apYT/B26xURmlZ+o7htv94T0bzcAaUCmk/bJZ7XM=;\n\tb=BQMlinfIvlVVrkM3UToc2/ZEiAFoXkIPzip4lED1HReb9E6EVj3E+K3JX2p7SAE34T\n\t3Jzjyww/LwMUKkOS9WeC+QdApIkZmX5IXUOpwCe4ramizj69al0c1bCVq++ziVm4sL3f\n\tyoLlwePWnT5TMiw5Jycog+jVscSiFtpKVygw0=","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=zK/apYT/B26xURmlZ+o7htv94T0bzcAaUCmk/bJZ7XM=;\n\tb=Rr23X18B0YyFV8FMh4RwLEbtKcwnfqizh4VrpGiHIA/fWO9vBdmh33vmxE7/Tv9wDI\n\tXN3Kib/xdzGL/BZ1eCqnu0wfMJUyvq22PzvGwBi0Dv5kJUr3pNs0MkfDcNKcsZdU0gTM\n\t4M4ZZC6gc2j5ueK5cJN6Nb/kGdJmBhvytv0GRNqtl2ADJJPTL4yWUtqozj8I8wiAMiUl\n\tRx0MOvy+IZWJMxAsMG248O1pKhRUw/ShiWjohjFVF3sjvfe0pNL1NCJ7Sq1392FimR2y\n\t+NykuwdBWUPt2N9k8YisLtdnWpMD5Uvi3841h8g76zkG+TgMtmocq5+eyO/uvRM35ymp\n\t/4yQ==","X-Gm-Message-State":"AOAM531h/5JZqt01ZDVANTSm/HtdWxh+cfps0Jl4w4XK7KSOIn/+Chlw\n\t6JKcnj59zqQPmRDQg8MysCFYWmbXyTCW1pNj66EMzg==","X-Google-Smtp-Source":"ABdhPJwP+JYfU8n7nLbswY3tUcR34EdbTAxK6PBupmo2T9ZrfwAJQTGLaHz/Udx5jVQwaYS44vFo/+UukL1lGP3n8jI=","X-Received":"by 2002:aa7:c38a:: with SMTP id\n\tk10mr25814129edq.325.1599542277317; \n\tMon, 07 Sep 2020 22:17:57 -0700 (PDT)","MIME-Version":"1.0","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-11-jacopo@jmondi.org>\n\t<20200905220524.GM6319@pendragon.ideasonboard.com>\n\t<20200907085333.wly5lcay7hzc6bfq@uno.localdomain>\n\t<20200907132806.GE6047@pendragon.ideasonboard.com>","In-Reply-To":"<20200907132806.GE6047@pendragon.ideasonboard.com>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Tue, 8 Sep 2020 14:17:46 +0900","Message-ID":"<CAO5uPHMw-KGAPzQ80UkxXUHgs96qHKEarDaggrQEV1wFEv2k+g@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 10/12] android: camera_device:\n\tRework CameraStream handling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"Tomasz Figa <tfiga@google.com>, libcamera-devel@lists.libcamera.org,\n\tHirokazu Honda <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\" <libcamera-devel-bounces@lists.libcamera.org>"}}]