| Message ID | 20210906225636.14683-14-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
On 06/09/2021 23:56, Laurent Pinchart wrote: > When dequeueing a buffer from a V4L2VideoDevice, the number of planes in > the FrameBuffer may not match the number of V4L2 buffer planes if the > PixelFormat is multi-planar (has multiple colour planes) and the V4L2 > format is single-planar (has a single buffer plane). In this case, we > need to split the single V4L2 buffer plane into FrameBuffer planes. Do > so, and add checks to reject invalid V4L2 buffers in case of a driver > issue. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v1: > > - Reduce indentation > --- > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++++++++++++--- > 1 file changed, 48 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 6c1ca9bb0a5c..2e458fcc22e0 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -7,6 +7,7 @@ > > #include "libcamera/internal/v4l2_videodevice.h" > > +#include <algorithm> > #include <array> > #include <fcntl.h> > #include <iomanip> > @@ -1669,12 +1670,54 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL > + buf.timestamp.tv_usec * 1000ULL; > > - buffer->metadata_.planes.clear(); > - if (multiPlanar) { > - for (unsigned int nplane = 0; nplane < buf.length; nplane++) > - buffer->metadata_.planes.push_back({ planes[nplane].bytesused }); > + if (V4L2_TYPE_IS_OUTPUT(buf.type)) > + return buffer; > + > + unsigned int numV4l2Planes = multiPlanar ? buf.length : 1; > + FrameMetadata &metadata = buffer->metadata_; > + metadata.planes.clear(); > + > + if (numV4l2Planes != buffer->planes().size()) { > + /* > + * If we have a multi-planar buffer with a V4L2 > + * single-planar format, split the V4L2 buffer across > + * the buffer planes. Only the last plane may have less > + * bytes used than its length. > + */ > + if (numV4l2Planes != 1) { > + LOG(V4L2, Error) > + << "Invalid number of planes (" << numV4l2Planes > + << " != " << buffer->planes().size() << ")"; I'm weary that this error print is more about the first conditional statement, but I think it still fits here anyway. > + > + metadata.status = FrameMetadata::FrameError; > + return buffer; > + } > + > + unsigned int bytesused = multiPlanar ? planes[0].bytesused > + : buf.bytesused; > + > + for (auto [i, plane] : utils::enumerate(buffer->planes())) { > + if (!bytesused) { > + LOG(V4L2, Error) > + << "Dequeued buffer is too small"; > + > + metadata.status = FrameMetadata::FrameError; > + return buffer; > + } > + > + metadata.planes.push_back({ std::min(plane.length, bytesused) }); > + bytesused -= metadata.planes.back().bytesused; > + } > + } else if (multiPlanar) { > + /* > + * If we use the multi-planar API, fill in the planes. > + * The number of planes in the frame buffer and in the > + * V4L2 buffer is guaranteed to be equal at this point. > + */ > + for (unsigned int i = 0; i < numV4l2Planes; ++i) > + metadata.planes.push_back({ planes[i].bytesused }); > } else { > - buffer->metadata_.planes.push_back({ buf.bytesused }); > + metadata.planes.push_back({ buf.bytesused }); I was going to ask about the metadata plan allocations, but now I see that's handled in 16/30 Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > } > > return buffer; >
Hi Kieran, On Tue, Sep 07, 2021 at 12:54:15AM +0100, Kieran Bingham wrote: > On 06/09/2021 23:56, Laurent Pinchart wrote: > > When dequeueing a buffer from a V4L2VideoDevice, the number of planes in > > the FrameBuffer may not match the number of V4L2 buffer planes if the > > PixelFormat is multi-planar (has multiple colour planes) and the V4L2 > > format is single-planar (has a single buffer plane). In this case, we > > need to split the single V4L2 buffer plane into FrameBuffer planes. Do > > so, and add checks to reject invalid V4L2 buffers in case of a driver > > issue. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v1: > > > > - Reduce indentation > > --- > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++++++++++++--- > > 1 file changed, 48 insertions(+), 5 deletions(-) > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index 6c1ca9bb0a5c..2e458fcc22e0 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -7,6 +7,7 @@ > > > > #include "libcamera/internal/v4l2_videodevice.h" > > > > +#include <algorithm> > > #include <array> > > #include <fcntl.h> > > #include <iomanip> > > @@ -1669,12 +1670,54 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL > > + buf.timestamp.tv_usec * 1000ULL; > > > > - buffer->metadata_.planes.clear(); > > - if (multiPlanar) { > > - for (unsigned int nplane = 0; nplane < buf.length; nplane++) > > - buffer->metadata_.planes.push_back({ planes[nplane].bytesused }); > > + if (V4L2_TYPE_IS_OUTPUT(buf.type)) > > + return buffer; > > + > > + unsigned int numV4l2Planes = multiPlanar ? buf.length : 1; > > + FrameMetadata &metadata = buffer->metadata_; > > + metadata.planes.clear(); > > + > > + if (numV4l2Planes != buffer->planes().size()) { *1 > > + /* > > + * If we have a multi-planar buffer with a V4L2 > > + * single-planar format, split the V4L2 buffer across > > + * the buffer planes. Only the last plane may have less > > + * bytes used than its length. > > + */ > > + if (numV4l2Planes != 1) { > > + LOG(V4L2, Error) > > + << "Invalid number of planes (" << numV4l2Planes > > + << " != " << buffer->planes().size() << ")"; > > I'm weary that this error print is more about the first conditional > statement, but I think it still fits here anyway. Did you mean *1 ? I'm not sure to follow you. > > + > > + metadata.status = FrameMetadata::FrameError; > > + return buffer; > > + } > > + > > + unsigned int bytesused = multiPlanar ? planes[0].bytesused > > + : buf.bytesused; > > + > > + for (auto [i, plane] : utils::enumerate(buffer->planes())) { > > + if (!bytesused) { > > + LOG(V4L2, Error) > > + << "Dequeued buffer is too small"; > > + > > + metadata.status = FrameMetadata::FrameError; > > + return buffer; > > + } > > + > > + metadata.planes.push_back({ std::min(plane.length, bytesused) }); > > + bytesused -= metadata.planes.back().bytesused; > > + } > > + } else if (multiPlanar) { > > + /* > > + * If we use the multi-planar API, fill in the planes. > > + * The number of planes in the frame buffer and in the > > + * V4L2 buffer is guaranteed to be equal at this point. > > + */ > > + for (unsigned int i = 0; i < numV4l2Planes; ++i) > > + metadata.planes.push_back({ planes[i].bytesused }); > > } else { > > - buffer->metadata_.planes.push_back({ buf.bytesused }); > > + metadata.planes.push_back({ buf.bytesused }); > > I was going to ask about the metadata plan allocations, but now I see > that's handled in 16/30 > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > } > > > > return buffer;
Hi Laurent, On Tue, Sep 7, 2021 at 9:06 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Kieran, > > On Tue, Sep 07, 2021 at 12:54:15AM +0100, Kieran Bingham wrote: > > On 06/09/2021 23:56, Laurent Pinchart wrote: > > > When dequeueing a buffer from a V4L2VideoDevice, the number of planes in > > > the FrameBuffer may not match the number of V4L2 buffer planes if the > > > PixelFormat is multi-planar (has multiple colour planes) and the V4L2 > > > format is single-planar (has a single buffer plane). In this case, we > > > need to split the single V4L2 buffer plane into FrameBuffer planes. Do > > > so, and add checks to reject invalid V4L2 buffers in case of a driver > > > issue. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > Changes since v1: > > > > > > - Reduce indentation > > > --- > > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++++++++++++--- > > > 1 file changed, 48 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > index 6c1ca9bb0a5c..2e458fcc22e0 100644 > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > @@ -7,6 +7,7 @@ > > > > > > #include "libcamera/internal/v4l2_videodevice.h" > > > > > > +#include <algorithm> > > > #include <array> > > > #include <fcntl.h> > > > #include <iomanip> > > > @@ -1669,12 +1670,54 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() > > > buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL > > > + buf.timestamp.tv_usec * 1000ULL; > > > > > > - buffer->metadata_.planes.clear(); > > > - if (multiPlanar) { > > > - for (unsigned int nplane = 0; nplane < buf.length; nplane++) > > > - buffer->metadata_.planes.push_back({ planes[nplane].bytesused }); > > > + if (V4L2_TYPE_IS_OUTPUT(buf.type)) > > > + return buffer; > > > + > > > + unsigned int numV4l2Planes = multiPlanar ? buf.length : 1; > > > + FrameMetadata &metadata = buffer->metadata_; > > > + metadata.planes.clear(); > > > + > > > + if (numV4l2Planes != buffer->planes().size()) { > > *1 > > > > + /* > > > + * If we have a multi-planar buffer with a V4L2 > > > + * single-planar format, split the V4L2 buffer across > > > + * the buffer planes. Only the last plane may have less > > > + * bytes used than its length. > > > + */ > > > + if (numV4l2Planes != 1) { > > > + LOG(V4L2, Error) > > > + << "Invalid number of planes (" << numV4l2Planes > > > + << " != " << buffer->planes().size() << ")"; > > > > I'm weary that this error print is more about the first conditional > > statement, but I think it still fits here anyway. > > Did you mean *1 ? I'm not sure to follow you. > > > > + > > > + metadata.status = FrameMetadata::FrameError; > > > + return buffer; > > > + } > > > + > > > + unsigned int bytesused = multiPlanar ? planes[0].bytesused > > > + : buf.bytesused; > > > + > > > + for (auto [i, plane] : utils::enumerate(buffer->planes())) { > > > + if (!bytesused) { > > > + LOG(V4L2, Error) > > > + << "Dequeued buffer is too small"; > > > + > > > + metadata.status = FrameMetadata::FrameError; > > > + return buffer; > > > + } > > > + > > > + metadata.planes.push_back({ std::min(plane.length, bytesused) }); > > > + bytesused -= metadata.planes.back().bytesused; > > > + } > > > + } else if (multiPlanar) { > > > + /* > > > + * If we use the multi-planar API, fill in the planes. > > > + * The number of planes in the frame buffer and in the > > > + * V4L2 buffer is guaranteed to be equal at this point. > > > + */ > > > + for (unsigned int i = 0; i < numV4l2Planes; ++i) > > > + metadata.planes.push_back({ planes[i].bytesused }); > > > } else { > > > - buffer->metadata_.planes.push_back({ buf.bytesused }); > > > + metadata.planes.push_back({ buf.bytesused }); > > > > I was going to ask about the metadata plan allocations, but now I see > > that's handled in 16/30 > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > } > > > > > > return buffer; > > -- > Regards, > > Laurent Pinchart
On 07/09/2021 01:05, Laurent Pinchart wrote: > Hi Kieran, > > On Tue, Sep 07, 2021 at 12:54:15AM +0100, Kieran Bingham wrote: >> On 06/09/2021 23:56, Laurent Pinchart wrote: >>> When dequeueing a buffer from a V4L2VideoDevice, the number of planes in >>> the FrameBuffer may not match the number of V4L2 buffer planes if the >>> PixelFormat is multi-planar (has multiple colour planes) and the V4L2 >>> format is single-planar (has a single buffer plane). In this case, we >>> need to split the single V4L2 buffer plane into FrameBuffer planes. Do >>> so, and add checks to reject invalid V4L2 buffers in case of a driver >>> issue. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> Changes since v1: >>> >>> - Reduce indentation >>> --- >>> src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++++++++++++--- >>> 1 file changed, 48 insertions(+), 5 deletions(-) >>> >>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp >>> index 6c1ca9bb0a5c..2e458fcc22e0 100644 >>> --- a/src/libcamera/v4l2_videodevice.cpp >>> +++ b/src/libcamera/v4l2_videodevice.cpp >>> @@ -7,6 +7,7 @@ >>> >>> #include "libcamera/internal/v4l2_videodevice.h" >>> >>> +#include <algorithm> >>> #include <array> >>> #include <fcntl.h> >>> #include <iomanip> >>> @@ -1669,12 +1670,54 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() >>> buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL >>> + buf.timestamp.tv_usec * 1000ULL; >>> >>> - buffer->metadata_.planes.clear(); >>> - if (multiPlanar) { >>> - for (unsigned int nplane = 0; nplane < buf.length; nplane++) >>> - buffer->metadata_.planes.push_back({ planes[nplane].bytesused }); >>> + if (V4L2_TYPE_IS_OUTPUT(buf.type)) >>> + return buffer; >>> + >>> + unsigned int numV4l2Planes = multiPlanar ? buf.length : 1; >>> + FrameMetadata &metadata = buffer->metadata_; >>> + metadata.planes.clear(); >>> + >>> + if (numV4l2Planes != buffer->planes().size()) { > > *1 > >>> + /* >>> + * If we have a multi-planar buffer with a V4L2 >>> + * single-planar format, split the V4L2 buffer across >>> + * the buffer planes. Only the last plane may have less >>> + * bytes used than its length. >>> + */ >>> + if (numV4l2Planes != 1) { >>> + LOG(V4L2, Error) >>> + << "Invalid number of planes (" << numV4l2Planes >>> + << " != " << buffer->planes().size() << ")"; >> >> I'm weary that this error print is more about the first conditional >> statement, but I think it still fits here anyway. > > Did you mean *1 ? I'm not sure to follow you. Yes, the error print matches the conditional check of *1, but the actual error is that (numV4l2Planes != buffer->planes().size) && numV4l2Planes != 1. It will be evident enough what has happened to any reader digging into that error message, so it's fine. > >>> + >>> + metadata.status = FrameMetadata::FrameError; >>> + return buffer; >>> + } >>> + >>> + unsigned int bytesused = multiPlanar ? planes[0].bytesused >>> + : buf.bytesused; >>> + >>> + for (auto [i, plane] : utils::enumerate(buffer->planes())) { >>> + if (!bytesused) { >>> + LOG(V4L2, Error) >>> + << "Dequeued buffer is too small"; >>> + >>> + metadata.status = FrameMetadata::FrameError; >>> + return buffer; >>> + } >>> + >>> + metadata.planes.push_back({ std::min(plane.length, bytesused) }); >>> + bytesused -= metadata.planes.back().bytesused; >>> + } >>> + } else if (multiPlanar) { >>> + /* >>> + * If we use the multi-planar API, fill in the planes. >>> + * The number of planes in the frame buffer and in the >>> + * V4L2 buffer is guaranteed to be equal at this point. >>> + */ >>> + for (unsigned int i = 0; i < numV4l2Planes; ++i) >>> + metadata.planes.push_back({ planes[i].bytesused }); >>> } else { >>> - buffer->metadata_.planes.push_back({ buf.bytesused }); >>> + metadata.planes.push_back({ buf.bytesused }); >> >> I was going to ask about the metadata plan allocations, but now I see >> that's handled in 16/30 >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> } >>> >>> return buffer; >
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 6c1ca9bb0a5c..2e458fcc22e0 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -7,6 +7,7 @@ #include "libcamera/internal/v4l2_videodevice.h" +#include <algorithm> #include <array> #include <fcntl.h> #include <iomanip> @@ -1669,12 +1670,54 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer() buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL + buf.timestamp.tv_usec * 1000ULL; - buffer->metadata_.planes.clear(); - if (multiPlanar) { - for (unsigned int nplane = 0; nplane < buf.length; nplane++) - buffer->metadata_.planes.push_back({ planes[nplane].bytesused }); + if (V4L2_TYPE_IS_OUTPUT(buf.type)) + return buffer; + + unsigned int numV4l2Planes = multiPlanar ? buf.length : 1; + FrameMetadata &metadata = buffer->metadata_; + metadata.planes.clear(); + + if (numV4l2Planes != buffer->planes().size()) { + /* + * If we have a multi-planar buffer with a V4L2 + * single-planar format, split the V4L2 buffer across + * the buffer planes. Only the last plane may have less + * bytes used than its length. + */ + if (numV4l2Planes != 1) { + LOG(V4L2, Error) + << "Invalid number of planes (" << numV4l2Planes + << " != " << buffer->planes().size() << ")"; + + metadata.status = FrameMetadata::FrameError; + return buffer; + } + + unsigned int bytesused = multiPlanar ? planes[0].bytesused + : buf.bytesused; + + for (auto [i, plane] : utils::enumerate(buffer->planes())) { + if (!bytesused) { + LOG(V4L2, Error) + << "Dequeued buffer is too small"; + + metadata.status = FrameMetadata::FrameError; + return buffer; + } + + metadata.planes.push_back({ std::min(plane.length, bytesused) }); + bytesused -= metadata.planes.back().bytesused; + } + } else if (multiPlanar) { + /* + * If we use the multi-planar API, fill in the planes. + * The number of planes in the frame buffer and in the + * V4L2 buffer is guaranteed to be equal at this point. + */ + for (unsigned int i = 0; i < numV4l2Planes; ++i) + metadata.planes.push_back({ planes[i].bytesused }); } else { - buffer->metadata_.planes.push_back({ buf.bytesused }); + metadata.planes.push_back({ buf.bytesused }); } return buffer;
When dequeueing a buffer from a V4L2VideoDevice, the number of planes in the FrameBuffer may not match the number of V4L2 buffer planes if the PixelFormat is multi-planar (has multiple colour planes) and the V4L2 format is single-planar (has a single buffer plane). In this case, we need to split the single V4L2 buffer plane into FrameBuffer planes. Do so, and add checks to reject invalid V4L2 buffers in case of a driver issue. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes since v1: - Reduce indentation --- src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 5 deletions(-)