[PATCH 2/2] libcamera: Add V4L2VideoDevice::getCachedFormat

[Cheng-Hao Yang]

Hi Kieran and developers,

On Sat, Oct 12, 2024 at 12:36 AM Cheng-Hao Yang
<chenghaoyang at chromium.org> wrote:
>
> Hi Kieran,
>
>
> On Thu, Oct 10, 2024 at 6:22 AM Kieran Bingham
> <kieran.bingham at ideasonboard.com> wrote:
> >
> > Quoting Harvey Yang (2024-10-09 20:58:51)
> > > In the upcoming mtkisp7 pipeline handler, it needs to access the cached
> > > format from a derived class. This patch adds a protected getter to allow
> > > that.
> >
> > "A derived class" sounds ... well - what is deriving from a
> > V4L2VideoDevice. We normally use encapsulation for these classes, not
> > derivation... ?
> >
> > Are you making some sort of specialisation of a V4L2VideoDevice that
> > somehow doesn't use the standard V4L2 API? or something else that makes
> > it 'not a V4L2VideoDevice' ?
>
> Yeah we overwrite V4L2VideoDevice in mtkisp7:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5675081
>
> I'll check with Han-lin why it's necessary though...

I've checked with Han-lin, and I found why we need this change:
In the following CL [1], we're trying to use VIDIOC_CREATE_BUFS to create
buffers with different formats within the same ImgSys, a V4L2VideoDevice.

The original API doesn't allow that, so the CL abstracts V4L2BufferCache [2],
lets ImgsysVideoDevice inherit V4L2VideoDevice [3], and use another cache
that contains multiple `SimpleV4L2BufferCache`s [4] (which is mostly the
original V4L2BufferCache, with an extra offset [5] that allows indices starting
from a non-zero index).

I know it's controversial, but let's discuss here what would be the best
approach to support multiple formats. Let me know your concerns and
preferences.

BR,
Harvey


[1]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5675081
[2]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5675081/1/include/libcamera/internal/v4l2_videodevice.h
[3]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5675081/1/src/libcamera/pipeline/mtkisp7/imgsys/imgsys.h
[4]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5675081/1/src/libcamera/pipeline/mtkisp7/imgsys/imgsys.h#65
[5]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5675081/1/include/libcamera/internal/v4l2_videodevice.h#174

>
> >
> >
> > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > ---
> > >  include/libcamera/internal/v4l2_videodevice.h | 2 ++
> > >  src/libcamera/v4l2_videodevice.cpp            | 5 +++++
> > >  2 files changed, 7 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > > index 9f53e37cd..389276302 100644
> > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > @@ -241,6 +241,8 @@ public:
> > >  protected:
> > >         std::string logPrefix() const override;
> > >
> > > +       V4L2DeviceFormat getCachedFormat() { return format_; }
> >
> > We don't normally call accessors 'cached'. Is this specifically cached
> > or would it be wrong to just name this getFormat()? Would that imply
> > that the function should call down to the device and get the format
> > perhaps ??
> >
> > In fact, I don't think we usually prefix 'get' either so it could just
> > be
> >         V4L2DeviceFormat format() { return format_; }
>
> I'm not sure if `format()` makes more sense, as there's
> `V4L2VideoDevice::getFormat()`, which seems to get info from V4L2
> API directly. `format_`, however, is the format that was set to V4L2 before.
>
> WDYT?
>
> BR,
> Harvey
>
>
> >
> >
> >
> > > +
> > >  private:
> > >         LIBCAMERA_DISABLE_COPY(V4L2VideoDevice)
> > >
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 1110fb535..1f6ad96c1 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -2113,6 +2113,11 @@ V4L2VideoDevice::fromEntityName(const MediaDevice *media,
> > >         return std::make_unique<V4L2VideoDevice>(mediaEntity);
> > >  }
> > >
> > > +/**
> > > + * \fn V4L2VideoDevice::getCachedFormat()
> > > + * \return Cached \a V4L2VideoDevice::format_
> >
> > And then I would state that this returns the format or perhaps returns
> > the current format;
> >
> > Which could at least be expanded on, or clarified in the \brief
> >
> > > + */
> > > +
> > >  /**
> > >   * \brief Convert \a PixelFormat to a V4L2PixelFormat supported by the device
> > >   * \param[in] pixelFormat The PixelFormat to convert
> > > --
> > > 2.47.0.rc0.187.ge670bccf7e-goog
> > >