[libcamera-devel] [RFC PATCH 7/8] android: camera_device: Query plane length

[Laurent Pinchart]

Hi Kieran,

(CC'ing Tomasz to get a review from a Chrome OS point of view)

Thank you for the patch.

On Mon, Jul 20, 2020 at 11:42:31PM +0100, Kieran Bingham wrote:
> Use lseek to query the length of planes where possible rather than leaving
> the plane.length as zero, which prevents mapping buffers for software
> processing.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> 
> I like this one ;-) We need to know the plane lengths for software
> streams now, but the correct way to get this would be to talk to the
> gralloc allocator. This would mean pulling in various extra
> cros-specific libraries, where instead we can query the size of the
> dmabuf by getting the offset when we SEEK_END.
> 
> 
>  src/android/camera_device.cpp | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 538b8ab5da03..6212ccdd61ec 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
>  			continue;
>  		}
>  
> -		/*
> -		 * Setting length to zero here is OK as the length is only used
> -		 * to map the memory of the plane. Libcamera do not need to poke
> -		 * at the memory content queued by the HAL.
> -		 */
> -		plane.length = 0;
> +		off_t length = lseek(plane.fd.fd(), 0, SEEK_END);
> +		if (length == -1) {
> +			/*
> +			 * A zero length plane is not fatal unless the
> +			 * FrameBuffer is used for a software stream, libcamera
> +			 * itself will not access the internal frame content.
> +			 */
> +			LOG(HAL, Error) << "Failed to query plane length";
> +			length = 0;

Do you expect this to happen ? If not, I'd return an error. If yes, an
error message would be printed in a loop, which isn't very nice. My
preference would be to return an error if possible.

Otherwise, nice hack :-)

> +		}
> +
> +		plane.length = length;
>  		planes.push_back(std::move(plane));
>  	}
>  

-- 
Regards,

Laurent Pinchart