[{"id":620,"web_url":"https://patchwork.libcamera.org/comment/620/","msgid":"<20190126090942.GB4596@pendragon.ideasonboard.com>","date":"2019-01-26T09:09:42","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: stream: add basic\n\tStreamConfiguration class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, Jan 25, 2019 at 04:33:35PM +0100, Niklas Söderlund wrote:\n> Add a simple StreamConfiguration class to hold configuration data for a\n> single stream of a Camera. In its current form not many configuration\n> parameters are supported but it's expected the number of options will\n> grow over time.\n> \n> At this stage the pixel format is represented as a unsigned int to allow\n\ns/as a/as an/\n\n> for a easy mapping to the V4L2 API. This might be subject to change in\n\ns/a easy/an easy/ (or just s/a easy/easy/)\n\n> the future as we finalize how libcamera shall represent pixelformats.\n\ns/pixelformats/pixel formats/\n\n> A StreamConfiguration objected needs to be created from the Stream\n> object it should configure. As the two objects are so closely related I\n> have at this stage opted to implement them in the same stream.{h,cpp} as\n> the Stream class.\n\nWhy does the StreamConfiguration object need to be created with the\nStream passed as an argument to the constructor ?\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/stream.h | 20 ++++++++++++\n> src/libcamera/stream.cpp | 64 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 84 insertions(+)\n> \n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index 415815ba12c65e47..8750797c36dd9b42 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -20,6 +20,26 @@ private:\n> \tunsigned int id_;\n> };\n> \n> +class StreamConfiguration final\n> +{\n> +public:\n> +\tStreamConfiguration(class Stream &stream);\n\ns/class //\n\n> +\n> +\tunsigned int id() const { return id_; };\n\nGiven that the stream is given to the constructor, should we store the\nstream reference instead of the id ?\n\n> +\tunsigned int width() const { return width_; };\n> +\tunsigned int height() const { return height_; };\n> +\tunsigned int pixelformat() const { return pixelformat_; };\n\nOpen question, pixelformat or pixelFormat ?\n\n> +\n> +\tvoid setDimension(unsigned int width, unsigned int height);\n\nI'd name this setSize(). I think we'll need a class to represent sizes\nand rectangles at some point, but that can be done later.\n\n> +\tvoid setPixelFormat(unsigned int pixelformat);\n> +\n> +private:\n> +\tunsigned int id_;\n> +\tunsigned int width_;\n> +\tunsigned int height_;\n> +\tunsigned int pixelformat_;\n\nWith accessors for width_, height_ and pixelformat_ that expose the\nfields directly, how about just making those three fields public ? The\nStreamConfiguration is more of a data structure that groups stream\nparameters without much associated processing than a real object with\nmethods.\n\n> +};\n> +\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_STREAM_H__ */\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index 3b44e834ee02b35a..e40756260c5768f3 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -63,4 +63,68 @@ Stream::Stream(unsigned int id)\n> * \\return The stream ID\n> */\n> \n> +/**\n> + * \\class StreamConfiguration\n> + * \\brief Stream configuration object\n\nMaybe \"Configuration parameters for a stream\" ?\n\n> + *\n> + * The StreamConfiguration class is a model of all information which can be\n> + * configured for a single video stream. A application should acquire a all\n> + * Stream object from a camera, select the ones it wish to use, create a\n> + * StreamConfiguration for each of those streams, set the desired parameter on\n> + * the objects and feed them back to the camera object to configure the camera.\n\nI think we should just describe here the StreamConfiguration class, and\nexplain how it interacts with Stream and Camera in the Camera\ndocumentation.\n\n> + */\n> +\n> +/**\n> + * \\fn StreamConfiguration::id()\n> + * \\brief Retrieve the streams ID\n\ns/streams/stream/\n\n> + * \\return The stream ID\n> + */\n> +\n> +/**\n> + * \\fn StreamConfiguration::width()\n> + * \\brief Retrieve the Stream width\n\ns/Stream/stream/\n\nWe should describe what the stream width (and height and pixel format\nbelow) is.\n\n> + * \\return The stream width\n> + */\n> +\n> +/**\n> + * \\fn StreamConfiguration::height()\n> + * \\brief Retrieve the Stream height\n\nSame here.\n\n> + * \\return The stream height\n> + */\n> +\n> +/**\n> + * \\fn StreamConfiguration::pixelformat()\n> + * \\brief Retrieve the Stream pixelformat\n\nAnd here too.\n\n> + * \\return The stream pixelformat\n> + */\n> +\n> +/**\n> + * \\brief Set desired width and height for the stream\n> + * \\param[in] width The desired width of the stream\n> + * \\param[in] height The desired height of the stream\n> + */\n> +void StreamConfiguration::setDimension(unsigned int width, unsigned int height)\n> +{\n> +\twidth_ = width;\n> +\theight_ = height;\n> +}\n> +\n> +/**\n> + * \\brief Set desired pixelformat for the stream\n> + * \\param[in] pixelformat The desired pixelformat of the stream\n> + */\n> +void StreamConfiguration::setPixelFormat(unsigned int pixelformat)\n> +{\n> +\tpixelformat_ = pixelformat;\n> +}\n> +\n> +/**\n> + * \\brief Create a new configuration container for a stream\n> + * \\param[in] stream The stream object the configuration object should act on\n> + */\n> +StreamConfiguration::StreamConfiguration(class Stream &stream)\n> +\t: id_(stream.id()), width_(0), height_(0), pixelformat_(0)\n> +{\n> +}\n\nPlease order functions as in the class definition.\n\n> +\n> } /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F9DF60C65\n\tfor ;\n\tSat, 26 Jan 2019 10:09:59 +0100 (CET)","from pendragon.ideasonboard.com (unknown [62.119.166.9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5BC5423D;\n\tSat, 26 Jan 2019 10:09:54 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548493799;\n\tbh=e6des2mnkFEwmM9mrMbPWUYxK0xlJqXcOzp5S6X8MS8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=piwK3Kx6Ko7lPrHqvDcIRPBdSZVv+yEgZAsENnUc3IDmDcRy9OQrOPab1xb9pV7sz\n\tHZvZ5Fw4p5CVG8KhRUF/WHt8as+CuCZnNCpWmCxLO6PIJhq90jQj6i+rlGPC1rpPms\n\tedGi+8YSdUVtYuxK2FOWLEiK8lzawu3Z5NMol0uU=","Date":"Sat, 26 Jan 2019 11:09:42 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190126090942.GB4596@pendragon.ideasonboard.com>","References":"<20190125153340.2744-1-niklas.soderlund@ragnatech.se>\n\t<20190125153340.2744-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190125153340.2744-3-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: stream: add basic\n\tStreamConfiguration class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 26 Jan 2019 09:09:59 -0000"}}]