[{"id":1339,"web_url":"https://patchwork.libcamera.org/comment/1339/","msgid":"<20190414204859.GQ1980@bigcity.dyn.berto.se>","date":"2019-04-14T20:48:59","subject":"Re: [libcamera-devel] [PATCH v4 01/12] libcamera: ipu3: Sub-class\n\tStream with IPU3Stream","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-04-09 21:25:37 +0200, Jacopo Mondi wrote:\n> In preparation for multiple stream support provide a Stream sub-class to\n> maintain IPU3 specific data. In order to be able to sub-class Stream\n> remove the 'final' specifier from the class definition and make its\n> private members protected.\n\nI liked that Stream was final, towards applications. I assume we will \nmake it so again once we introduce d-pointers :-)\n\nThis patch should be split in two, one touching stream.h whit a commit \nmessage explaining why Stream needs to be able to be sub-classed and one \ntouching ipu3.cpp making use of the change.\n\nThe changes themself are good.\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/stream.h | 4 ++--\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++++++++++++--\n> 2 files changed, 16 insertions(+), 4 deletions(-)\n> \n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index d0f7b0e12485..8a47930f8614 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -43,7 +43,7 @@ private:\n> \tSize size_;\n> };\n> \n> -class Stream final\n> +class Stream\n> {\n> public:\n> \tclass StillCapture : public StreamUsage\n> @@ -68,7 +68,7 @@ public:\n> \tBufferPool &bufferPool() { return bufferPool_; }\n> \tconst StreamConfiguration &configuration() const { return configuration_; }\n> \n> -private:\n> +protected:\n> \tfriend class Camera;\n> \n> \tBufferPool bufferPool_;\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index ca09da753b90..00907bb53891 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -133,6 +133,18 @@ public:\n> \tBufferPool pool_;\n> };\n> \n> +class IPU3Stream : public Stream\n> +{\n> +public:\n> +\tIPU3Stream()\n> +\t\t: active_(false)\n> +\t{\n> +\t}\n> +\n> +\tbool active_;\n> +\tstd::string name_;\n> +};\n> +\n> class PipelineHandlerIPU3 : public PipelineHandler\n> {\n> public:\n> @@ -171,7 +183,7 @@ private:\n> \t\tCIO2Device cio2_;\n> \t\tImgUDevice *imgu_;\n> \n> -\t\tStream stream_;\n> +\t\tIPU3Stream stream_;\n> \t};\n> \n> \tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> @@ -404,7 +416,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> \tV4L2Device *output = data->imgu_->output_.dev;\n> -\tStream *stream = &data->stream_;\n> +\tIPU3Stream *stream = &data->stream_;\n> \n> \t/* Queue a buffer to the ImgU output for capture. */\n> \tBuffer *buffer = request->findBuffer(stream);\n> -- \n> 2.21.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 51F0F60DBE\n\tfor ;\n\tSun, 14 Apr 2019 22:49:01 +0200 (CEST)","by mail-lj1-x233.google.com with SMTP id r24so13716072ljg.3\n\tfor ;\n\tSun, 14 Apr 2019 13:49:01 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\td23sm2531187lfj.64.2019.04.14.13.48.59\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 14 Apr 2019 13:48:59 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=yG9ygfGpKkoTTbIKFeY5bHePUhYKhTTxU8yOTAOtpvo=;\n\tb=T3TNziWqaPEYYZByWaEovgV2rqLNu69VKvFAFn3zFpZt4D7P401Lvcbofh4J0Cig5h\n\tAvPIe1nvcXFuOhZou7yqVYcmxHR6cDHHeUGU8CA8dlCXLjhhp4espne+KOgjoiPjjRIA\n\tnDLFy6SB0UuuHCu7rgB4JK3ew3+7LlGgG0B9/ZE5vAWC48OKlQV2Np/KZ5NlXxVHh4yQ\n\tPVxQvDeFpFxzH41DJ//8KaaZfbu6+r/mHzoDYLeLe7w76mzugxnyPmiB/dP49vY7MjTF\n\tVpq2du8zzag3FX87jd9oO0rMoxrk9gv012PRuXWkraBFMatVUeAYM0PI4L/NaoHAR/II\n\t9SQw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=yG9ygfGpKkoTTbIKFeY5bHePUhYKhTTxU8yOTAOtpvo=;\n\tb=Jf8DlHgmbS10l/U3OpS6Ym2Arkz9/WP/BXkJ+6f+YS2ashMJ7rAOBQB8/OPHQRZWRo\n\tl3vSw89Jk30pT6AEBEQZ8RZsSVh9WCauo7Z6lr6pP/plNIrfiu1AryLh7P6ptFLySGyD\n\to52wiQYROPkefm4gvgwgO0z2hSbJrYIX+RS8moimA6MEktd2DoP2FiCFMnGGGcfgzE+H\n\tQOcqGWtG6lq69nWTDuuwVkLAK5jxS93g+giIfLRoI/0lAnENSFrUTxRAjBvnbC7FYXOl\n\tpCzngqb4OUoy86Zo1uCBIb5uqII7xup6Gi+8ltBnVijaNy4USs9IrTBuitVZbO+x12/b\n\t1/7g==","X-Gm-Message-State":"APjAAAXpgh0/JhBAzjg7yFaR6JK+C9u5QFrhrV/XSGLeX0B196Mpaq4B\n\tA3DS/f2t9t7zn3uDGRea7C1S5g==","X-Google-Smtp-Source":"APXvYqxN8Vp2RLJ9IH+nopapU4Md8J01qzGpvbcHQAeL2vIlPdJyDqMQr22F02nBWVdLKRSOM5TXEQ==","X-Received":"by 2002:a2e:974d:: with SMTP id\n\tf13mr36881584ljj.140.1555274940626; \n\tSun, 14 Apr 2019 13:49:00 -0700 (PDT)","Date":"Sun, 14 Apr 2019 22:48:59 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190414204859.GQ1980@bigcity.dyn.berto.se>","References":"<20190409192548.20325-1-jacopo@jmondi.org>\n\t<20190409192548.20325-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190409192548.20325-2-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v4 01/12] libcamera: ipu3: Sub-class\n\tStream with IPU3Stream","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":"Sun, 14 Apr 2019 20:49:01 -0000"}},{"id":1341,"web_url":"https://patchwork.libcamera.org/comment/1341/","msgid":"<20190415080829.GB4809@pendragon.ideasonboard.com>","date":"2019-04-15T08:08:29","subject":"Re: [libcamera-devel] [PATCH v4 01/12] libcamera: ipu3: Sub-class\n\tStream with IPU3Stream","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sun, Apr 14, 2019 at 10:48:59PM +0200, Niklas Söderlund wrote:\n> On 2019-04-09 21:25:37 +0200, Jacopo Mondi wrote:\n> > In preparation for multiple stream support provide a Stream sub-class to\n\ns/stream/streams/\n\n> > maintain IPU3 specific data. In order to be able to sub-class Stream\n> > remove the 'final' specifier from the class definition and make its\n> > private members protected.\n> \n> I liked that Stream was final, towards applications. I assume we will \n> make it so again once we introduce d-pointers :-)\n> \n> This patch should be split in two, one touching stream.h whit a commit \n> message explaining why Stream needs to be able to be sub-classed and one \n> touching ipu3.cpp making use of the change.\n> \n> The changes themself are good.\n> \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/stream.h | 4 ++--\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++++++++++++--\n> > 2 files changed, 16 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > index d0f7b0e12485..8a47930f8614 100644\n> > --- a/include/libcamera/stream.h\n> > +++ b/include/libcamera/stream.h\n> > @@ -43,7 +43,7 @@ private:\n> > \tSize size_;\n> > };\n> > \n> > -class Stream final\n> > +class Stream\n> > {\n> > public:\n> > \tclass StillCapture : public StreamUsage\n> > @@ -68,7 +68,7 @@ public:\n> > \tBufferPool &bufferPool() { return bufferPool_; }\n> > \tconst StreamConfiguration &configuration() const { return configuration_; }\n> > \n> > -private:\n> > +protected:\n> > \tfriend class Camera;\n> > \n> > \tBufferPool bufferPool_;\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index ca09da753b90..00907bb53891 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -133,6 +133,18 @@ public:\n> > \tBufferPool pool_;\n> > };\n> > \n> > +class IPU3Stream : public Stream\n> > +{\n> > +public:\n> > +\tIPU3Stream()\n> > +\t\t: active_(false)\n> > +\t{\n> > +\t}\n> > +\n> > +\tbool active_;\n> > +\tstd::string name_;\n\nYou don't use those two fields yet, I'd add them in the patch that makes\nuse of them. Or, given that the IPU3 part of this patch will become very\nthin after splitting the patch in two as requested by Niklas, you could\nsquash the IPU3 changes with 02/12 from this series.\n\n> > +};\n> > +\n> > class PipelineHandlerIPU3 : public PipelineHandler\n> > {\n> > public:\n> > @@ -171,7 +183,7 @@ private:\n> > \t\tCIO2Device cio2_;\n> > \t\tImgUDevice *imgu_;\n> > \n> > -\t\tStream stream_;\n> > +\t\tIPU3Stream stream_;\n> > \t};\n> > \n> > \tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> > @@ -404,7 +416,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > \tV4L2Device *output = data->imgu_->output_.dev;\n> > -\tStream *stream = &data->stream_;\n> > +\tIPU3Stream *stream = &data->stream_;\n> > \n> > \t/* Queue a buffer to the ImgU output for capture. */\n> > \tBuffer *buffer = request->findBuffer(stream);","headers":{"Return-Path":"","Received":["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 E2B7860B2E\n\tfor ;\n\tMon, 15 Apr 2019 10:08:37 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5174F301;\n\tMon, 15 Apr 2019 10:08:37 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555315717;\n\tbh=zlBYSjc/bmWKSwnW6rXxCZ5DiCY7rgKZW3tvLDpASO8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EgTbXrZWmVahNvqE2VuchAT7mQF59U8DWk+/JyCeP+JEWrfyphvlV6bRUnfkoftnV\n\tAM48iWZudEqT2gEIezTFCeYB2gnGz0nxtYA3QmF/PAw9iO/9+vG/gTP0WO78MJGRgL\n\tQWHdeSd5lrfYrRpn3Jnm0TND9pO5aj2T/f6pvNFs=","Date":"Mon, 15 Apr 2019 11:08:29 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190415080829.GB4809@pendragon.ideasonboard.com>","References":"<20190409192548.20325-1-jacopo@jmondi.org>\n\t<20190409192548.20325-2-jacopo@jmondi.org>\n\t<20190414204859.GQ1980@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190414204859.GQ1980@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 01/12] libcamera: ipu3: Sub-class\n\tStream with IPU3Stream","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":"Mon, 15 Apr 2019 08:08:38 -0000"}}]