[libcamera-devel] [RFC PATCH v1 09/12] libcamera: v4l2_videodevice: Cache PixelFormatInfo

[Laurent Pinchart]

Hi Hiro,

On Fri, Sep 03, 2021 at 08:54:52PM +0900, Hirokazu Honda wrote:
> On Fri, Sep 3, 2021 at 5:48 PM Laurent Pinchart wrote:
> > On Fri, Sep 03, 2021 at 09:29:40AM +0100, Kieran Bingham wrote:
> > > On 02/09/2021 19:26, Laurent Pinchart wrote:
> > > > On Thu, Sep 02, 2021 at 11:21:29AM +0100, Kieran Bingham wrote:
> > > >> On 02/09/2021 05:23, Laurent Pinchart wrote:
> > > >>> Cache the PixelFormatInfo instead of looking it up in every call to
> > > >>> createBuffer(). This prepare for usage of the info in queueBuffer(), to
> > > >>
> > > >> s/prepare/prepares/
> > > >>
> > > >>> avoid a looking every time a buffer is queued.
> > > >>>
> > > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > >>> ---
> > > >>>  include/libcamera/internal/v4l2_videodevice.h |  1 +
> > > >>>  src/libcamera/v4l2_videodevice.cpp            | 19 +++++++++++--------
> > > >>>  2 files changed, 12 insertions(+), 8 deletions(-)
> > > >>>
> > > >>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > > >>> index 7a145f608a5b..6096cd609b97 100644
> > > >>> --- a/include/libcamera/internal/v4l2_videodevice.h
> > > >>> +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > >>> @@ -243,6 +243,7 @@ private:
> > > >>>
> > > >>>   V4L2Capability caps_;
> > > >>>   V4L2DeviceFormat format_;
> > > >>> + const PixelFormatInfo *formatInfo_;
> > > >>>
> > > >>>   enum v4l2_buf_type bufferType_;
> > > >>>   enum v4l2_memory memoryType_;
> > > >>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > >>> index 82ddaed3656f..2d9a94c3c974 100644
> > > >>> --- a/src/libcamera/v4l2_videodevice.cpp
> > > >>> +++ b/src/libcamera/v4l2_videodevice.cpp
> > > >>> @@ -482,8 +482,8 @@ const std::string V4L2DeviceFormat::toString() const
> > > >>>   * \param[in] deviceNode The file-system path to the video device node
> > > >>>   */
> > > >>>  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> > > >>> - : V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr),
> > > >>> -   streaming_(false)
> > > >>> + : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),
> > > >>> +   fdBufferNotifier_(nullptr), streaming_(false)
> > > >>>  {
> > > >>>   /*
> > > >>>    * We default to an MMAP based CAPTURE video device, however this will
> > > >>> @@ -586,6 +586,7 @@ int V4L2VideoDevice::open()
> > > >>>           return ret;
> > > >>>   }
> > > >>>
> > > >>> + formatInfo_ = &PixelFormatInfo::info(format_.fourcc);
> > > >>
> > > >> I shudder a little seeing code hugging the return like that ;-)
> > > >>
> > > >> But it's purely subjective, same below of course.
> > > >
> > > > We have various patterns indeed :-) I haven't been able to establish yet
> > > > what my mental process to deal with these are, sometimes it bothers me
> > > > too, but not always.
> > > >
> > > >>>   return 0;
> > > >>>  }
> > > >>>
> > > >>> @@ -681,6 +682,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
> > > >>>           return ret;
> > > >>>   }
> > > >>>
> > > >>> + formatInfo_ = &PixelFormatInfo::info(format_.fourcc);
> > > >>>   return 0;
> > > >>>  }
> > > >>>
> > > >>> @@ -695,6 +697,8 @@ void V4L2VideoDevice::close()
> > > >>>   releaseBuffers();
> > > >>>   delete fdBufferNotifier_;
> > > >>>
> > > >>> + formatInfo_ = nullptr;
> > > >>> +
> > > >>>   V4L2Device::close();
> > > >>>  }
> > > >>>
> > > >>> @@ -787,6 +791,7 @@ int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)
> > > >>>           return ret;
> > > >>>
> > > >>>   format_ = *format;
> > > >>> + formatInfo_ = &PixelFormatInfo::info(format_.fourcc);
> > > >>
> > > >> Looks like we were already using that pattern though ;-)
> > > >>
> > > >>>   return 0;
> > > >>>  }
> > > >>>
> > > >>> @@ -1325,19 +1330,17 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> > > >>>           planes.push_back(std::move(plane));
> > > >>>   }
> > > >>>
> > > >>> - const auto &info = PixelFormatInfo::info(format_.fourcc);
> > > >>> - if (info.isValid() && info.numPlanes() != numPlanes) {
> > > >>> + if (formatInfo_->isValid() && formatInfo_->numPlanes() != numPlanes) {
> > > >>>           ASSERT(numPlanes == 1u);
> > > >>
> > > >> Not for this patch, but I guess this assert needed a
> > > >>   \todo Multiplanar support
> > > >>
> > > >> around it?
> > > >
> > > > No, when formatInfo_->numPlanes() != numPlanes, it means that we have a
> > > > multi-planar FrameBuffer with a single-planar V4L2 buffer format (e.g.
> > > > V4L2_PIX_FMT_NV12 as opposed to V4L2_PIX_FMT_NV12M). numPlanes must be 1
> > > > in that case, otherwise it's a kernel bug.
> > >
> > > As you're updating the conditional there anyway, could you add a short
> > > brief to explain that before the ASSERT please?
> > >
> > > I don't think that extra context is clear from just the if statement.
> > > (Perhaps there is more context outside of the hunk maybe, but it's not
> > > clear from just this diff).
> >
> > That's because this diff only deals with format caching :-) It's a good
> > point though, it's not trivial so I'll add a separate patch to document
> > this properly.
> >
> > > >> However, this patch is just for caching the formatInfo, which seems
> > > >> reasonable so
> > > >>
> > > >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > >>
> > > >>> -         const size_t numColorPlanes = info.numPlanes();
> > > >>> -         planes.resize(numColorPlanes);
> > > >>> +         planes.resize(formatInfo_->numPlanes());
> 
> By the way, what I would not like in PixelFormatInfo(), numPlanes()
> does for-loops always.
> Shall we add numPlanes to PixelFormatInfo?

The information is redundant, but I think it would make sense.

> Definitely, it is not related to this patch.
> 
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> 
> -Hiro
> > > >>>           const FileDescriptor &fd = planes[0].fd;
> > > >>>           size_t offset = 0;
> > > >>> -         for (size_t i = 0; i < numColorPlanes; ++i) {
> > > >>> +         for (size_t i = 0; i < planes.size(); ++i) {
> > > >>>                   planes[i].fd = fd;
> > > >>>                   planes[i].offset = offset;
> > > >>>
> > > >>>                   /* \todo Take the V4L2 stride into account */
> > > >>> -                 planes[i].length = info.planeSize(format_.size, i);
> > > >>> +                 planes[i].length = formatInfo_->planeSize(format_.size, i);
> > > >>>                   offset += planes[i].length;
> > > >>>           }
> > > >>>   }

-- 
Regards,

Laurent Pinchart