[{"id":4187,"web_url":"https://patchwork.libcamera.org/comment/4187/","msgid":"<20200323111231.GE4768@pendragon.ideasonboard.com>","date":"2020-03-23T11:12:31","subject":"Re: [libcamera-devel] [RFC 4/6] libcamera: buffer: Add helper to\n\tmemcopy a FrameBuffer","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 Mon, Mar 16, 2020 at 03:41:44AM +0100, Niklas Söderlund wrote:\n> This helper may be used to memory copy a while FrameBuffer content to\n\ns/while/whole/ ?\n\n> another buffer. The operation is not fast and should not be used without\n> grate care by pipelines.\n\nUnless you really meant to talk about BBQs, s/grate/great/\n\n> \n> The intended use-case is to have an option to copy out RAW buffers from\n> the middle of an pipeline.\n\ns/an/a/\n\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/buffer.h | 1 +\n> src/libcamera/buffer.cpp | 43 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 44 insertions(+)\n> \n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 8e5ec699e3925eee..669d2591a90e5f37 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -60,6 +60,7 @@ public:\n> private:\n> \tfriend class Request; /* Needed to update request_. */\n> \tfriend class V4L2VideoDevice; /* Needed to update metadata_. */\n> +\tfriend int FrameBufferMemCopy(FrameBuffer *destination, const FrameBuffer *source);\n> \n> \tstd::vector planes_;\n> \n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 673a63d3d1658190..f680d1879b57a68b 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -211,4 +211,47 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> * core never modifies the buffer cookie.\n> */\n> \n> +int FrameBufferMemCopy(FrameBuffer *dst, const FrameBuffer *src)\n\nThis is a function, it should start with a lower-case f, and should have\na proper declaration in buffer.h. This being said, I think it would be\nbest to move it to a member function. How about\n\nint FrameBuffer::copyTo(FrameBuffer *dst)\n\n? Or maybe FrameBuffer::copyFrom(FrameBuffer *src) ?\n\nDocumentation is also needed.\n\n> +{\n> +\tif (dst->planes_.size() != src->planes_.size()) {\n> +\t\tLOG(Buffer, Error) << \"Different number of planes\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tfor (unsigned int i = 0; i < dst->planes_.size(); i++) {\n> +\t\tif (dst->planes_[i].length < src->planes_[i].length) {\n> +\t\t\tLOG(Buffer, Error) << \"Plane \" << i << \" is too small\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t}\n\nWe don't support that yet, so it's not really much of a concern, but\nwill we have to handle the case where the stride differs ? And how about\ndata offsets (when we'll have them too) ? Will we store that information\nin FrameBuffer::Plane or FrameMetadata::Plane ? I suspect the latter, so\nwe'll have to ensure that metadata is valid, is it guaranteed ?\n\n> +\n> +\tfor (unsigned int i = 0; i < dst->planes_.size(); i++) {\n> +\t\tvoid *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE, MAP_SHARED,\n> +\t\t\t dst->planes_[i].fd.fd(), 0);\n\n\t\tvoid *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE,\n\t\t\t\t MAP_SHARED, dst->planes_[i].fd.fd(), 0);\n\nAnd I think I'd name the variable dstmem or something similar.\n\n> +\n> +\t\tif (out == MAP_FAILED) {\n> +\t\t\tLOG(Buffer, Error) << \"Failed to map output plane \" << i;\n\ns/output/destination/\n\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tvoid *in = mmap(NULL, src->planes_[i].length, PROT_READ, MAP_SHARED,\n> +\t\t\t src->planes_[i].fd.fd(), 0);\n\nSame here, with srcmem.\n\n> +\n> +\t\tif (in == MAP_FAILED) {\n> +\t\t\tmunmap(out, dst->planes_[i].length);\n> +\t\t\tLOG(Buffer, Error) << \"Failed to map input plane \" << i;\n\ns/input/source/\n\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tmemcpy(out, in, src->planes_[i].length);\n\nlength, or bytesused from the metadata ?\n\n> +\n> +\t\tmunmap(in, src->planes_[i].length);\n> +\t\tmunmap(out, dst->planes_[i].length);\n\nI really don't like how we create and tear down mappings every time :-(\nThe alternative is to cache the mappings in the FrameBuffer class, but\nthat's a slippery slope. We may not need to fix this now, but I think we\nneed to consider our options.\n\n> +\t}\n> +\n> +\tdst->metadata_ = src->metadata_;\n> +\n> +\treturn 0;\n> +}\n> +\n> } /* namespace libcamera */","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 B4D1B60413\n\tfor ;\n\tMon, 23 Mar 2020 12:12:40 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 50F46308;\n\tMon, 23 Mar 2020 12:12:40 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584961960;\n\tbh=dbdSoSueDoVgOz0oWsqLsL8tgaNG1KYV5PURqm9fPn8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n6Q0zmCbQh5/RZG5URMXkxDMkCfwy53RG77urmwCjaORNQz4Kx8z7gXfNpI8bHZI5\n\tDrxUlZZbhA6Ei+vsIjkRzELZurdxSHqSTrKVx8LxtWYsizzgxGrdFuwUDN/xUnRmUF\n\t9KuqLR6/caCVPuCnyGOgFi1p0kBOWBAqKrtE0OBA=","Date":"Mon, 23 Mar 2020 13:12:31 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200323111231.GE4768@pendragon.ideasonboard.com>","References":"<20200316024146.2474424-1-niklas.soderlund@ragnatech.se>\n\t<20200316024146.2474424-5-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":"<20200316024146.2474424-5-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 4/6] libcamera: buffer: Add helper to\n\tmemcopy a FrameBuffer","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","X-List-Received-Date":"Mon, 23 Mar 2020 11:12:40 -0000"}},{"id":4244,"web_url":"https://patchwork.libcamera.org/comment/4244/","msgid":"<20200323190727.GA625480@oden.dyn.berto.se>","date":"2020-03-23T19:07:27","subject":"Re: [libcamera-devel] [RFC 4/6] libcamera: buffer: Add helper to\n\tmemcopy a FrameBuffer","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2020-03-23 13:12:31 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Mon, Mar 16, 2020 at 03:41:44AM +0100, Niklas Söderlund wrote:\n> > This helper may be used to memory copy a while FrameBuffer content to\n> \n> s/while/whole/ ?\n> \n> > another buffer. The operation is not fast and should not be used without\n> > grate care by pipelines.\n> \n> Unless you really meant to talk about BBQs, s/grate/great/\n> \n> > \n> > The intended use-case is to have an option to copy out RAW buffers from\n> > the middle of an pipeline.\n> \n> s/an/a/\n> \n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/buffer.h | 1 +\n> > src/libcamera/buffer.cpp | 43 ++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 44 insertions(+)\n> > \n> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > index 8e5ec699e3925eee..669d2591a90e5f37 100644\n> > --- a/include/libcamera/buffer.h\n> > +++ b/include/libcamera/buffer.h\n> > @@ -60,6 +60,7 @@ public:\n> > private:\n> > \tfriend class Request; /* Needed to update request_. */\n> > \tfriend class V4L2VideoDevice; /* Needed to update metadata_. */\n> > +\tfriend int FrameBufferMemCopy(FrameBuffer *destination, const FrameBuffer *source);\n> > \n> > \tstd::vector planes_;\n> > \n> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > index 673a63d3d1658190..f680d1879b57a68b 100644\n> > --- a/src/libcamera/buffer.cpp\n> > +++ b/src/libcamera/buffer.cpp\n> > @@ -211,4 +211,47 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > * core never modifies the buffer cookie.\n> > */\n> > \n> > +int FrameBufferMemCopy(FrameBuffer *dst, const FrameBuffer *src)\n> \n> This is a function, it should start with a lower-case f, and should have\n> a proper declaration in buffer.h. This being said, I think it would be\n> best to move it to a member function. How about\n> \n> int FrameBuffer::copyTo(FrameBuffer *dst)\n> \n> ? Or maybe FrameBuffer::copyFrom(FrameBuffer *src) ?\n\nI like copyFrom().\n\n> \n> Documentation is also needed.\n\nYes.\n\n> \n> > +{\n> > +\tif (dst->planes_.size() != src->planes_.size()) {\n> > +\t\tLOG(Buffer, Error) << \"Different number of planes\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tfor (unsigned int i = 0; i < dst->planes_.size(); i++) {\n> > +\t\tif (dst->planes_[i].length < src->planes_[i].length) {\n> > +\t\t\tLOG(Buffer, Error) << \"Plane \" << i << \" is too small\";\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\t}\n> \n> We don't support that yet, so it's not really much of a concern, but\n> will we have to handle the case where the stride differs ? And how about\n> data offsets (when we'll have them too) ? Will we store that information\n> in FrameBuffer::Plane or FrameMetadata::Plane ? I suspect the latter, so\n> we'll have to ensure that metadata is valid, is it guaranteed ?\n\nI'm not sure how we will deal with strides and offsets when we add \nsupport for it. When I write this code my intention was to allow copy it \none-to-one. I thought about verifying the buffer status before allowing \nit to be copied but decided against it.\n\nDo you think such a check is the right thing? I don't feel strongly \nabout it.\n\n> \n> > +\n> > +\tfor (unsigned int i = 0; i < dst->planes_.size(); i++) {\n> > +\t\tvoid *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE, MAP_SHARED,\n> > +\t\t\t dst->planes_[i].fd.fd(), 0);\n> \n> \t\tvoid *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE,\n> \t\t\t\t MAP_SHARED, dst->planes_[i].fd.fd(), 0);\n> \n> And I think I'd name the variable dstmem or something similar.\n> \n> > +\n> > +\t\tif (out == MAP_FAILED) {\n> > +\t\t\tLOG(Buffer, Error) << \"Failed to map output plane \" << i;\n> \n> s/output/destination/\n> \n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\tvoid *in = mmap(NULL, src->planes_[i].length, PROT_READ, MAP_SHARED,\n> > +\t\t\t src->planes_[i].fd.fd(), 0);\n> \n> Same here, with srcmem.\n> \n> > +\n> > +\t\tif (in == MAP_FAILED) {\n> > +\t\t\tmunmap(out, dst->planes_[i].length);\n> > +\t\t\tLOG(Buffer, Error) << \"Failed to map input plane \" << i;\n> \n> s/input/source/\n> \n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\tmemcpy(out, in, src->planes_[i].length);\n> \n> length, or bytesused from the metadata ?\n\nI picked length as it describes the full buffer length. But it might \nmake sens to use bytesused as a small optimization. I think it goes hand \nin hand with your question above, if the buffer status shall be \nvalidated or not.\n\n> \n> > +\n> > +\t\tmunmap(in, src->planes_[i].length);\n> > +\t\tmunmap(out, dst->planes_[i].length);\n> \n> I really don't like how we create and tear down mappings every time :-(\n> The alternative is to cache the mappings in the FrameBuffer class, but\n> that's a slippery slope. We may not need to fix this now, but I think we\n> need to consider our options.\n\nI don't like it either, but adding a cache of it to FrameBuffer even \nless so. But maybe we will be forced to do so at some point.\n\n> \n> > +\t}\n> > +\n> > +\tdst->metadata_ = src->metadata_;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > } /* namespace libcamera */\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1500760429\n\tfor ;\n\tMon, 23 Mar 2020 20:07:31 +0100 (CET)","by mail-lj1-x22b.google.com with SMTP id w1so15892917ljh.5\n\tfor ;\n\tMon, 23 Mar 2020 12:07:31 -0700 (PDT)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\tu10sm8878307ljk.56.2020.03.23.12.07.28\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 23 Mar 2020 12:07:28 -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\tbh=mh5/S0/AUBeqONPn86xWWdHPuQt8Oe3XA1gY38SJ/ys=;\n\tb=vTuNTs7VmiK5VevVCxcktVdKSQiHBcxB5vyx87fiJy7DHkqRGFDRtOI/cmnqg+ODmu\n\tTQHlmfT7axa9mwjF5Rf83PZOeypgLG8VFh5xzLhzV2JfaFL15VVZu5/3oAGmr6ldgmmA\n\tUfex6iIw/V6kayfZHyVjhMs7fp+KapcFcrnRwWN4erkArrqO1pOf+JCrYBW4fLptu47B\n\tJrEb8YAW5gTbqbbypcz8uVskJbk5OtusohwpkS9L3hJmhDeGXb0WHTMAbWEALe1NjXL+\n\tzroNMBrzXfO7r2HgDie7DGYtWngPqUdjc9n+MkuIX6oUDxs11mQQQBKXVNFN5/MgBpbS\n\tMMOQ==","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;\n\tbh=mh5/S0/AUBeqONPn86xWWdHPuQt8Oe3XA1gY38SJ/ys=;\n\tb=Y8DH2kY219Ic1ukKXqRry3bI2Yv0AJkiYLSlWojYsmYkQzL6jr1RXsnb3R9KQuzveB\n\t9ziLuqNhMyXD6LlM2CCPWskpCL5AWUSQlqIL4VWEDgLxss28544GUCHt/fkMW8a86cR3\n\tos8iab5mUbJroyTVt7magJeZNLXxZGh3ruUnO+K9KpOpgng9cB5xFAP6U7O6hfhW71Fu\n\trcEpUNM06K4WQuhV6g9YznIc0F3IXimWSWC9j5qVQwX8EjJwHvEA0Oodn1NfwIvf/w3N\n\tmty5BIUTVgZcFN0+gqKx591Lk6MPShe8318yaQWN69QYxljXY16IhLxf4czOeoXOK4gi\n\tfUfQ==","X-Gm-Message-State":"ANhLgQ1GlYSJeZ7cKtmxdt4Jh5VUvvLf5p8IOs9Ivdc2Zl+Wo77r+UOZ\n\t20nw8U77ls9FVmA8xcReW3c7gqy2Z7Q=","X-Google-Smtp-Source":"ADFU+vvyAJwdgQH35rh3TIyuK7/VwQG9SPGhOEHYOu4kwwSziiiOzsC+h6wtl0cud3S16+x5B+hMvg==","X-Received":"by 2002:a2e:800f:: with SMTP id\n\tj15mr13536702ljg.32.1584990450001; \n\tMon, 23 Mar 2020 12:07:30 -0700 (PDT)","Date":"Mon, 23 Mar 2020 20:07:27 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200323190727.GA625480@oden.dyn.berto.se>","References":"<20200316024146.2474424-1-niklas.soderlund@ragnatech.se>\n\t<20200316024146.2474424-5-niklas.soderlund@ragnatech.se>\n\t<20200323111231.GE4768@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200323111231.GE4768@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC 4/6] libcamera: buffer: Add helper to\n\tmemcopy a FrameBuffer","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","X-List-Received-Date":"Mon, 23 Mar 2020 19:07:31 -0000"}},{"id":4246,"web_url":"https://patchwork.libcamera.org/comment/4246/","msgid":"<20200323193727.GU4768@pendragon.ideasonboard.com>","date":"2020-03-23T19:37:27","subject":"Re: [libcamera-devel] [RFC 4/6] libcamera: buffer: Add helper to\n\tmemcopy a FrameBuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Mon, Mar 23, 2020 at 08:07:27PM +0100, Niklas Söderlund wrote:\n> On 2020-03-23 13:12:31 +0200, Laurent Pinchart wrote:\n> > On Mon, Mar 16, 2020 at 03:41:44AM +0100, Niklas Söderlund wrote:\n> >> This helper may be used to memory copy a while FrameBuffer content to\n> > \n> > s/while/whole/ ?\n> > \n> >> another buffer. The operation is not fast and should not be used without\n> >> grate care by pipelines.\n> > \n> > Unless you really meant to talk about BBQs, s/grate/great/\n> > \n> >> \n> >> The intended use-case is to have an option to copy out RAW buffers from\n> >> the middle of an pipeline.\n> > \n> > s/an/a/\n> > \n> >> \n> >> Signed-off-by: Niklas Söderlund \n> >> ---\n> >> include/libcamera/buffer.h | 1 +\n> >> src/libcamera/buffer.cpp | 43 ++++++++++++++++++++++++++++++++++++++\n> >> 2 files changed, 44 insertions(+)\n> >> \n> >> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> >> index 8e5ec699e3925eee..669d2591a90e5f37 100644\n> >> --- a/include/libcamera/buffer.h\n> >> +++ b/include/libcamera/buffer.h\n> >> @@ -60,6 +60,7 @@ public:\n> >> private:\n> >> \tfriend class Request; /* Needed to update request_. */\n> >> \tfriend class V4L2VideoDevice; /* Needed to update metadata_. */\n> >> +\tfriend int FrameBufferMemCopy(FrameBuffer *destination, const FrameBuffer *source);\n> >> \n> >> \tstd::vector planes_;\n> >> \n> >> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> >> index 673a63d3d1658190..f680d1879b57a68b 100644\n> >> --- a/src/libcamera/buffer.cpp\n> >> +++ b/src/libcamera/buffer.cpp\n> >> @@ -211,4 +211,47 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> >> * core never modifies the buffer cookie.\n> >> */\n> >> \n> >> +int FrameBufferMemCopy(FrameBuffer *dst, const FrameBuffer *src)\n> > \n> > This is a function, it should start with a lower-case f, and should have\n> > a proper declaration in buffer.h. This being said, I think it would be\n> > best to move it to a member function. How about\n> > \n> > int FrameBuffer::copyTo(FrameBuffer *dst)\n> > \n> > ? Or maybe FrameBuffer::copyFrom(FrameBuffer *src) ?\n> \n> I like copyFrom().\n> \n> > \n> > Documentation is also needed.\n> \n> Yes.\n> \n> >> +{\n> >> +\tif (dst->planes_.size() != src->planes_.size()) {\n> >> +\t\tLOG(Buffer, Error) << \"Different number of planes\";\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\tfor (unsigned int i = 0; i < dst->planes_.size(); i++) {\n> >> +\t\tif (dst->planes_[i].length < src->planes_[i].length) {\n> >> +\t\t\tLOG(Buffer, Error) << \"Plane \" << i << \" is too small\";\n> >> +\t\t\treturn -EINVAL;\n> >> +\t\t}\n> >> +\t}\n> > \n> > We don't support that yet, so it's not really much of a concern, but\n> > will we have to handle the case where the stride differs ? And how about\n> > data offsets (when we'll have them too) ? Will we store that information\n> > in FrameBuffer::Plane or FrameMetadata::Plane ? I suspect the latter, so\n> > we'll have to ensure that metadata is valid, is it guaranteed ?\n> \n> I'm not sure how we will deal with strides and offsets when we add \n> support for it. When I write this code my intention was to allow copy it \n> one-to-one. I thought about verifying the buffer status before allowing \n> it to be copied but decided against it.\n> \n> Do you think such a check is the right thing? I don't feel strongly \n> about it.\n\nIf the source and destination have different strides then we should do a\nstride-aware copy. Imagine a source with a padding to 128 bytes on every\nline, and a destination that wouldn't have that constraint. The\ndestination size would actually be smaller than the source. We'd have to\ncopy the data, not the padding.\n\nI wouldn't necessarily validate the state (the caller should know\nbetter), but I think the stride will need to be taken into account as\nit defines the data layout.\n\n> >> +\n> >> +\tfor (unsigned int i = 0; i < dst->planes_.size(); i++) {\n> >> +\t\tvoid *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE, MAP_SHARED,\n> >> +\t\t\t dst->planes_[i].fd.fd(), 0);\n> > \n> > \t\tvoid *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE,\n> > \t\t\t\t MAP_SHARED, dst->planes_[i].fd.fd(), 0);\n> > \n> > And I think I'd name the variable dstmem or something similar.\n> > \n> >> +\n> >> +\t\tif (out == MAP_FAILED) {\n> >> +\t\t\tLOG(Buffer, Error) << \"Failed to map output plane \" << i;\n> > \n> > s/output/destination/\n> > \n> >> +\t\t\treturn -EINVAL;\n> >> +\t\t}\n> >> +\n> >> +\t\tvoid *in = mmap(NULL, src->planes_[i].length, PROT_READ, MAP_SHARED,\n> >> +\t\t\t src->planes_[i].fd.fd(), 0);\n> > \n> > Same here, with srcmem.\n> > \n> >> +\n> >> +\t\tif (in == MAP_FAILED) {\n> >> +\t\t\tmunmap(out, dst->planes_[i].length);\n> >> +\t\t\tLOG(Buffer, Error) << \"Failed to map input plane \" << i;\n> > \n> > s/input/source/\n> > \n> >> +\t\t\treturn -EINVAL;\n> >> +\t\t}\n> >> +\n> >> +\t\tmemcpy(out, in, src->planes_[i].length);\n> > \n> > length, or bytesused from the metadata ?\n> \n> I picked length as it describes the full buffer length. But it might \n> make sens to use bytesused as a small optimization. I think it goes hand \n> in hand with your question above, if the buffer status shall be \n> validated or not.\n> \n> >> +\n> >> +\t\tmunmap(in, src->planes_[i].length);\n> >> +\t\tmunmap(out, dst->planes_[i].length);\n> > \n> > I really don't like how we create and tear down mappings every time :-(\n> > The alternative is to cache the mappings in the FrameBuffer class, but\n> > that's a slippery slope. We may not need to fix this now, but I think we\n> > need to consider our options.\n> \n> I don't like it either, but adding a cache of it to FrameBuffer even \n> less so. But maybe we will be forced to do so at some point.\n> \n> >> +\t}\n> >> +\n> >> +\tdst->metadata_ = src->metadata_;\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> } /* namespace libcamera */","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 6A20E60429\n\tfor ;\n\tMon, 23 Mar 2020 20:37:37 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CF320308;\n\tMon, 23 Mar 2020 20:37:36 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584992257;\n\tbh=EB/HelgG9mkNu4415AWPUv6dVNtO9+geiCKR6b3TW1Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=d48eWIHBqJdsi2Gv0HD1n5POJz1pfJcaciff8tG6kdaBsHzaUIHUZG5+iIUQHgtuB\n\tVKgamLJCHLGzE6FgJ2IEqHyX0KoMFfLPuFid+fEC11DFEJJ1heNOjN4H6P1q/TQCPe\n\tkUMovy8AjXSFVojDL9NDkp3pUcX3e9k8Wx7/GrjQ=","Date":"Mon, 23 Mar 2020 21:37:27 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200323193727.GU4768@pendragon.ideasonboard.com>","References":"<20200316024146.2474424-1-niklas.soderlund@ragnatech.se>\n\t<20200316024146.2474424-5-niklas.soderlund@ragnatech.se>\n\t<20200323111231.GE4768@pendragon.ideasonboard.com>\n\t<20200323190727.GA625480@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200323190727.GA625480@oden.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 4/6] libcamera: buffer: Add helper to\n\tmemcopy a FrameBuffer","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","X-List-Received-Date":"Mon, 23 Mar 2020 19:37:37 -0000"}}]