| Message ID | 20200822134036.16209-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 22/08/2020 14:40, Laurent Pinchart wrote: > The payload size in a captured framebuffer is usually equal to the > buffer size. However, for compressed formats, the payload may be > smaller Only write the payload when capturing to a file. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/cam/buffer_writer.cpp | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp > index c5a5eb46224a..6305958924a4 100644 > --- a/src/cam/buffer_writer.cpp > +++ b/src/cam/buffer_writer.cpp > @@ -64,9 +64,17 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName) > if (fd == -1) > return -errno; > > - for (const FrameBuffer::Plane &plane : buffer->planes()) { > + 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]; > + > void *data = mappedBuffers_[plane.fd.fd()].first; > - unsigned int length = plane.length; > + unsigned int length = std::min(meta.bytesused, plane.length); > + > + if (length < meta.bytesused) > + std::cerr << "payload size " << meta.bytesused > + << " smaller than plane size " << plane.length > + << std::endl; Is the cerr necessary? If saving out an MJPEG stream from an MJPEG UVC camera - this would print every frame... Which is however the use case that makes me believe this patch is useful... So with that considered either way, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > ret = ::write(fd, data, length); > if (ret < 0) { >
Hi Kieran, Laurent, On 2020-08-24 10:32:24 +0100, Kieran Bingham wrote: > Hi Laurent, > > On 22/08/2020 14:40, Laurent Pinchart wrote: > > The payload size in a captured framebuffer is usually equal to the > > buffer size. However, for compressed formats, the payload may be > > smaller Only write the payload when capturing to a file. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/cam/buffer_writer.cpp | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp > > index c5a5eb46224a..6305958924a4 100644 > > --- a/src/cam/buffer_writer.cpp > > +++ b/src/cam/buffer_writer.cpp > > @@ -64,9 +64,17 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName) > > if (fd == -1) > > return -errno; > > > > - for (const FrameBuffer::Plane &plane : buffer->planes()) { > > + 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]; > > + > > void *data = mappedBuffers_[plane.fd.fd()].first; > > - unsigned int length = plane.length; > > + unsigned int length = std::min(meta.bytesused, plane.length); > > + > > + if (length < meta.bytesused) > > + std::cerr << "payload size " << meta.bytesused > > + << " smaller than plane size " << plane.length > > + << std::endl; > > Is the cerr necessary? If saving out an MJPEG stream from an MJPEG UVC > camera - this would print every frame... Now I'm confused :-) Would not the cerr only print if length is set to plane.length while meta.bytesused is larger? I read this as plane.length being the buffer's max allocated size while if the cerr triggers the driver is reporitng that more then the max size is used as bytesused is larger then the plane? > > Which is however the use case that makes me believe this patch is useful... > > So with that considered either way, > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > ret = ::write(fd, data, length); > > if (ret < 0) { > > > > -- > Regards > -- > Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Tue, Aug 25, 2020 at 12:04:11AM +0200, Niklas Söderlund wrote: > On 2020-08-24 10:32:24 +0100, Kieran Bingham wrote: > > On 22/08/2020 14:40, Laurent Pinchart wrote: > > > The payload size in a captured framebuffer is usually equal to the > > > buffer size. However, for compressed formats, the payload may be > > > smaller Only write the payload when capturing to a file. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/cam/buffer_writer.cpp | 12 ++++++++++-- > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp > > > index c5a5eb46224a..6305958924a4 100644 > > > --- a/src/cam/buffer_writer.cpp > > > +++ b/src/cam/buffer_writer.cpp > > > @@ -64,9 +64,17 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName) > > > if (fd == -1) > > > return -errno; > > > > > > - for (const FrameBuffer::Plane &plane : buffer->planes()) { > > > + 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]; > > > + > > > void *data = mappedBuffers_[plane.fd.fd()].first; > > > - unsigned int length = plane.length; > > > + unsigned int length = std::min(meta.bytesused, plane.length); > > > + > > > + if (length < meta.bytesused) > > > + std::cerr << "payload size " << meta.bytesused > > > + << " smaller than plane size " << plane.length > > > + << std::endl; > > > > Is the cerr necessary? If saving out an MJPEG stream from an MJPEG UVC > > camera - this would print every frame... > > Now I'm confused :-) > > Would not the cerr only print if length is set to plane.length while > meta.bytesused is larger? I read this as plane.length being the buffer's > max allocated size while if the cerr triggers the driver is reporitng > that more then the max size is used as bytesused is larger then the > plane? The check is correct, but the message isn't. And the check could actually be be made clearer. if (meta.bytesused > plane.length) std::cerr << "payload size " << meta.bytesused << " larger than plane size " << plane.length << std::endl; > > Which is however the use case that makes me believe this patch is useful... > > > > So with that considered either way, > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > ret = ::write(fd, data, length); > > > if (ret < 0) { > > >
On 24/08/2020 23:53, Laurent Pinchart wrote: > On Tue, Aug 25, 2020 at 12:04:11AM +0200, Niklas Söderlund wrote: >> On 2020-08-24 10:32:24 +0100, Kieran Bingham wrote: >>> On 22/08/2020 14:40, Laurent Pinchart wrote: >>>> The payload size in a captured framebuffer is usually equal to the >>>> buffer size. However, for compressed formats, the payload may be >>>> smaller Only write the payload when capturing to a file. >>>> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> --- >>>> src/cam/buffer_writer.cpp | 12 ++++++++++-- >>>> 1 file changed, 10 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp >>>> index c5a5eb46224a..6305958924a4 100644 >>>> --- a/src/cam/buffer_writer.cpp >>>> +++ b/src/cam/buffer_writer.cpp >>>> @@ -64,9 +64,17 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName) >>>> if (fd == -1) >>>> return -errno; >>>> >>>> - for (const FrameBuffer::Plane &plane : buffer->planes()) { >>>> + 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]; >>>> + >>>> void *data = mappedBuffers_[plane.fd.fd()].first; >>>> - unsigned int length = plane.length; >>>> + unsigned int length = std::min(meta.bytesused, plane.length); >>>> + >>>> + if (length < meta.bytesused) >>>> + std::cerr << "payload size " << meta.bytesused >>>> + << " smaller than plane size " << plane.length >>>> + << std::endl; >>> >>> Is the cerr necessary? If saving out an MJPEG stream from an MJPEG UVC >>> camera - this would print every frame... >> >> Now I'm confused :-) >> >> Would not the cerr only print if length is set to plane.length while >> meta.bytesused is larger? I read this as plane.length being the buffer's >> max allocated size while if the cerr triggers the driver is reporitng >> that more then the max size is used as bytesused is larger then the >> plane? > > The check is correct, but the message isn't. And the check could > actually be be made clearer. > > if (meta.bytesused > plane.length) > std::cerr << "payload size " << meta.bytesused > << " larger than plane size " << plane.length > << std::endl; Ahh, now that would be a lot easier to parse. +1 -- Kieran > >>> Which is however the use case that makes me believe this patch is useful... >>> >>> So with that considered either way, >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>>> >>>> ret = ::write(fd, data, length); >>>> if (ret < 0) { >>>> >
diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp index c5a5eb46224a..6305958924a4 100644 --- a/src/cam/buffer_writer.cpp +++ b/src/cam/buffer_writer.cpp @@ -64,9 +64,17 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName) if (fd == -1) return -errno; - for (const FrameBuffer::Plane &plane : buffer->planes()) { + 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]; + void *data = mappedBuffers_[plane.fd.fd()].first; - unsigned int length = plane.length; + unsigned int length = std::min(meta.bytesused, plane.length); + + if (length < meta.bytesused) + std::cerr << "payload size " << meta.bytesused + << " smaller than plane size " << plane.length + << std::endl; ret = ::write(fd, data, length); if (ret < 0) {
The payload size in a captured framebuffer is usually equal to the buffer size. However, for compressed formats, the payload may be smaller Only write the payload when capturing to a file. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/cam/buffer_writer.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)