[{"id":18501,"web_url":"https://patchwork.libcamera.org/comment/18501/","msgid":"<20210803061845.GX2167@pyrite.rasen.tech>","date":"2021-08-03T06:18:45","subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Fri, Jul 30, 2021 at 04:03:00AM +0300, Laurent Pinchart wrote:\n> The FrameSink class serves as a base to implement components that\n> consume frames. This allows handling frame sinks in a generic way,\n> independent of their nature. The BufferWrite class will be ported to\n> FrameSink in a second step.\n> \n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Niklas Söderlund \n\nReviewed-by: Paul Elder \n\n> ---\n> src/cam/frame_sink.cpp | 31 +++++++++++++++++++++++++++++++\n> src/cam/frame_sink.h | 34 ++++++++++++++++++++++++++++++++++\n> src/cam/meson.build | 1 +\n> 3 files changed, 66 insertions(+)\n> create mode 100644 src/cam/frame_sink.cpp\n> create mode 100644 src/cam/frame_sink.h\n> \n> diff --git a/src/cam/frame_sink.cpp b/src/cam/frame_sink.cpp\n> new file mode 100644\n> index 000000000000..6e15c1007f12\n> --- /dev/null\n> +++ b/src/cam/frame_sink.cpp\n> @@ -0,0 +1,31 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Ideas on Board Oy\n> + *\n> + * frame_sink.cpp - Base Frame Sink Class\n> + */\n> +\n> +#include \"frame_sink.h\"\n> +\n> +FrameSink::~FrameSink()\n> +{\n> +}\n> +\n> +int FrameSink::configure([[maybe_unused]] const libcamera::CameraConfiguration &config)\n> +{\n> +\treturn 0;\n> +}\n> +\n> +void FrameSink::mapBuffer([[maybe_unused]] libcamera::FrameBuffer *buffer)\n> +{\n> +}\n> +\n> +int FrameSink::start()\n> +{\n> +\treturn 0;\n> +}\n> +\n> +int FrameSink::stop()\n> +{\n> +\treturn 0;\n> +}\n> diff --git a/src/cam/frame_sink.h b/src/cam/frame_sink.h\n> new file mode 100644\n> index 000000000000..116e5267bebe\n> --- /dev/null\n> +++ b/src/cam/frame_sink.h\n> @@ -0,0 +1,34 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Ideas on Board Oy\n> + *\n> + * frame_sink.h - Base Frame Sink Class\n> + */\n> +#ifndef __CAM_FRAME_SINK_H__\n> +#define __CAM_FRAME_SINK_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +class CameraConfiguration;\n> +class FrameBuffer;\n> +class Request;\n> +} /* namespace libcamera */\n> +\n> +class FrameSink\n> +{\n> +public:\n> +\tvirtual ~FrameSink();\n> +\n> +\tvirtual int configure(const libcamera::CameraConfiguration &config);\n> +\n> +\tvirtual void mapBuffer(libcamera::FrameBuffer *buffer);\n> +\n> +\tvirtual int start();\n> +\tvirtual int stop();\n> +\n> +\tvirtual bool consumeRequest(libcamera::Request *request) = 0;\n> +\tlibcamera::Signal requestReleased;\n> +};\n> +\n> +#endif /* __CAM_FRAME_SINK_H__ */\n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index 1e90ee521f74..649cc990d867 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -13,6 +13,7 @@ cam_sources = files([\n> 'buffer_writer.cpp',\n> 'camera_session.cpp',\n> 'event_loop.cpp',\n> + 'frame_sink.cpp',\n> 'main.cpp',\n> 'options.cpp',\n> 'stream_options.cpp',\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","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 38FC2C3232\n\tfor ;\n\tTue, 3 Aug 2021 06:18:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E45B3687CB;\n\tTue, 3 Aug 2021 08:18:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6246960269\n\tfor ;\n\tTue, 3 Aug 2021 08:18:53 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 057564A3;\n\tTue, 3 Aug 2021 08:18:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SoxkYKa2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627971533;\n\tbh=++DUp/t3RdG66G5wNTMaL++gJubDYWeyKtb8+XFRwwM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SoxkYKa2E7c6OGio21f+P5VxCMPD/+2AXZ4RONxZmGEW5/Zczx/1bEX4ooDWGoKbl\n\tJyzP1mqJYgZMZqbDo7LG3Zs7otNUdgkvX9YPB0J6eC24Alkk9sBBr1BDyxHcXenOPl\n\tGqhuoJ32/sczHfmN1a3X6rajXamb2gX8KUOgXc1E=","Date":"Tue, 3 Aug 2021 15:18:45 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210803061845.GX2167@pyrite.rasen.tech>","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-3-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210730010306.19956-3-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18512,"web_url":"https://patchwork.libcamera.org/comment/18512/","msgid":"","date":"2021-08-03T10:46:55","subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 30/07/2021 02:03, Laurent Pinchart wrote:\n> The FrameSink class serves as a base to implement components that\n> consume frames. This allows handling frame sinks in a generic way,\n> independent of their nature. The BufferWrite class will be ported to\n> FrameSink in a second step.\n> \n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Niklas Söderlund \n> ---\n> src/cam/frame_sink.cpp | 31 +++++++++++++++++++++++++++++++\n> src/cam/frame_sink.h | 34 ++++++++++++++++++++++++++++++++++\n> src/cam/meson.build | 1 +\n> 3 files changed, 66 insertions(+)\n> create mode 100644 src/cam/frame_sink.cpp\n> create mode 100644 src/cam/frame_sink.h\n> \n> diff --git a/src/cam/frame_sink.cpp b/src/cam/frame_sink.cpp\n> new file mode 100644\n> index 000000000000..6e15c1007f12\n> --- /dev/null\n> +++ b/src/cam/frame_sink.cpp\n> @@ -0,0 +1,31 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Ideas on Board Oy\n\n2021 now?\n\n> + *\n> + * frame_sink.cpp - Base Frame Sink Class\n> + */\n> +\n> +#include \"frame_sink.h\"\n> +\n> +FrameSink::~FrameSink()\n> +{\n> +}\n> +\n> +int FrameSink::configure([[maybe_unused]] const libcamera::CameraConfiguration &config)\n\nI wonder if a FrameSink should be configured based on a\nStreamConfiguration rather than a CameraConfiguration ...\n\nPresumably each FrameSink represents a single stream - not the full\ncamera outputs...\n\nBut lets see - maybe that's handled differently later.\n\nPerhaps tentatively:\n\nReviewed-by: Kieran Bingham \n\n> +{\n> +\treturn 0;\n> +}\n> +\n> +void FrameSink::mapBuffer([[maybe_unused]] libcamera::FrameBuffer *buffer)\n> +{\n> +}\n> +\n> +int FrameSink::start()\n> +{\n> +\treturn 0;\n> +}\n> +\n> +int FrameSink::stop()\n> +{\n> +\treturn 0;\n> +}\n> diff --git a/src/cam/frame_sink.h b/src/cam/frame_sink.h\n> new file mode 100644\n> index 000000000000..116e5267bebe\n> --- /dev/null\n> +++ b/src/cam/frame_sink.h\n> @@ -0,0 +1,34 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Ideas on Board Oy\n\nSame,\n\n> + *\n> + * frame_sink.h - Base Frame Sink Class\n> + */\n> +#ifndef __CAM_FRAME_SINK_H__\n> +#define __CAM_FRAME_SINK_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +class CameraConfiguration;\n> +class FrameBuffer;\n> +class Request;\n> +} /* namespace libcamera */\n> +\n> +class FrameSink\n> +{\n> +public:\n> +\tvirtual ~FrameSink();\n> +\n> +\tvirtual int configure(const libcamera::CameraConfiguration &config);\n> +\n> +\tvirtual void mapBuffer(libcamera::FrameBuffer *buffer);\n> +\n> +\tvirtual int start();\n> +\tvirtual int stop();\n> +\n> +\tvirtual bool consumeRequest(libcamera::Request *request) = 0;\n> +\tlibcamera::Signal requestReleased;\n> +};\n> +\n> +#endif /* __CAM_FRAME_SINK_H__ */\n> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> index 1e90ee521f74..649cc990d867 100644\n> --- a/src/cam/meson.build\n> +++ b/src/cam/meson.build\n> @@ -13,6 +13,7 @@ cam_sources = files([\n> 'buffer_writer.cpp',\n> 'camera_session.cpp',\n> 'event_loop.cpp',\n> + 'frame_sink.cpp',\n> 'main.cpp',\n> 'options.cpp',\n> 'stream_options.cpp',\n>","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 5A095C3235\n\tfor ;\n\tTue, 3 Aug 2021 10:47:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD2AE687CC;\n\tTue, 3 Aug 2021 12:46:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C2436026A\n\tfor ;\n\tTue, 3 Aug 2021 12:46:58 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 10FCD3F0;\n\tTue, 3 Aug 2021 12:46:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eSmL18aL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627987618;\n\tbh=gQqDBlYNXt6/bsZqMcGNXL3gJD1lnn12zt39OQFk2HE=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=eSmL18aLmfiAMWlK76DXwL/S6YNiigT8tarD8+mLkSD/jNQOvImWxmGPQNDZv19lg\n\tbmVl3OcvuF8EHG+ApXieNHwcdb7x5V1Ub/ZzSQmIXvrsQp+S9sCg7vt4Fnr+4mUzvJ\n\tOjsHeFc7pPQUNmwPg8AgCqm29gzvogGiuJnhDuJc=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-3-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"","Date":"Tue, 3 Aug 2021 11:46:55 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210730010306.19956-3-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18513,"web_url":"https://patchwork.libcamera.org/comment/18513/","msgid":"","date":"2021-08-03T12:44:34","subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Aug 03, 2021 at 11:46:55AM +0100, Kieran Bingham wrote:\n> On 30/07/2021 02:03, Laurent Pinchart wrote:\n> > The FrameSink class serves as a base to implement components that\n> > consume frames. This allows handling frame sinks in a generic way,\n> > independent of their nature. The BufferWrite class will be ported to\n> > FrameSink in a second step.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Niklas Söderlund \n> > ---\n> > src/cam/frame_sink.cpp | 31 +++++++++++++++++++++++++++++++\n> > src/cam/frame_sink.h | 34 ++++++++++++++++++++++++++++++++++\n> > src/cam/meson.build | 1 +\n> > 3 files changed, 66 insertions(+)\n> > create mode 100644 src/cam/frame_sink.cpp\n> > create mode 100644 src/cam/frame_sink.h\n> > \n> > diff --git a/src/cam/frame_sink.cpp b/src/cam/frame_sink.cpp\n> > new file mode 100644\n> > index 000000000000..6e15c1007f12\n> > --- /dev/null\n> > +++ b/src/cam/frame_sink.cpp\n> > @@ -0,0 +1,31 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Ideas on Board Oy\n> \n> 2021 now?\n> \n> > + *\n> > + * frame_sink.cpp - Base Frame Sink Class\n> > + */\n> > +\n> > +#include \"frame_sink.h\"\n> > +\n> > +FrameSink::~FrameSink()\n> > +{\n> > +}\n> > +\n> > +int FrameSink::configure([[maybe_unused]] const libcamera::CameraConfiguration &config)\n> \n> I wonder if a FrameSink should be configured based on a\n> StreamConfiguration rather than a CameraConfiguration ...\n> \n> Presumably each FrameSink represents a single stream - not the full\n> camera outputs...\n\nFrameSink models a sink for a complete request, not for a single buffer.\nIt could be interesting to change that, for instance to write some\nstreams to disk and display other streams on the screen (not entirely\nsure about the use cases though). However, even in that case, we'll have\nto bundle multiple streams together somehow, as the KMS sink would need\nto update all planes at once, not sequentially and in isolation. I'd\nconsider this a topic for future research :-)\n\n> But lets see - maybe that's handled differently later.\n> \n> Perhaps tentatively:\n> \n> Reviewed-by: Kieran Bingham \n> \n> > +{\n> > +\treturn 0;\n> > +}\n> > +\n> > +void FrameSink::mapBuffer([[maybe_unused]] libcamera::FrameBuffer *buffer)\n> > +{\n> > +}\n> > +\n> > +int FrameSink::start()\n> > +{\n> > +\treturn 0;\n> > +}\n> > +\n> > +int FrameSink::stop()\n> > +{\n> > +\treturn 0;\n> > +}\n> > diff --git a/src/cam/frame_sink.h b/src/cam/frame_sink.h\n> > new file mode 100644\n> > index 000000000000..116e5267bebe\n> > --- /dev/null\n> > +++ b/src/cam/frame_sink.h\n> > @@ -0,0 +1,34 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Ideas on Board Oy\n> \n> Same,\n> \n> > + *\n> > + * frame_sink.h - Base Frame Sink Class\n> > + */\n> > +#ifndef __CAM_FRAME_SINK_H__\n> > +#define __CAM_FRAME_SINK_H__\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +class CameraConfiguration;\n> > +class FrameBuffer;\n> > +class Request;\n> > +} /* namespace libcamera */\n> > +\n> > +class FrameSink\n> > +{\n> > +public:\n> > +\tvirtual ~FrameSink();\n> > +\n> > +\tvirtual int configure(const libcamera::CameraConfiguration &config);\n> > +\n> > +\tvirtual void mapBuffer(libcamera::FrameBuffer *buffer);\n> > +\n> > +\tvirtual int start();\n> > +\tvirtual int stop();\n> > +\n> > +\tvirtual bool consumeRequest(libcamera::Request *request) = 0;\n> > +\tlibcamera::Signal requestReleased;\n> > +};\n> > +\n> > +#endif /* __CAM_FRAME_SINK_H__ */\n> > diff --git a/src/cam/meson.build b/src/cam/meson.build\n> > index 1e90ee521f74..649cc990d867 100644\n> > --- a/src/cam/meson.build\n> > +++ b/src/cam/meson.build\n> > @@ -13,6 +13,7 @@ cam_sources = files([\n> > 'buffer_writer.cpp',\n> > 'camera_session.cpp',\n> > 'event_loop.cpp',\n> > + 'frame_sink.cpp',\n> > 'main.cpp',\n> > 'options.cpp',\n> > 'stream_options.cpp',","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 5BFC4C3235\n\tfor ;\n\tTue, 3 Aug 2021 12:44:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B224C68536;\n\tTue, 3 Aug 2021 14:44:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B67666026A\n\tfor ;\n\tTue, 3 Aug 2021 14:44:47 +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 2FC033F0;\n\tTue, 3 Aug 2021 14:44:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"k0C+FUnh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627994686;\n\tbh=QvPbyFNcNN4jKjaukq+w26sn9YHLPGyUzIsM7fES1RI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=k0C+FUnhhG5VbREU+XCRlfI+P+OssoaDDf8p5pCGqoARx0qEivvpw6X+jQORaDOhr\n\tP/Sqd9DDLS16c5yT3jnRcFLjNav/TpAflV3xOGPmZdfNNoCnv1vViRnC8I5tgpDhQL\n\tvE0WueFOmFSY9nnOke+QGqK3J7Cqu7WGyX9x2K4Q=","Date":"Tue, 3 Aug 2021 15:44:34 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-3-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18514,"web_url":"https://patchwork.libcamera.org/comment/18514/","msgid":"<32adf448-11e8-2f49-f08e-b2019215e9b5@ideasonboard.com>","date":"2021-08-03T12:55:38","subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 03/08/2021 13:44, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Tue, Aug 03, 2021 at 11:46:55AM +0100, Kieran Bingham wrote:\n>> On 30/07/2021 02:03, Laurent Pinchart wrote:\n>>> The FrameSink class serves as a base to implement components that\n>>> consume frames. This allows handling frame sinks in a generic way,\n>>> independent of their nature. The BufferWrite class will be ported to\n>>> FrameSink in a second step.\n>>>\n>>> Signed-off-by: Laurent Pinchart \n>>> Reviewed-by: Niklas Söderlund \n>>> ---\n>>> src/cam/frame_sink.cpp | 31 +++++++++++++++++++++++++++++++\n>>> src/cam/frame_sink.h | 34 ++++++++++++++++++++++++++++++++++\n>>> src/cam/meson.build | 1 +\n>>> 3 files changed, 66 insertions(+)\n>>> create mode 100644 src/cam/frame_sink.cpp\n>>> create mode 100644 src/cam/frame_sink.h\n>>>\n>>> diff --git a/src/cam/frame_sink.cpp b/src/cam/frame_sink.cpp\n>>> new file mode 100644\n>>> index 000000000000..6e15c1007f12\n>>> --- /dev/null\n>>> +++ b/src/cam/frame_sink.cpp\n>>> @@ -0,0 +1,31 @@\n>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>> +/*\n>>> + * Copyright (C) 2020, Ideas on Board Oy\n>>\n>> 2021 now?\n>>\n>>> + *\n>>> + * frame_sink.cpp - Base Frame Sink Class\n>>> + */\n>>> +\n>>> +#include \"frame_sink.h\"\n>>> +\n>>> +FrameSink::~FrameSink()\n>>> +{\n>>> +}\n>>> +\n>>> +int FrameSink::configure([[maybe_unused]] const libcamera::CameraConfiguration &config)\n>>\n>> I wonder if a FrameSink should be configured based on a\n>> StreamConfiguration rather than a CameraConfiguration ...\n>>\n>> Presumably each FrameSink represents a single stream - not the full\n>> camera outputs...\n> \n> FrameSink models a sink for a complete request, not for a single buffer.\n\nThen should it be a RequestSink ... not a FrameSink?\n\nTo me a FrameSink - sinks a single frame ... that's why I would have\nexpected this to be modelled around a single stream.\n\nI would have thought that one FrameSink would be created per stream, and\nwhen a request completes, if there is a sink configured for the streams\nit has - it can process the Frames through the FrameSink ...?\n\n> It could be interesting to change that, for instance to write some\n> streams to disk and display other streams on the screen (not entirely\n> sure about the use cases though). However, even in that case, we'll have\n> to bundle multiple streams together somehow, as the KMS sink would need\n> to update all planes at once, not sequentially and in isolation. I'd\n> consider this a topic for future research :-)\n\nafterall we're just re-implementing gstreamer anyway ;-)\n\n\n>> But lets see - maybe that's handled differently later.\n>>\n>> Perhaps tentatively:\n>>\n>> Reviewed-by: Kieran Bingham \n>>\n>>> +{\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> +void FrameSink::mapBuffer([[maybe_unused]] libcamera::FrameBuffer *buffer)\n>>> +{\n>>> +}\n>>> +\n>>> +int FrameSink::start()\n>>> +{\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> +int FrameSink::stop()\n>>> +{\n>>> +\treturn 0;\n>>> +}\n>>> diff --git a/src/cam/frame_sink.h b/src/cam/frame_sink.h\n>>> new file mode 100644\n>>> index 000000000000..116e5267bebe\n>>> --- /dev/null\n>>> +++ b/src/cam/frame_sink.h\n>>> @@ -0,0 +1,34 @@\n>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>> +/*\n>>> + * Copyright (C) 2020, Ideas on Board Oy\n>>\n>> Same,\n>>\n>>> + *\n>>> + * frame_sink.h - Base Frame Sink Class\n>>> + */\n>>> +#ifndef __CAM_FRAME_SINK_H__\n>>> +#define __CAM_FRAME_SINK_H__\n>>> +\n>>> +#include \n>>> +\n>>> +namespace libcamera {\n>>> +class CameraConfiguration;\n>>> +class FrameBuffer;\n>>> +class Request;\n>>> +} /* namespace libcamera */\n>>> +\n>>> +class FrameSink\n>>> +{\n>>> +public:\n>>> +\tvirtual ~FrameSink();\n>>> +\n>>> +\tvirtual int configure(const libcamera::CameraConfiguration &config);\n>>> +\n>>> +\tvirtual void mapBuffer(libcamera::FrameBuffer *buffer);\n>>> +\n>>> +\tvirtual int start();\n>>> +\tvirtual int stop();\n>>> +\n>>> +\tvirtual bool consumeRequest(libcamera::Request *request) = 0;\n>>> +\tlibcamera::Signal requestReleased;\n>>> +};\n>>> +\n>>> +#endif /* __CAM_FRAME_SINK_H__ */\n>>> diff --git a/src/cam/meson.build b/src/cam/meson.build\n>>> index 1e90ee521f74..649cc990d867 100644\n>>> --- a/src/cam/meson.build\n>>> +++ b/src/cam/meson.build\n>>> @@ -13,6 +13,7 @@ cam_sources = files([\n>>> 'buffer_writer.cpp',\n>>> 'camera_session.cpp',\n>>> 'event_loop.cpp',\n>>> + 'frame_sink.cpp',\n>>> 'main.cpp',\n>>> 'options.cpp',\n>>> 'stream_options.cpp',\n>","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 5ED09C3235\n\tfor ;\n\tTue, 3 Aug 2021 12:55:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D1944687CC;\n\tTue, 3 Aug 2021 14:55:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DF02E6026A\n\tfor ;\n\tTue, 3 Aug 2021 14:55:41 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 702DC3F0;\n\tTue, 3 Aug 2021 14:55:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Q6ynVQcW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627995341;\n\tbh=W9gfcGDk4rLIAOFapNBAgfLXUngFrbhVYinMnIdtEAQ=;\n\th=From:Subject:To:Cc:References:Date:In-Reply-To:From;\n\tb=Q6ynVQcWIMquQWwb0YhMrVqK/p6sb6I4TObbZQrmOsN56fc/OP7bhxI6mSCAej38F\n\tb3vs3vzxGhjKe3wF+8Cpp6qHiAV/7QOZcHF4dyJfnlW83z2XibSha9DfLHZz+poBN+\n\tIETex55rcn3BZdCw+Nsm0Yf+xtagqgj/4XWWpWQM=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-3-laurent.pinchart@ideasonboard.com>\n\t\n\t","Message-ID":"<32adf448-11e8-2f49-f08e-b2019215e9b5@ideasonboard.com>","Date":"Tue, 3 Aug 2021 13:55:38 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18515,"web_url":"https://patchwork.libcamera.org/comment/18515/","msgid":"","date":"2021-08-03T12:59:10","subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Aug 03, 2021 at 01:55:38PM +0100, Kieran Bingham wrote:\n> On 03/08/2021 13:44, Laurent Pinchart wrote:\n> > On Tue, Aug 03, 2021 at 11:46:55AM +0100, Kieran Bingham wrote:\n> >> On 30/07/2021 02:03, Laurent Pinchart wrote:\n> >>> The FrameSink class serves as a base to implement components that\n> >>> consume frames. This allows handling frame sinks in a generic way,\n> >>> independent of their nature. The BufferWrite class will be ported to\n> >>> FrameSink in a second step.\n> >>>\n> >>> Signed-off-by: Laurent Pinchart \n> >>> Reviewed-by: Niklas Söderlund \n> >>> ---\n> >>> src/cam/frame_sink.cpp | 31 +++++++++++++++++++++++++++++++\n> >>> src/cam/frame_sink.h | 34 ++++++++++++++++++++++++++++++++++\n> >>> src/cam/meson.build | 1 +\n> >>> 3 files changed, 66 insertions(+)\n> >>> create mode 100644 src/cam/frame_sink.cpp\n> >>> create mode 100644 src/cam/frame_sink.h\n> >>>\n> >>> diff --git a/src/cam/frame_sink.cpp b/src/cam/frame_sink.cpp\n> >>> new file mode 100644\n> >>> index 000000000000..6e15c1007f12\n> >>> --- /dev/null\n> >>> +++ b/src/cam/frame_sink.cpp\n> >>> @@ -0,0 +1,31 @@\n> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2020, Ideas on Board Oy\n> >>\n> >> 2021 now?\n> >>\n> >>> + *\n> >>> + * frame_sink.cpp - Base Frame Sink Class\n> >>> + */\n> >>> +\n> >>> +#include \"frame_sink.h\"\n> >>> +\n> >>> +FrameSink::~FrameSink()\n> >>> +{\n> >>> +}\n> >>> +\n> >>> +int FrameSink::configure([[maybe_unused]] const libcamera::CameraConfiguration &config)\n> >>\n> >> I wonder if a FrameSink should be configured based on a\n> >> StreamConfiguration rather than a CameraConfiguration ...\n> >>\n> >> Presumably each FrameSink represents a single stream - not the full\n> >> camera outputs...\n> > \n> > FrameSink models a sink for a complete request, not for a single buffer.\n> \n> Then should it be a RequestSink ... not a FrameSink?\n> \n> To me a FrameSink - sinks a single frame ... that's why I would have\n> expected this to be modelled around a single stream.\n> \n> I would have thought that one FrameSink would be created per stream, and\n> when a request completes, if there is a sink configured for the streams\n> it has - it can process the Frames through the FrameSink ...?\n\nI'm not sure exactly how it would be architectured, there will be\ndetails to handle, those are usually annoying. I hope it's not a blocker\n:-)\n\nRequestSink sounds a bit weird as a name, even if I've thought about it\nas well when replying to your previous e-mails. Maybe, for the meantime,\nwe could think about FrameSink as an abstract concept, like a dishwasher\ndoesn't have to be called a disheswasher to wash more than one dish ?\n:-)\n\n> > It could be interesting to change that, for instance to write some\n> > streams to disk and display other streams on the screen (not entirely\n> > sure about the use cases though). However, even in that case, we'll have\n> > to bundle multiple streams together somehow, as the KMS sink would need\n> > to update all planes at once, not sequentially and in isolation. I'd\n> > consider this a topic for future research :-)\n> \n> afterall we're just re-implementing gstreamer anyway ;-)\n\nMy thoughts as well :-)\n\n> >> But lets see - maybe that's handled differently later.\n> >>\n> >> Perhaps tentatively:\n> >>\n> >> Reviewed-by: Kieran Bingham \n> >>\n> >>> +{\n> >>> +\treturn 0;\n> >>> +}\n> >>> +\n> >>> +void FrameSink::mapBuffer([[maybe_unused]] libcamera::FrameBuffer *buffer)\n> >>> +{\n> >>> +}\n> >>> +\n> >>> +int FrameSink::start()\n> >>> +{\n> >>> +\treturn 0;\n> >>> +}\n> >>> +\n> >>> +int FrameSink::stop()\n> >>> +{\n> >>> +\treturn 0;\n> >>> +}\n> >>> diff --git a/src/cam/frame_sink.h b/src/cam/frame_sink.h\n> >>> new file mode 100644\n> >>> index 000000000000..116e5267bebe\n> >>> --- /dev/null\n> >>> +++ b/src/cam/frame_sink.h\n> >>> @@ -0,0 +1,34 @@\n> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2020, Ideas on Board Oy\n> >>\n> >> Same,\n> >>\n> >>> + *\n> >>> + * frame_sink.h - Base Frame Sink Class\n> >>> + */\n> >>> +#ifndef __CAM_FRAME_SINK_H__\n> >>> +#define __CAM_FRAME_SINK_H__\n> >>> +\n> >>> +#include \n> >>> +\n> >>> +namespace libcamera {\n> >>> +class CameraConfiguration;\n> >>> +class FrameBuffer;\n> >>> +class Request;\n> >>> +} /* namespace libcamera */\n> >>> +\n> >>> +class FrameSink\n> >>> +{\n> >>> +public:\n> >>> +\tvirtual ~FrameSink();\n> >>> +\n> >>> +\tvirtual int configure(const libcamera::CameraConfiguration &config);\n> >>> +\n> >>> +\tvirtual void mapBuffer(libcamera::FrameBuffer *buffer);\n> >>> +\n> >>> +\tvirtual int start();\n> >>> +\tvirtual int stop();\n> >>> +\n> >>> +\tvirtual bool consumeRequest(libcamera::Request *request) = 0;\n> >>> +\tlibcamera::Signal requestReleased;\n> >>> +};\n> >>> +\n> >>> +#endif /* __CAM_FRAME_SINK_H__ */\n> >>> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> >>> index 1e90ee521f74..649cc990d867 100644\n> >>> --- a/src/cam/meson.build\n> >>> +++ b/src/cam/meson.build\n> >>> @@ -13,6 +13,7 @@ cam_sources = files([\n> >>> 'buffer_writer.cpp',\n> >>> 'camera_session.cpp',\n> >>> 'event_loop.cpp',\n> >>> + 'frame_sink.cpp',\n> >>> 'main.cpp',\n> >>> 'options.cpp',\n> >>> 'stream_options.cpp',\n> >","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 9E92BC3232\n\tfor ;\n\tTue, 3 Aug 2021 12:59:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 759CD68536;\n\tTue, 3 Aug 2021 14:59:24 +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 A8F476026A\n\tfor ;\n\tTue, 3 Aug 2021 14:59:22 +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 1C31F3F0;\n\tTue, 3 Aug 2021 14:59:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WlrTEQlL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627995562;\n\tbh=CJrkiOo75xtZ7WezL9IeX3SaRwiuUdQ+zymvqSnfO5E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WlrTEQlLhd+ZkfWeIQ4VBo5TkDf1KW9kaXcBlroRRocWu5Htuw18ZGtSURewwbLdE\n\tU1ktqe1zJSQJgAomVXbFNq3L3LZOLxVyT1SII9Au+5FFg29mzEgXB9CydWF+dHwGGC\n\t7vWwQaU/XNfgKcaPfot9fKs8AUpx/rIU2fkUNxB4=","Date":"Tue, 3 Aug 2021 15:59:10 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-3-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t<32adf448-11e8-2f49-f08e-b2019215e9b5@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<32adf448-11e8-2f49-f08e-b2019215e9b5@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18537,"web_url":"https://patchwork.libcamera.org/comment/18537/","msgid":"<6e3268e3-534f-2181-9ba1-32fc13e4a774@ideasonboard.com>","date":"2021-08-04T07:50:09","subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 8/3/21 6:29 PM, Laurent Pinchart wrote:\n> Hi Kieran,\n>\n> On Tue, Aug 03, 2021 at 01:55:38PM +0100, Kieran Bingham wrote:\n>> On 03/08/2021 13:44, Laurent Pinchart wrote:\n>>> On Tue, Aug 03, 2021 at 11:46:55AM +0100, Kieran Bingham wrote:\n>>>> On 30/07/2021 02:03, Laurent Pinchart wrote:\n>>>>> The FrameSink class serves as a base to implement components that\n>>>>> consume frames. This allows handling frame sinks in a generic way,\n>>>>> independent of their nature. The BufferWrite class will be ported to\n>>>>> FrameSink in a second step.\n>>>>>\n>>>>> Signed-off-by: Laurent Pinchart \n>>>>> Reviewed-by: Niklas Söderlund \n>>>>> ---\n>>>>> src/cam/frame_sink.cpp | 31 +++++++++++++++++++++++++++++++\n>>>>> src/cam/frame_sink.h | 34 ++++++++++++++++++++++++++++++++++\n>>>>> src/cam/meson.build | 1 +\n>>>>> 3 files changed, 66 insertions(+)\n>>>>> create mode 100644 src/cam/frame_sink.cpp\n>>>>> create mode 100644 src/cam/frame_sink.h\n>>>>>\n>>>>> diff --git a/src/cam/frame_sink.cpp b/src/cam/frame_sink.cpp\n>>>>> new file mode 100644\n>>>>> index 000000000000..6e15c1007f12\n>>>>> --- /dev/null\n>>>>> +++ b/src/cam/frame_sink.cpp\n>>>>> @@ -0,0 +1,31 @@\n>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>>> +/*\n>>>>> + * Copyright (C) 2020, Ideas on Board Oy\n>>>> 2021 now?\n>>>>\n>>>>> + *\n>>>>> + * frame_sink.cpp - Base Frame Sink Class\n>>>>> + */\n>>>>> +\n>>>>> +#include \"frame_sink.h\"\n>>>>> +\n>>>>> +FrameSink::~FrameSink()\n>>>>> +{\n>>>>> +}\n>>>>> +\n>>>>> +int FrameSink::configure([[maybe_unused]] const libcamera::CameraConfiguration &config)\n>>>> I wonder if a FrameSink should be configured based on a\n>>>> StreamConfiguration rather than a CameraConfiguration ...\n>>>>\n>>>> Presumably each FrameSink represents a single stream - not the full\n>>>> camera outputs...\n>>> FrameSink models a sink for a complete request, not for a single buffer.\n>> Then should it be a RequestSink ... not a FrameSink?\n>>\n>> To me a FrameSink - sinks a single frame ... that's why I would have\n>> expected this to be modelled around a single stream.\n>>\n>> I would have thought that one FrameSink would be created per stream, and\n>> when a request completes, if there is a sink configured for the streams\n>> it has - it can process the Frames through the FrameSink ...?\n> I'm not sure exactly how it would be architectured, there will be\n> details to handle, those are usually annoying. I hope it's not a blocker\n> :-)\n>\n> RequestSink sounds a bit weird as a name, even if I've thought about it\n> as well when replying to your previous e-mails. Maybe, for the meantime,\n> we could think about FrameSink as an abstract concept, like a dishwasher\n> doesn't have to be called a disheswasher to wash more than one dish ?\n> :-)\n\nUsually there is one / two line description of the class (especially \nparent classes) at the top of the file, after the licensing. It's \n\"nice-to-have\" else one can dig into git log though, to know about the \nspecifics. Since as you say, this is an \"abstract\" concept (maybe an \nabstract class as well?), I got reminded that these the brief \ndescriptions of the class are useful :)\n\nAnyways,\n\nReviewed-by: Umang Jain \n\n>\n>>> It could be interesting to change that, for instance to write some\n>>> streams to disk and display other streams on the screen (not entirely\n>>> sure about the use cases though). However, even in that case, we'll have\n>>> to bundle multiple streams together somehow, as the KMS sink would need\n>>> to update all planes at once, not sequentially and in isolation. I'd\n>>> consider this a topic for future research :-)\n>> afterall we're just re-implementing gstreamer anyway ;-)\n> My thoughts as well :-)\n>\n>>>> But lets see - maybe that's handled differently later.\n>>>>\n>>>> Perhaps tentatively:\n>>>>\n>>>> Reviewed-by: Kieran Bingham \n>>>>\n>>>>> +{\n>>>>> +\treturn 0;\n>>>>> +}\n>>>>> +\n>>>>> +void FrameSink::mapBuffer([[maybe_unused]] libcamera::FrameBuffer *buffer)\n>>>>> +{\n>>>>> +}\n>>>>> +\n>>>>> +int FrameSink::start()\n>>>>> +{\n>>>>> +\treturn 0;\n>>>>> +}\n>>>>> +\n>>>>> +int FrameSink::stop()\n>>>>> +{\n>>>>> +\treturn 0;\n>>>>> +}\n>>>>> diff --git a/src/cam/frame_sink.h b/src/cam/frame_sink.h\n>>>>> new file mode 100644\n>>>>> index 000000000000..116e5267bebe\n>>>>> --- /dev/null\n>>>>> +++ b/src/cam/frame_sink.h\n>>>>> @@ -0,0 +1,34 @@\n>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>>> +/*\n>>>>> + * Copyright (C) 2020, Ideas on Board Oy\n>>>> Same,\n>>>>\n>>>>> + *\n>>>>> + * frame_sink.h - Base Frame Sink Class\n>>>>> + */\n>>>>> +#ifndef __CAM_FRAME_SINK_H__\n>>>>> +#define __CAM_FRAME_SINK_H__\n>>>>> +\n>>>>> +#include \n>>>>> +\n>>>>> +namespace libcamera {\n>>>>> +class CameraConfiguration;\n>>>>> +class FrameBuffer;\n>>>>> +class Request;\n>>>>> +} /* namespace libcamera */\n>>>>> +\n>>>>> +class FrameSink\n>>>>> +{\n>>>>> +public:\n>>>>> +\tvirtual ~FrameSink();\n>>>>> +\n>>>>> +\tvirtual int configure(const libcamera::CameraConfiguration &config);\n>>>>> +\n>>>>> +\tvirtual void mapBuffer(libcamera::FrameBuffer *buffer);\n>>>>> +\n>>>>> +\tvirtual int start();\n>>>>> +\tvirtual int stop();\n>>>>> +\n>>>>> +\tvirtual bool consumeRequest(libcamera::Request *request) = 0;\n>>>>> +\tlibcamera::Signal requestReleased;\n>>>>> +};\n>>>>> +\n>>>>> +#endif /* __CAM_FRAME_SINK_H__ */\n>>>>> diff --git a/src/cam/meson.build b/src/cam/meson.build\n>>>>> index 1e90ee521f74..649cc990d867 100644\n>>>>> --- a/src/cam/meson.build\n>>>>> +++ b/src/cam/meson.build\n>>>>> @@ -13,6 +13,7 @@ cam_sources = files([\n>>>>> 'buffer_writer.cpp',\n>>>>> 'camera_session.cpp',\n>>>>> 'event_loop.cpp',\n>>>>> + 'frame_sink.cpp',\n>>>>> 'main.cpp',\n>>>>> 'options.cpp',\n>>>>> 'stream_options.cpp',","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 E369AC3232\n\tfor ;\n\tWed, 4 Aug 2021 07:50:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E8BD6881B;\n\tWed, 4 Aug 2021 09:50:17 +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 DE73C6880F\n\tfor ;\n\tWed, 4 Aug 2021 09:50:15 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.40])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9A1A824F;\n\tWed, 4 Aug 2021 09:50:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ieUzMI/D\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628063415;\n\tbh=QqBf2v3uYthI4YiVMAvOzC18JPoA/d822GNKc1ZzohA=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=ieUzMI/DfnTfFyq+NDXnmcxIc8E+Pct4gl0Xkoq2weN7ofZnmIqlpKP3G5OgYs0Tt\n\tcyJYmLbHb6lp+oEXRYrU/cAoPleGbD1TEHXpL3E4eYLtSgv/zJzBxe8agZF5GCNBsz\n\t7pu4WPILyvy65shR0Ahv5eaQv2Z+ZCJ08nIGkeBA=","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-3-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t<32adf448-11e8-2f49-f08e-b2019215e9b5@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<6e3268e3-534f-2181-9ba1-32fc13e4a774@ideasonboard.com>","Date":"Wed, 4 Aug 2021 13:20:09 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18546,"web_url":"https://patchwork.libcamera.org/comment/18546/","msgid":"","date":"2021-08-04T09:02:48","subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Aug 04, 2021 at 01:20:09PM +0530, Umang Jain wrote:\n> On 8/3/21 6:29 PM, Laurent Pinchart wrote:\n> > On Tue, Aug 03, 2021 at 01:55:38PM +0100, Kieran Bingham wrote:\n> >> On 03/08/2021 13:44, Laurent Pinchart wrote:\n> >>> On Tue, Aug 03, 2021 at 11:46:55AM +0100, Kieran Bingham wrote:\n> >>>> On 30/07/2021 02:03, Laurent Pinchart wrote:\n> >>>>> The FrameSink class serves as a base to implement components that\n> >>>>> consume frames. This allows handling frame sinks in a generic way,\n> >>>>> independent of their nature. The BufferWrite class will be ported to\n> >>>>> FrameSink in a second step.\n> >>>>>\n> >>>>> Signed-off-by: Laurent Pinchart \n> >>>>> Reviewed-by: Niklas Söderlund \n> >>>>> ---\n> >>>>> src/cam/frame_sink.cpp | 31 +++++++++++++++++++++++++++++++\n> >>>>> src/cam/frame_sink.h | 34 ++++++++++++++++++++++++++++++++++\n> >>>>> src/cam/meson.build | 1 +\n> >>>>> 3 files changed, 66 insertions(+)\n> >>>>> create mode 100644 src/cam/frame_sink.cpp\n> >>>>> create mode 100644 src/cam/frame_sink.h\n> >>>>>\n> >>>>> diff --git a/src/cam/frame_sink.cpp b/src/cam/frame_sink.cpp\n> >>>>> new file mode 100644\n> >>>>> index 000000000000..6e15c1007f12\n> >>>>> --- /dev/null\n> >>>>> +++ b/src/cam/frame_sink.cpp\n> >>>>> @@ -0,0 +1,31 @@\n> >>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>>>> +/*\n> >>>>> + * Copyright (C) 2020, Ideas on Board Oy\n> >>>> 2021 now?\n> >>>>\n> >>>>> + *\n> >>>>> + * frame_sink.cpp - Base Frame Sink Class\n> >>>>> + */\n> >>>>> +\n> >>>>> +#include \"frame_sink.h\"\n> >>>>> +\n> >>>>> +FrameSink::~FrameSink()\n> >>>>> +{\n> >>>>> +}\n> >>>>> +\n> >>>>> +int FrameSink::configure([[maybe_unused]] const libcamera::CameraConfiguration &config)\n> >>>> I wonder if a FrameSink should be configured based on a\n> >>>> StreamConfiguration rather than a CameraConfiguration ...\n> >>>>\n> >>>> Presumably each FrameSink represents a single stream - not the full\n> >>>> camera outputs...\n> >>> FrameSink models a sink for a complete request, not for a single buffer.\n> >> Then should it be a RequestSink ... not a FrameSink?\n> >>\n> >> To me a FrameSink - sinks a single frame ... that's why I would have\n> >> expected this to be modelled around a single stream.\n> >>\n> >> I would have thought that one FrameSink would be created per stream, and\n> >> when a request completes, if there is a sink configured for the streams\n> >> it has - it can process the Frames through the FrameSink ...?\n> > I'm not sure exactly how it would be architectured, there will be\n> > details to handle, those are usually annoying. I hope it's not a blocker\n> > :-)\n> >\n> > RequestSink sounds a bit weird as a name, even if I've thought about it\n> > as well when replying to your previous e-mails. Maybe, for the meantime,\n> > we could think about FrameSink as an abstract concept, like a dishwasher\n> > doesn't have to be called a disheswasher to wash more than one dish ?\n> > :-)\n> \n> Usually there is one / two line description of the class (especially \n> parent classes) at the top of the file, after the licensing. It's \n> \"nice-to-have\" else one can dig into git log though, to know about the \n> specifics. Since as you say, this is an \"abstract\" concept (maybe an \n> abstract class as well?), I got reminded that these the brief \n> descriptions of the class are useful :)\n\nIt's a good point, I'll add that.\n\n> Anyways,\n> \n> Reviewed-by: Umang Jain \n> \n> >>> It could be interesting to change that, for instance to write some\n> >>> streams to disk and display other streams on the screen (not entirely\n> >>> sure about the use cases though). However, even in that case, we'll have\n> >>> to bundle multiple streams together somehow, as the KMS sink would need\n> >>> to update all planes at once, not sequentially and in isolation. I'd\n> >>> consider this a topic for future research :-)\n> >> afterall we're just re-implementing gstreamer anyway ;-)\n> > My thoughts as well :-)\n> >\n> >>>> But lets see - maybe that's handled differently later.\n> >>>>\n> >>>> Perhaps tentatively:\n> >>>>\n> >>>> Reviewed-by: Kieran Bingham \n> >>>>\n> >>>>> +{\n> >>>>> +\treturn 0;\n> >>>>> +}\n> >>>>> +\n> >>>>> +void FrameSink::mapBuffer([[maybe_unused]] libcamera::FrameBuffer *buffer)\n> >>>>> +{\n> >>>>> +}\n> >>>>> +\n> >>>>> +int FrameSink::start()\n> >>>>> +{\n> >>>>> +\treturn 0;\n> >>>>> +}\n> >>>>> +\n> >>>>> +int FrameSink::stop()\n> >>>>> +{\n> >>>>> +\treturn 0;\n> >>>>> +}\n> >>>>> diff --git a/src/cam/frame_sink.h b/src/cam/frame_sink.h\n> >>>>> new file mode 100644\n> >>>>> index 000000000000..116e5267bebe\n> >>>>> --- /dev/null\n> >>>>> +++ b/src/cam/frame_sink.h\n> >>>>> @@ -0,0 +1,34 @@\n> >>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>>>> +/*\n> >>>>> + * Copyright (C) 2020, Ideas on Board Oy\n> >>>> Same,\n> >>>>\n> >>>>> + *\n> >>>>> + * frame_sink.h - Base Frame Sink Class\n> >>>>> + */\n> >>>>> +#ifndef __CAM_FRAME_SINK_H__\n> >>>>> +#define __CAM_FRAME_SINK_H__\n> >>>>> +\n> >>>>> +#include \n> >>>>> +\n> >>>>> +namespace libcamera {\n> >>>>> +class CameraConfiguration;\n> >>>>> +class FrameBuffer;\n> >>>>> +class Request;\n> >>>>> +} /* namespace libcamera */\n> >>>>> +\n> >>>>> +class FrameSink\n> >>>>> +{\n> >>>>> +public:\n> >>>>> +\tvirtual ~FrameSink();\n> >>>>> +\n> >>>>> +\tvirtual int configure(const libcamera::CameraConfiguration &config);\n> >>>>> +\n> >>>>> +\tvirtual void mapBuffer(libcamera::FrameBuffer *buffer);\n> >>>>> +\n> >>>>> +\tvirtual int start();\n> >>>>> +\tvirtual int stop();\n> >>>>> +\n> >>>>> +\tvirtual bool consumeRequest(libcamera::Request *request) = 0;\n> >>>>> +\tlibcamera::Signal requestReleased;\n> >>>>> +};\n> >>>>> +\n> >>>>> +#endif /* __CAM_FRAME_SINK_H__ */\n> >>>>> diff --git a/src/cam/meson.build b/src/cam/meson.build\n> >>>>> index 1e90ee521f74..649cc990d867 100644\n> >>>>> --- a/src/cam/meson.build\n> >>>>> +++ b/src/cam/meson.build\n> >>>>> @@ -13,6 +13,7 @@ cam_sources = files([\n> >>>>> 'buffer_writer.cpp',\n> >>>>> 'camera_session.cpp',\n> >>>>> 'event_loop.cpp',\n> >>>>> + 'frame_sink.cpp',\n> >>>>> 'main.cpp',\n> >>>>> 'options.cpp',\n> >>>>> 'stream_options.cpp',","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 991BAC3232\n\tfor ;\n\tWed, 4 Aug 2021 09:03:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F0AC6026C;\n\tWed, 4 Aug 2021 11:03:02 +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 37CD06026C\n\tfor ;\n\tWed, 4 Aug 2021 11:03:00 +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 A463F24F;\n\tWed, 4 Aug 2021 11:02:59 +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=\"BG+5Bdnk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628067779;\n\tbh=rNuDgCIssMVxZKA/fb0a8pSGuVbcajL7YzdiIA/eVlg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BG+5BdnksYo0Xvbjnws9e62mOhgvnbQEfNtowakROMebGfYDjNAs521AdzZQ9U4xl\n\tMBJNQyVlnQBUrP/2N36Ctl10FPKv29gXm3dYwJ1Ad8nExGDoXmHeTJj8LKebladxFk\n\t4TiltJgypIQIajhdwqJ3gGYFEmvVLh5vXhuT7Wc4=","Date":"Wed, 4 Aug 2021 12:02:48 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210730010306.19956-1-laurent.pinchart@ideasonboard.com>\n\t<20210730010306.19956-3-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t<32adf448-11e8-2f49-f08e-b2019215e9b5@ideasonboard.com>\n\t\n\t<6e3268e3-534f-2181-9ba1-32fc13e4a774@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<6e3268e3-534f-2181-9ba1-32fc13e4a774@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/8] cam: Add FrameSink base class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]