[{"id":3166,"web_url":"https://patchwork.libcamera.org/comment/3166/","msgid":"<20191202104529.owipy2iewrilkgtg@uno.localdomain>","date":"2019-12-02T10:45:29","subject":"Re: [libcamera-devel] [PATCH 18/30] libcamera: v4l2_videodevice:\n\tAdd V4L2BufferCache to deal with index mapping","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Nov 27, 2019 at 12:36:08AM +0100, Niklas Söderlund wrote:\n> In preparation for the FrameBuffer interface add a class which will deal\ns/FrameBuffer interface/switch to use the FrameBuffer interface/ ?\n\n> with keeping the cache between dmafds and V4L2 video device buffer\n> indexes.\n>\n> This initial implement ensures that no hot association is lost while its\n> eviction strategy could be improved in the future.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/include/v4l2_videodevice.h | 20 ++++-\n> src/libcamera/v4l2_videodevice.cpp | 105 ++++++++++++++++++++++-\n> 2 files changed, 123 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> index 34bbff41760753bd..254f8797af42dd8a 100644\n> --- a/src/libcamera/include/v4l2_videodevice.h\n> +++ b/src/libcamera/include/v4l2_videodevice.h\n> @@ -12,6 +12,7 @@\n>\n> #include \n>\n> +#include \n> #include \n> #include \n> #include \n> @@ -22,7 +23,6 @@\n>\n> namespace libcamera {\n>\n> -class Buffer;\n\nIs this and the removal of the buffer.h inclusion in\nv4l2_videodevice.cpp related with the introduction of this class ?\n\n> class BufferMemory;\n> class BufferPool;\n> class EventNotifier;\n> @@ -105,6 +105,24 @@ struct V4L2Capability final : v4l2_capability {\n> \t}\n> };\n>\n> +class V4L2BufferCache\n> +{\n> +public:\n> +\tV4L2BufferCache(unsigned int size);\n> +\tV4L2BufferCache(const std::vector buffers);\n> +\n> +\tint fetch(const FrameBuffer *buffer);\n\nNot sure what's best, as I've not seen this in use, but what about a\nconst FrameBuffer & ?\n\n> +\tvoid put(unsigned int index);\n> +\n> +private:\n> +\tstruct CacheInfo {\n\nCacheEntry ?\n\n> +\t\tbool free;\n> +\t\tstd::vector last;\n> +\t};\n> +\n> +\tstd::vector cache_;\n> +};\n> +\n> class V4L2DeviceFormat\n> {\n> public:\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index a05dd6a1f7d86eaa..c82f2829601bd14c 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -19,7 +19,6 @@\n>\n> #include \n>\n> -#include \n> #include \n>\n> #include \"log.h\"\n> @@ -134,6 +133,110 @@ LOG_DECLARE_CATEGORY(V4L2)\n> * \\return True if the video device provides Streaming I/O IOCTLs\n> */\n>\n> +/**\n> + * \\class V4L2BufferCache\n> + * \\brief Hot cache of associations between V4L2 index and FrameBuffer\n\nV4L2 buffer indexes ?\n\n> + *\n> + * There is performance to be gained if the same V4L2 buffer index can be\n> + * reused for the same FrameBuffer object as the kernel don't have to redo\n\nthe kernel -doesn't-\n\n> + * the mapping. The V4L2BufferCache tries to keep a hot-cache of mappings\n> + * between the two.\n> + *\n> + * If there is a cache miss is not critical, everything still works as expected.\n> + */\n> +\n> +/**\n> + * \\brief Create a empty cache of a given size\n\nan empty\nof a give \\a size\n\n> + * \\param[in] size Size of cache to create\n\nNumber of entries to reserve in the cache ?\n\n> + *\n> + * Create a cold cache with \\a size entries. The cache will be populated as\n> + * it's being used.\n\nnot sure, but this sounds better to me as\n\"as it is used\"\n\n> + */\n> +V4L2BufferCache::V4L2BufferCache(unsigned int size)\n> +{\n> +\tcache_.resize(size, { .free = true, .last = {} });\n> +}\n> +\n> +/**\n> + * \\brief Create a pre-populated cache\n> + * \\param[in] buffers Array of buffers to pre-populated with\n> + *\n> + * Create a warm cache from \\a buffers.\n> + */\n> +V4L2BufferCache::V4L2BufferCache(const std::vector buffers)\n> +{\n> +\tfor (const FrameBuffer *buffer : buffers)\n> +\t\tcache_.push_back({ .free = true, .last = buffer->planes() });\n> +}\n\nHere and in all over this patch, are you copying vectors around ?\n\n> +\n> +/**\n> + * \\brief Fetch a index from the cache\n\nan index\n\nI would:\n\"Retrieve the V4L2 buffer index associated with \\a buffer\"\n\n> + * \\param[in] buffer FrameBuffer to match\n\nThe FrameBuffer\n\n> + *\n> + * Try to find \\a buffer in cache and if it's free reuse the last used index\n> + * for this buffer. If the buffer have never been seen or if have been evinced\n\nthe buffer -has-\n\n> + * from the cache the first free index is pieced instead. Likewise if the last\n\nnot sure about what pieced means.\n\n> + * used index is in use a new free index is picked.\n\nI would just\n\"If no index is associated with \\a buffer in the cache, or the index\nhas been re-assigned, use the first available one, if any\"\n\n> + *\n> + * When an index is picked it is marked as in-use and returned to the caller.\n> + * The association is also recorded so it if possible can reused the next time\n> + * the FrameBuffer is seen.\n\nThat's the purpose of a cache :)\n\n> + *\n> + * \\return V4L2 buffer index\n\nor -ENOENT\n\n> + */\n> +int V4L2BufferCache::fetch(const FrameBuffer *buffer)\n> +{\n> +\tint use = -1;\n> +\n> +\tfor (unsigned int index = 0; index < cache_.size(); index++) {\n> +\t\tif (!cache_[index].free)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (use < 0)\n> +\t\t\tuse = index;\n\nI wonder if we shouldn't keep the V4L2 buffer index in the CacheInfo\ninstead of using the CacheInfo entry position in the vector.\n\n> +\n> +\t\t/* Try to find a cache hit by comparing the planes. */\n> +\t\tstd::vector planes = buffer->planes();\n\nAre you copying the vector ?\n\n> +\t\tif (cache_[index].last.size() != planes.size())\n> +\t\t\tcontinue;\n> +\n> +\t\tbool match = true;\n> +\t\tfor (unsigned int i = 0; i < planes.size(); i++) {\n> +\t\t\tif (cache_[index].last[i].fd != planes[i].fd ||\n> +\t\t\t cache_[index].last[i].length != planes[i].length) {\n> +\t\t\t\tmatch = false;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n\nI would here just use the address of the planes() vector as the key\ninstead of comparing it's content. Or is there a risk or relocation?\n\n> +\t\t}\n> +\n> +\t\tif (!match)\n> +\t\t\tcontinue;\n> +\n> +\t\tuse = index;\n> +\t\tbreak;\n> +\t}\n> +\n> +\tif (use < 0)\n> +\t\treturn -ENOENT;\n> +\n> +\tcache_[use].free = false;\n> +\tcache_[use].last = buffer->planes();\n\nThis seems to be a copy as well\n\n> +\n> +\treturn use;\n> +}\n> +\n> +/**\n> + * \\brief Pit a V4L2 index back in the cache\n\ns/Pit/Put\n\nI would:\n\"Make the V4L2 buffer \\a idex available again in the cache\"\n\n\n> + * \\param[in] index V4L2 index\n\nThe V4L2 buffer index\n\n> + *\n> + * Mark the \\a index as free in the cache so it can be reused.\n\nthe v4l2 buffer \\a index\n\n> + */\n> +void V4L2BufferCache::put(unsigned int index)\n\nput would suggest a get() counterpart ? Or fetch/put is a good enough\noperation names pair ?\n\n> +{\n> +\tASSERT(index < cache_.size());\n> +\tcache_[index].free = true;\n\nIf you keep the v4l2 index in the CacheInfo class as I've suggested\nyou would here pay the price of iterating through the vector. How\noften is a v4l2 buffer index given back to the cache ?\n\nThanks\n j\n\n> +}\n> +\n> /**\n> * \\class V4L2DeviceFormat\n> * \\brief The V4L2 video device image format and sizes\n> --\n> 2.24.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 relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A2B4360BFF\n\tfor ;\n\tMon, 2 Dec 2019 11:43:20 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 0E642240017;\n\tMon, 2 Dec 2019 10:43:19 +0000 (UTC)"],"Date":"Mon, 2 Dec 2019 11:45:29 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191202104529.owipy2iewrilkgtg@uno.localdomain>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-19-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"qsjddrne2pyrofzm\"","Content-Disposition":"inline","In-Reply-To":"<20191126233620.1695316-19-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 18/30] libcamera: v4l2_videodevice:\n\tAdd V4L2BufferCache to deal with index mapping","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, 02 Dec 2019 10:43:20 -0000"}},{"id":3247,"web_url":"https://patchwork.libcamera.org/comment/3247/","msgid":"<20191215011621.GB4857@pendragon.ideasonboard.com>","date":"2019-12-15T01:16:21","subject":"Re: [libcamera-devel] [PATCH 18/30] libcamera: v4l2_videodevice:\n\tAdd V4L2BufferCache to deal with index mapping","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, Dec 02, 2019 at 11:45:29AM +0100, Jacopo Mondi wrote:\n> On Wed, Nov 27, 2019 at 12:36:08AM +0100, Niklas Söderlund wrote:\n> > In preparation for the FrameBuffer interface add a class which will deal\n> s/FrameBuffer interface/switch to use the FrameBuffer interface/ ?\n> \n> > with keeping the cache between dmafds and V4L2 video device buffer\n> > indexes.\n> >\n> > This initial implement ensures that no hot association is lost while its\n> > eviction strategy could be improved in the future.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/include/v4l2_videodevice.h | 20 ++++-\n> > src/libcamera/v4l2_videodevice.cpp | 105 ++++++++++++++++++++++-\n> > 2 files changed, 123 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> > index 34bbff41760753bd..254f8797af42dd8a 100644\n> > --- a/src/libcamera/include/v4l2_videodevice.h\n> > +++ b/src/libcamera/include/v4l2_videodevice.h\n> > @@ -12,6 +12,7 @@\n> >\n> > #include \n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -22,7 +23,6 @@\n> >\n> > namespace libcamera {\n> >\n> > -class Buffer;\n> \n> Is this and the removal of the buffer.h inclusion in\n> v4l2_videodevice.cpp related with the introduction of this class ?\n\nThe removal of the Buffer forward declaration is related to the\ninclusion of buffer.h, which is required for the\nstd::vector below.\n\n> > class BufferMemory;\n> > class BufferPool;\n\nThe BufferMemory and BufferPool forward declarations should be removed\ntoo.\n\n> > class EventNotifier;\n> > @@ -105,6 +105,24 @@ struct V4L2Capability final : v4l2_capability {\n> > \t}\n> > };\n> >\n> > +class V4L2BufferCache\n> > +{\n> > +public:\n> > +\tV4L2BufferCache(unsigned int size);\n> > +\tV4L2BufferCache(const std::vector buffers);\n> > +\n> > +\tint fetch(const FrameBuffer *buffer);\n> \n> Not sure what's best, as I've not seen this in use, but what about a\n> const FrameBuffer & ?\n\nAs buffer can't be NULL, I would indeed use a const reference.\n> \n> > +\tvoid put(unsigned int index);\n\nI think you also need a reset() method to be called at stream stop time\nfor imported buffers. It should set the free flag for all entries.\n\n> > +\n> > +private:\n> > +\tstruct CacheInfo {\n> \n> CacheEntry ?\n\nSounds better to me, or just Entry.\n\n> > +\t\tbool free;\n> > +\t\tstd::vector last;\n> > +\t};\n> > +\n> > +\tstd::vector cache_;\n> > +};\n> > +\n> > class V4L2DeviceFormat\n> > {\n> > public:\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index a05dd6a1f7d86eaa..c82f2829601bd14c 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -19,7 +19,6 @@\n> >\n> > #include \n> >\n> > -#include \n> > #include \n> >\n> > #include \"log.h\"\n> > @@ -134,6 +133,110 @@ LOG_DECLARE_CATEGORY(V4L2)\n> > * \\return True if the video device provides Streaming I/O IOCTLs\n> > */\n> >\n> > +/**\n> > + * \\class V4L2BufferCache\n> > + * \\brief Hot cache of associations between V4L2 index and FrameBuffer\n> \n> V4L2 buffer indexes ?\n> \n> > + *\n> > + * There is performance to be gained if the same V4L2 buffer index can be\n> > + * reused for the same FrameBuffer object as the kernel don't have to redo\n> \n> the kernel -doesn't-\n> \n> > + * the mapping. The V4L2BufferCache tries to keep a hot-cache of mappings\n> > + * between the two.\n> > + *\n> > + * If there is a cache miss is not critical, everything still works as expected.\n\n\"A performance to be gained\" is a bit of an understatement, as trashing\nthis cash would just make the camera unusable in many cases :-) And I\nthink we should explain that this related to dmabuf mappings, it's not\napparent from the text. How about the following ?\n\n * When importing buffers, V4L2 performs lazy mapping of dmabuf instances at\n * VIDIOC_QBUF (or VIDIOC_PREPARE_BUF) time and keeps the mapping associated\n * with the V4L2 buffer, as identified by its index. If the same V4L2 buffer is\n * then reused and queued with different dmabufs, the old dmabufs will be\n * unmapped and the new ones mapped. To keep this process efficient, it is\n * crucial to consistently use the same V4L2 buffer for given dmabufs through\n * the whole duration of a capture cycle.\n *\n * The V4L2BufferCache class keeps a map of previous dmabufs to V4L2 buffer\n * index associations to help selecting V4L2 buffers. It tracks, for every\n * entry, if the V4L2 buffer is in use, and offers lookup of the best free V4L2\n * buffer for a set of dmabufs.\n\nAnd now that I've written this, it occurs to me that this isn't really a\ncache in the traditional sense of storing data so that future requests\nfor that data will be served faster :-) I can't really think of a better\nname for the class though, as V4L2BufferMap would likely be a bit more\naccurate, but less descriptive.\n\n> > + */\n> > +\n> > +/**\n> > + * \\brief Create a empty cache of a given size\n> \n> an empty\n> of a give \\a size\n\ns/given/give/ or just \"with \\a size entries\"\n\n> \n> > + * \\param[in] size Size of cache to create\n> \n> Number of entries to reserve in the cache ?\n\nSeems better to me. The parameter could be named numEntries to make the\nAPI more explicit.\n\n> > + *\n> > + * Create a cold cache with \\a size entries. The cache will be populated as\n> > + * it's being used.\n> \n> not sure, but this sounds better to me as\n> \"as it is used\"\n\nAgreed. I would also say \"empty\" instead of \"cold\". Or possibly better,\n\n * Create a cache with \\a size entries all marked as unused. The entries will be\n * populated as the cache is used. This is typically used to implement buffer\n * import, with buffers added to the cache as they are queued.\n\n> > + */\n> > +V4L2BufferCache::V4L2BufferCache(unsigned int size)\n> > +{\n> > +\tcache_.resize(size, { .free = true, .last = {} });\n\nIf you replaced .free with .used (or .inUse or a similar name), this\ncould become\n\n\tcache_.resize(size);\n\nand the data will be zero-initialized:\n\nhttps://en.cppreference.com/w/cpp/container/vector/resize\n\n\"If the current size is less than count,\n1) additional default-inserted elements are appended\"\n\nhttps://en.cppreference.com/w/cpp/named_req/DefaultInsertable\n\n\"By default, this will call placement-new, as by ::new((void*)p) T()\n(that is, value-initialize the object pointed to by p).\"\n\nhttps://en.cppreference.com/w/cpp/language/value_initialization\n\n\"2) if T is a class type with a default constructor that is neither\nuser-provided nor deleted (that is, it may be a class with an\nimplicitly-defined or defaulted default constructor), the object is\nzero-initialized and then it is default-initialized if it has a\nnon-trivial default constructor;\"\n\n> > +}\n> > +\n> > +/**\n> > + * \\brief Create a pre-populated cache\n> > + * \\param[in] buffers Array of buffers to pre-populated with\n> > + *\n> > + * Create a warm cache from \\a buffers.\n\nAnd to align with the proposal above,\n\n * Create a cache pre-populated with \\a buffers. This is typically used to\n * implement buffer export, with all buffers added to the cache when they are\n * allocated. \n\n> > + */\n> > +V4L2BufferCache::V4L2BufferCache(const std::vector buffers)\n> > +{\n> > +\tfor (const FrameBuffer *buffer : buffers)\n> > +\t\tcache_.push_back({ .free = true, .last = buffer->planes() });\n\nHow about\n\n\t.emplace_back(true, buffer->planes());\n\nwith a constructor for CacheInfo\n\n\t\tCacheInfo() = default;\n\t\tCacheInfo(bool free, const std::vector &last)\n\t\t\t: free(free), last(last)\n\t\t{\n\t\t}\n\nThe default constructor is required to make cache_.resize(size) valid\nabove, as the user-defined constructor causes the compiler to not\ngenerate the default constructor otherwise.\n\nYou could even possibly skip the free parameter as it's always set to\ntrue.\n\n> > +}\n> \n> Here and in all over this patch, are you copying vectors around ?\n\nDo you mean for .last ? It seems to be the case, yes, but I don't think\nwe can really avoid that. We can't store a reference (or pointer)\ninstead as that would have implications on the lifetime of the\nFrameBuffer vector that would be annoying for applications. For exported\nbuffers we may be fine as the FrameBuffer vector is allocated by the\nBufferAllocator, but for imported buffers we would need to ensure that\napplications never delete a FrameBuffer before stopping the stream. We\ncould document this requirement but it would be cumbersome to enforce\nprogrammatically, and thus error prone.\n\n> > +\n> > +/**\n> > + * \\brief Fetch a index from the cache\n> \n> an index\n> \n> I would:\n> \"Retrieve the V4L2 buffer index associated with \\a buffer\"\n\n\"Find the best V4L2 buffer for a FrameBuffer\" ?\n\n> > + * \\param[in] buffer FrameBuffer to match\n> \n> The FrameBuffer\n\nAnd I would either drop \"to match\" or expand the line to \"The\nFrameBuffer to get a V4L2 buffer for\", with a preference for the shorter\noption.\n\n> > + *\n> > + * Try to find \\a buffer in cache and if it's free reuse the last used index\n> > + * for this buffer. If the buffer have never been seen or if have been evinced\n> \n> the buffer -has-\n> \n> > + * from the cache the first free index is pieced instead. Likewise if the last\n> \n> not sure about what pieced means.\n\npicked ?\n\n> > + * used index is in use a new free index is picked.\n> \n> I would just\n> \"If no index is associated with \\a buffer in the cache, or the index\n> has been re-assigned, use the first available one, if any\"\n>\n> > + *\n> > + * When an index is picked it is marked as in-use and returned to the caller.\n> > + * The association is also recorded so it if possible can reused the next time\n> > + * the FrameBuffer is seen.\n> \n> That's the purpose of a cache :)\n\n * Find the best V4L2 buffer index to be used for the FrameBuffer \\a buffer\n * based on previous mappings of frame buffers to V4L2 buffers. If a free V4L2\n * buffer previously used with the same dmabufs as \\a buffer is found in the\n * cache, return its index. Otherwise return the index of the first free V4L2\n * buffer and record its association with the dmabufs of \\a buffer.\n\n> \n> > + *\n> > + * \\return V4L2 buffer index\n> \n> or -ENOENT\n\n\"The index of the best V4L2 buffer, or -ENOENT if no free V4L2 buffer is\navailable\"\n\nThe only caller doesn't perform any error check by the way. In theory\nthis would indicate a serious bug anyway, but I don't think we prevent\napplications from creating more FrameBuffer instances and queuing more\nrequests than the number of allocated buffers for the V4L2 devices. This\nis an issue that should be handled in the pipeline handlers (or possibly\nin the base PipelineHandler class), on top of this series. You could add\nan error check in the caller in the meantime.\n\n> > + */\n> > +int V4L2BufferCache::fetch(const FrameBuffer *buffer)\n> > +{\n> > +\tint use = -1;\n> > +\n> > +\tfor (unsigned int index = 0; index < cache_.size(); index++) {\n\n\t\tconst CacheInfo &entry = cache_[index];\n\nand use entry everywhere below.\n\n> > +\t\tif (!cache_[index].free)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tif (use < 0)\n> > +\t\t\tuse = index;\n\nYou will use the first free entry as a replacement policy instead of the\noldest entry as done today. What are the implications on performances ?\n\n> I wonder if we shouldn't keep the V4L2 buffer index in the CacheInfo\n> instead of using the CacheInfo entry position in the vector.\n> \n> > +\n> > +\t\t/* Try to find a cache hit by comparing the planes. */\n> > +\t\tstd::vector planes = buffer->planes();\n> \n> Are you copying the vector ?\n\nThis should be a const reference.\n\n> > +\t\tif (cache_[index].last.size() != planes.size())\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tbool match = true;\n> > +\t\tfor (unsigned int i = 0; i < planes.size(); i++) {\n> > +\t\t\tif (cache_[index].last[i].fd != planes[i].fd ||\n> > +\t\t\t cache_[index].last[i].length != planes[i].length) {\n\nMaybe\n\n\t\t\tif (entry.last != buffer->planes()) {\n\nand drop the size check above (and the local planes variable) ?\n\n> > +\t\t\t\tmatch = false;\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> \n> I would here just use the address of the planes() vector as the key\n> instead of comparing it's content. Or is there a risk or relocation?\n\nPlease see above.\n\n> > +\t\t}\n> > +\n> > +\t\tif (!match)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tuse = index;\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\tif (use < 0)\n> > +\t\treturn -ENOENT;\n> > +\n> > +\tcache_[use].free = false;\n> > +\tcache_[use].last = buffer->planes();\n> \n> This seems to be a copy as well\n> \n> > +\n> > +\treturn use;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Pit a V4L2 index back in the cache\n> \n> s/Pit/Put\n> \n> I would:\n> \"Make the V4L2 buffer \\a idex available again in the cache\"\n\nThis ends up duplicating the longer description below. I would just\nremove it and use\n\n * \\brief Mark buffer \\a index as free in the cache\n\n> > + * \\param[in] index V4L2 index\n> \n> The V4L2 buffer index\n> \n> > + *\n> > + * Mark the \\a index as free in the cache so it can be reused.\n> \n> the v4l2 buffer \\a index\n\ns/v4l2/V4L2/\n\nBut I would drop this line as it just duplicates the \\brief.\n\n> > + */\n> > +void V4L2BufferCache::put(unsigned int index)\n> \n> put would suggest a get() counterpart ? Or fetch/put is a good enough\n> operation names pair ?\n\nThe names are possibly not the best, but the name of the class is also\nnot quite accurate as explained above. This is an internal API and its\nusage is limited to V4L2VideoDevice so I would go for get() and put().\n\n> > +{\n> > +\tASSERT(index < cache_.size());\n> > +\tcache_[index].free = true;\n> \n> If you keep the v4l2 index in the CacheInfo class as I've suggested\n> you would here pay the price of iterating through the vector. How\n> often is a v4l2 buffer index given back to the cache ?\n\nEvery time the buffer is dequeued, so once per frame. The lookup is also\nperformed once per frame.\n\n> > +}\n> > +\n> > /**\n> > * \\class V4L2DeviceFormat\n> > * \\brief The V4L2 video device image format and sizes","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 4327A601E5\n\tfor ;\n\tSun, 15 Dec 2019 02:16:31 +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 9FABC2D1;\n\tSun, 15 Dec 2019 02:16:30 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1576372590;\n\tbh=6criRsVthkcVa/UAk/gSmUMIOm3z9i3NQHt4AtFB7lo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jp+74o/IN30jjfPrD8IRNnreAm7Jb8KQdtorCjAT8Ii6Bk1L1g+/QDJBBrWG11Hzp\n\tRH4KBzyThz62O/e6mkB57FY5JfQTeIxxFa8xc4wGzYlBVP2bp5BAK8Zc9Yo03luAHn\n\tyHOnIYWewUflrYvMVeGCu+r5DcUAjAJaV8NqRgsM=","Date":"Sun, 15 Dec 2019 03:16:21 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20191215011621.GB4857@pendragon.ideasonboard.com>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-19-niklas.soderlund@ragnatech.se>\n\t<20191202104529.owipy2iewrilkgtg@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191202104529.owipy2iewrilkgtg@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 18/30] libcamera: v4l2_videodevice:\n\tAdd V4L2BufferCache to deal with index mapping","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":"Sun, 15 Dec 2019 01:16:31 -0000"}}]