[libcamera-devel] [PATCH] cam: Limit file write to payload size

[Laurent Pinchart]

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 at 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 at ideasonboard.com>
> > 
> > >  
> > >  		ret = ::write(fd, data, length);
> > >  		if (ret < 0) {
> > > 

-- 
Regards,

Laurent Pinchart