[libcamera-devel] [RFC PATCH v1 08/12] libcamera: framebuffer: Prevent modifying the number of metadata planes
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Sep 3 18:30:33 CEST 2021
Hi Hiro,
On Fri, Sep 03, 2021 at 08:40:16PM +0900, Hirokazu Honda wrote:
> On Fri, Sep 3, 2021 at 5:26 PM Kieran Bingham wrote:
> > On 02/09/2021 19:20, Laurent Pinchart wrote:
> > > 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.
Yes, and that's the problem that this series tries to address. It's
something that was missing from your series, that I thought we could
live with for the time being, but it turns out to cause issues.
I'll continue working to fix the fallout of the offset addition over the
weekend, but if I run out of time, I'll revert it all before Monday.
> > >>> 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;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list