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

[Cheng-Hao Yang]

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...

>
>
> > 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
> >