[libcamera-devel] [RFC PATCH v1 08/12] libcamera: framebuffer: Prevent modifying the number of metadata planes
Hirokazu Honda
hiroh at chromium.org
Fri Sep 3 13:40:16 CEST 2021
Hi Laurent, thank you for the patch.
On Fri, Sep 3, 2021 at 5:26 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> On 02/09/2021 19:20, Laurent Pinchart wrote:
> > Hi Kieran,
> >
> > On Thu, Sep 02, 2021 at 11:03:47AM +0100, Kieran Bingham wrote:
> >> On 02/09/2021 05:22, Laurent Pinchart wrote:
> >>> The number of metadata planes should always match the number of frame
> >>> buffer planes. Enforce this by making the vector private and providing
> >>> accessor functions.
> >>>
I am confused by the definition of Metadata::Planes.
It has bytesused and it is set to v4l2_buf.bytesused. It seems to
match the v4l2 format plane.
That is, it is not always the number of frame buffer planes because
planes in FrameBuffer are color format planes.
Regards,
-Hiro
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>> include/libcamera/framebuffer.h | 10 +++++++++-
> >>> src/cam/camera_session.cpp | 4 ++--
> >>> src/cam/file_sink.cpp | 2 +-
> >>> src/libcamera/framebuffer.cpp | 12 +++++++++---
> >>> src/libcamera/v4l2_videodevice.cpp | 14 +++++++-------
> >>> src/qcam/main_window.cpp | 2 +-
> >>> src/qcam/viewfinder_gl.cpp | 2 +-
> >>> src/qcam/viewfinder_qt.cpp | 2 +-
> >>> src/v4l2/v4l2_camera_proxy.cpp | 2 +-
> >>> 9 files changed, 32 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> >>> index fd68ed0a139d..7f2f176af691 100644
> >>> --- a/include/libcamera/framebuffer.h
> >>> +++ b/include/libcamera/framebuffer.h
> >>> @@ -13,6 +13,7 @@
> >>> #include <vector>
> >>>
> >>> #include <libcamera/base/class.h>
> >>> +#include <libcamera/base/span.h>
> >>>
> >>> #include <libcamera/file_descriptor.h>
> >>>
> >>> @@ -34,7 +35,14 @@ struct FrameMetadata {
> >>> Status status;
> >>> unsigned int sequence;
> >>> uint64_t timestamp;
> >>> - std::vector<Plane> planes;
> >>> +
> >>> + Span<Plane> planes() { return planes_; }
> >>> + Span<const Plane> planes() const { return planes_; }
> >>
> >> Returning a span here is nice.
> >>
> >> This likely causes compile breakage for any external app.
> >>
> >> I know we're not ABI/API stable, but I wonder if we should highlight
> >> when we do cause breakage somehow, perhaps in the commit message at
> >> least, as we already have external users who we might want to notify.
> >
> > Most changes to public header files will be ABI/API breakages at this
> > point, and users will certainly notice when they get a compilation
> > failure :-) What value do you think this would bring, who would read
> > those messages ? I think it could be different for things that change
> > the ABI without breaking compilation and that would require more than a
> > recompilation to fix, there a warning seems to have more value, but I'm
> > also sure we'll forget from time to time, if not most of the time.
>
> Because I think we need to start being more aware of when we introduce
> both ABI and API breakages.
>
> At least making it explicitly noted in the commit message states that we
> were aware of the breakage at that point.
>
>
> >>> +
> >>> +private:
> >>> + friend class FrameBuffer;
> >>> +
> >>> + std::vector<Plane> planes_;
> >>> };
> >>>
> >>> class FrameBuffer final : public Extensible
> >>> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> >>> index 60d640f2b15c..32a373a99b72 100644
> >>> --- a/src/cam/camera_session.cpp
> >>> +++ b/src/cam/camera_session.cpp
> >>> @@ -374,9 +374,9 @@ void CameraSession::processRequest(Request *request)
> >>> << " bytesused: ";
> >>>
> >>> unsigned int nplane = 0;
> >>> - for (const FrameMetadata::Plane &plane : metadata.planes) {
> >>> + for (const FrameMetadata::Plane &plane : metadata.planes()) {
> >>> info << plane.bytesused;
> >>> - if (++nplane < metadata.planes.size())
> >>> + if (++nplane < metadata.planes().size())
> >>> info << "/";
> >>> }
> >>> }
> >>> diff --git a/src/cam/file_sink.cpp b/src/cam/file_sink.cpp
> >>> index 0b529e3eb767..0fc7d621f50b 100644
> >>> --- a/src/cam/file_sink.cpp
> >>> +++ b/src/cam/file_sink.cpp
> >>> @@ -110,7 +110,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer)
> >>>
> >>> for (unsigned int i = 0; i < buffer->planes().size(); ++i) {
> >>> const FrameBuffer::Plane &plane = buffer->planes()[i];
> >>> - const FrameMetadata::Plane &meta = buffer->metadata().planes[i];
> >>> + const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];
> >>>
> >>> uint8_t *data = planeData_[&plane];
> >>> unsigned int length = std::min(meta.bytesused, plane.length);
> >>> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> >>> index c1529dfb0ad1..83cf7b83d182 100644
> >>> --- a/src/libcamera/framebuffer.cpp
> >>> +++ b/src/libcamera/framebuffer.cpp
> >>> @@ -91,8 +91,14 @@ LOG_DEFINE_CATEGORY(Buffer)
> >>> */
> >>>
> >>> /**
> >>> - * \var FrameMetadata::planes
> >>> - * \brief Array of per-plane metadata
> >>> + * \fn FrameMetadata::planes()
> >>> + * \copydoc FrameMetadata::planes() const
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn FrameMetadata::planes() const
> >>> + * \brief Retrieve the array of per-plane metadata
> >>> + * \return The array of per-plane metadata
> >>> */
> >>>
> >>> /**
> >>> @@ -210,7 +216,7 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> >>> : Extensible(std::make_unique<Private>()), planes_(planes),
> >>> cookie_(cookie)
> >>> {
> >>> - metadata_.planes.resize(planes_.size());
> >>> + metadata_.planes_.resize(planes_.size());
> >>>
> >>> unsigned int offset = 0;
> >>>
> >>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> >>> index a51971879e75..82ddaed3656f 100644
> >>> --- a/src/libcamera/v4l2_videodevice.cpp
> >>> +++ b/src/libcamera/v4l2_videodevice.cpp
> >>> @@ -1485,14 +1485,14 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> >>>
> >>> if (multiPlanar) {
> >>> unsigned int nplane = 0;
> >>> - for (const FrameMetadata::Plane &plane : metadata.planes) {
> >>> + for (const FrameMetadata::Plane &plane : metadata.planes()) {
> >>> v4l2Planes[nplane].bytesused = plane.bytesused;
> >>> v4l2Planes[nplane].length = buffer->planes()[nplane].length;
> >>> nplane++;
> >>> }
> >>> } else {
> >>> - if (metadata.planes.size())
> >>> - buf.bytesused = metadata.planes[0].bytesused;
> >>> + if (metadata.planes().size())
> >>> + buf.bytesused = metadata.planes()[0].bytesused;
> >>> }
> >>>
> >>> buf.sequence = metadata.sequence;
> >>> @@ -1587,17 +1587,17 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> >>> + buf.timestamp.tv_usec * 1000ULL;
> >>>
> >>> if (multiPlanar) {
> >>> - if (buf.length > buffer->metadata_.planes.size()) {
> >>> + if (buf.length > buffer->metadata_.planes().size()) {
> >>> LOG(V4L2, Error)
> >>> << "Invalid number of planes (" << buf.length
> >>> - << " != " << buffer->metadata_.planes.size() << ")";
> >>> + << " != " << buffer->metadata_.planes().size() << ")";
> >>> return nullptr;
> >>> }
> >>>
> >>> for (unsigned int nplane = 0; nplane < buf.length; nplane++)
> >>> - buffer->metadata_.planes[nplane].bytesused = planes[nplane].bytesused;
> >>> + buffer->metadata_.planes()[nplane].bytesused = planes[nplane].bytesused;
> >>> } else {
> >>> - buffer->metadata_.planes[0].bytesused = buf.bytesused;
> >>> + buffer->metadata_.planes()[0].bytesused = buf.bytesused;
> >>> }
> >>>
> >>> return buffer;
> >>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> >>> index 1536b2b5bd66..ac853e360aea 100644
> >>> --- a/src/qcam/main_window.cpp
> >>> +++ b/src/qcam/main_window.cpp
> >>> @@ -756,7 +756,7 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)
> >>>
> >>> qDebug().noquote()
> >>> << QString("seq: %1").arg(metadata.sequence, 6, 10, QLatin1Char('0'))
> >>> - << "bytesused:" << metadata.planes[0].bytesused
> >>> + << "bytesused:" << metadata.planes()[0].bytesused
> >>> << "timestamp:" << metadata.timestamp
> >>> << "fps:" << Qt::fixed << qSetRealNumberPrecision(2) << fps;
> >>>
> >>> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp
> >>> index 40226601f9fd..d2ef036974f4 100644
> >>> --- a/src/qcam/viewfinder_gl.cpp
> >>> +++ b/src/qcam/viewfinder_gl.cpp
> >>> @@ -125,7 +125,7 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer,
> >>> /*
> >>> * \todo Get the stride from the buffer instead of computing it naively
> >>> */
> >>> - stride_ = buffer->metadata().planes[0].bytesused / size_.height();
> >>> + stride_ = buffer->metadata().planes()[0].bytesused / size_.height();
> >>
> >> Can this be obtained from the PixelFormatInfo now ?
> >> or do we still not expose that to applications...
> >
> > PixelFormatInfo is still internal. I'm considering exposing it, but then
> > it would be good to move the V4L2-specific part somewhere else.
> >
> >> Anyway, that's likely a change on top even if we could to solve the \todo.
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>
> >>> update();
> >>> buffer_ = buffer;
> >>> }
> >>> diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp
> >>> index efa1d412584b..a0bf99b0b522 100644
> >>> --- a/src/qcam/viewfinder_qt.cpp
> >>> +++ b/src/qcam/viewfinder_qt.cpp
> >>> @@ -87,7 +87,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer,
> >>> }
> >>>
> >>> unsigned char *memory = mem.data();
> >>> - size_t size = buffer->metadata().planes[0].bytesused;
> >>> + size_t size = buffer->metadata().planes()[0].bytesused;
> >>>
> >>> {
> >>> QMutexLocker locker(&mutex_);
> >>> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> >>> index 8d8ee395954f..68e47ee81834 100644
> >>> --- a/src/v4l2/v4l2_camera_proxy.cpp
> >>> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> >>> @@ -212,7 +212,7 @@ void V4L2CameraProxy::updateBuffers()
> >>>
> >>> switch (fmd.status) {
> >>> case FrameMetadata::FrameSuccess:
> >>> - buf.bytesused = fmd.planes[0].bytesused;
> >>> + buf.bytesused = fmd.planes()[0].bytesused;
> >>> buf.field = V4L2_FIELD_NONE;
> >>> buf.timestamp.tv_sec = fmd.timestamp / 1000000000;
> >>> buf.timestamp.tv_usec = fmd.timestamp % 1000000;
> >
More information about the libcamera-devel
mailing list