| Message ID | 20241009200142.3213065-3-chenghaoyang@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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' ? > Signed-off-by: Harvey Yang <chenghaoyang@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_; } > + > 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 >
Hi Kieran, On Thu, Oct 10, 2024 at 6:22 AM Kieran Bingham <kieran.bingham@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@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 > >
Hi Kieran and developers, On Sat, Oct 12, 2024 at 12:36 AM Cheng-Hao Yang <chenghaoyang@chromium.org> wrote: > > Hi Kieran, > > > On Thu, Oct 10, 2024 at 6:22 AM Kieran Bingham > <kieran.bingham@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@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 > > >
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_; } + 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_ + */ + /** * \brief Convert \a PixelFormat to a V4L2PixelFormat supported by the device * \param[in] pixelFormat The PixelFormat to convert
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. Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> --- include/libcamera/internal/v4l2_videodevice.h | 2 ++ src/libcamera/v4l2_videodevice.cpp | 5 +++++ 2 files changed, 7 insertions(+)