[libcamera-devel,RFC,1/3] android: generic_camera_buffer: Correct buffer mapping
diff mbox series

Message ID 20210823124321.980847-2-hiroh@chromium.org
State Accepted
Headers show
Series
  • Improve CameraBuffer implementation
Related show

Commit Message

Hirokazu Honda Aug. 23, 2021, 12:43 p.m. UTC 
buffer_hanlde_t doesn't provide sufficient info to map a buffer
properly. cros::CameraBufferManager enables handling the buffer on
ChromeOS, but no way is provided for other platforms.

Therefore, we put the assumption that planes are in the same buffer
and they are consecutive. This modifies the way of mapping in
generic_camera_buffer with the assumption.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_buffer.h              | 14 +++-
 src/android/camera_stream.cpp            |  4 +-
 src/android/mm/cros_camera_buffer.cpp    |  7 +-
 src/android/mm/generic_camera_buffer.cpp | 82 +++++++++++++++++-------
 4 files changed, 78 insertions(+), 29 deletions(-)

Comments

Laurent Pinchart Aug. 23, 2021, 9:23 p.m. UTC | #1 
Hi Hiro,

Thank you for the patch.

On Mon, Aug 23, 2021 at 09:43:19PM +0900, Hirokazu Honda wrote:
> buffer_hanlde_t doesn't provide sufficient info to map a buffer

s/buffer_hanlde_t/buffer_handle_t/

> properly. cros::CameraBufferManager enables handling the buffer on
> ChromeOS, but no way is provided for other platforms.
> 
> Therefore, we put the assumption that planes are in the same buffer
> and they are consecutive. This modifies the way of mapping in
> generic_camera_buffer with the assumption.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_buffer.h              | 14 +++-
>  src/android/camera_stream.cpp            |  4 +-
>  src/android/mm/cros_camera_buffer.cpp    |  7 +-
>  src/android/mm/generic_camera_buffer.cpp | 82 +++++++++++++++++-------
>  4 files changed, 78 insertions(+), 29 deletions(-)
> 
> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> index c4e3a9e7..87df2570 100644
> --- a/src/android/camera_buffer.h
> +++ b/src/android/camera_buffer.h
> @@ -11,13 +11,17 @@
>  
>  #include <libcamera/base/class.h>
>  #include <libcamera/base/span.h>
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
>  
>  class CameraBuffer final : public libcamera::Extensible
>  {
>  	LIBCAMERA_DECLARE_PRIVATE()
>  
>  public:
> -	CameraBuffer(buffer_handle_t camera3Buffer, int flags);
> +	CameraBuffer(buffer_handle_t camera3Buffer,
> +		     libcamera::PixelFormat pixelFormat,
> +		     const libcamera::Size &size, int flags);

I'm not thrilled by the idea of having to pass a format and size, but as
you correctly explained, buffer_handle_t isn't enough, so I don't see
any other option :-(

>  	~CameraBuffer();
>  
>  	bool isValid() const;
> @@ -31,8 +35,12 @@ public:
>  };
>  
>  #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION				\
> -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)	\
> -	: Extensible(std::make_unique<Private>(this, camera3Buffer, flags)) \
> +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer,		\
> +			   libcamera::PixelFormat pixelFormat,		\
> +			   const libcamera::Size &size, int flags)	\
> +	: Extensible(std::make_unique<Private>(this, camera3Buffer,	\
> +					       pixelFormat, size,	\
> +					       flags))			\
>  {									\
>  }									\
>  CameraBuffer::~CameraBuffer()						\
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 61b44183..01909ec7 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,
>  	 * \todo Buffer mapping and processing should be moved to a
>  	 * separate thread.
>  	 */
> -	CameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);
> +	const StreamConfiguration &output = configuration();
> +	CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
> +			  PROT_READ | PROT_WRITE);
>  	if (!dest.isValid()) {
>  		LOG(HAL, Error) << "Failed to map android blob buffer";
>  		return -EINVAL;
> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> index e8783ff8..50732637 100644
> --- a/src/android/mm/cros_camera_buffer.cpp
> +++ b/src/android/mm/cros_camera_buffer.cpp
> @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private
>  	LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)
>  
>  public:
> -	Private(CameraBuffer *cameraBuffer,
> -		buffer_handle_t camera3Buffer, int flags);
> +	Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,
> +		libcamera::PixelFormat pixelFormat, const libcamera::Size &size,
> +		int flags);
>  	~Private();
>  
>  	bool isValid() const { return valid_; }
> @@ -46,6 +47,8 @@ private:
>  
>  CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>  			       buffer_handle_t camera3Buffer,
> +			       [[maybe_unused]] libcamera::PixelFormat pixelFormat,
> +			       [[maybe_unused]] const libcamera::Size &size,
>  			       [[maybe_unused]] int flags)
>  	: handle_(camera3Buffer), numPlanes_(0), valid_(false),
>  	  registered_(false)
> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> index b3af194c..3127069f 100644
> --- a/src/android/mm/generic_camera_buffer.cpp
> +++ b/src/android/mm/generic_camera_buffer.cpp
> @@ -12,6 +12,7 @@
>  
>  #include <libcamera/base/log.h>
>  
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/mapped_framebuffer.h"
>  
>  using namespace libcamera;
> @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,
>  	LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)
>  
>  public:
> -	Private(CameraBuffer *cameraBuffer,
> -		buffer_handle_t camera3Buffer, int flags);
> +	Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,
> +		libcamera::PixelFormat pixelFormat, const libcamera::Size &size,
> +		int flags);
>  	~Private();
>  
>  	unsigned int numPlanes() const;
> @@ -33,35 +35,69 @@ public:
>  	Span<uint8_t> plane(unsigned int plane);
>  
>  	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
> +
> +private:
> +	/* \todo remove planes_ is added to MappedBuffer. */

I'm not sure to follow you. Do you mean "Remove planes_ when it will be
added to MappedBuffer" maybe ?

> +	std::vector<Span<uint8_t>> planes_;
>  };
>  
>  CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> -			       buffer_handle_t camera3Buffer, int flags)
> +			       buffer_handle_t camera3Buffer,
> +			       libcamera::PixelFormat pixelFormat,
> +			       const libcamera::Size &size, int flags)
>  {
> -	maps_.reserve(camera3Buffer->numFds);
>  	error_ = 0;
>  

I'd add a comment here:

	/*
	 * As Android doesn't offer an API to query buffer layouts, assume for
	 * now that the buffer is backed by a single dmabuf, with planes being
	 * stored contiguously.
	 */

> +	int fd = -1;
>  	for (int i = 0; i < camera3Buffer->numFds; i++) {
> -		if (camera3Buffer->data[i] == -1)
> -			continue;
> -
> -		off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);
> -		if (length < 0) {
> -			error_ = -errno;
> -			LOG(HAL, Error) << "Failed to query plane length";
> +		if (camera3Buffer->data[i] != -1) {
> +			fd = camera3Buffer->data[i];
>  			break;
>  		}
> +	}

Could we verify the assumption ? Maybe something like

	int fd = -1;
	for (int i = 0; i < camera3Buffer->numFds; i++) {
		if (camera3Buffer->data[i] == -1 ||
		    camera3Buffer->data[i] == fd)
			continue;

		if (fd != -1)
			LOG(HAL, Fatal)
				<< "Discontiguous planes are not supported";
		fd = camera3Buffer->data[i];
	}

>  
> -		void *address = mmap(nullptr, length, flags, MAP_SHARED,
> -				     camera3Buffer->data[i], 0);
> -		if (address == MAP_FAILED) {
> -			error_ = -errno;
> -			LOG(HAL, Error) << "Failed to mmap plane";
> -			break;
> -		}
> +	if (fd != -1) {

That should be

	if (fd == -1) {

> +		error_ = EINVAL;

-EINVAL

> +		LOG(HAL, Error) << "No valid file descriptor";
> +		return;
> +	}
> +
> +	const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);
> +	if (!info.isValid()) {
> +		error_ = EINVAL;

-EINVAL

> +		LOG(HAL, Error) << "Invalid pixel format: "
> +				<< pixelFormat.toString();
> +		return;
> +	}
> +
> +	size_t bufferLength = lseek(fd, 0, SEEK_END);
> +	if (bufferLength < 0) {

This comparison is always false, as size_t is unsigned. The correct data
type if off_t for the return value of lseek.

> +		error_ = -errno;
> +		LOG(HAL, Error) << "Failed to get buffer length";
> +		return;
> +	}
> +
> +	void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);
> +	if (address == MAP_FAILED) {
> +		error_ = -errno;
> +		LOG(HAL, Error) << "Failed to mmap plane";
> +		return;
> +	}
> +	maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
> +
> +	const unsigned int numPlanes = info.numPlanes();
> +	planes_.resize(numPlanes);
> +	unsigned int offset = 0;
> +	for (unsigned int i = 0; i < numPlanes; ++i) {
> +		const unsigned int vertSubSample = info.planes[i].verticalSubSampling;
> +		const unsigned int stride = info.stride(size.width, i, 1u);
> +		const unsigned int planeSize =
> +			stride * ((size.height + vertSubSample - 1) / vertSubSample);

It may make sense to move this calculation to a new helper function in
libcamera::PixelFormatInfo at some point, but it's fine for now.

> +
> +		planes_[i] = libcamera::Span<uint8_t>(
> +			static_cast<uint8_t *>(address) + offset, planeSize);
>  
> -		maps_.emplace_back(static_cast<uint8_t *>(address),
> -				   static_cast<size_t>(length));
> +		offset += planeSize;
>  	}
>  }
>  
> @@ -71,15 +107,15 @@ CameraBuffer::Private::~Private()
>  
>  unsigned int CameraBuffer::Private::numPlanes() const
>  {
> -	return maps_.size();
> +	return planes_.size();
>  }
>  
>  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
>  {
> -	if (plane >= maps_.size())
> +	if (plane >= planes_.size())
>  		return {};
>  
> -	return maps_[plane];
> +	return planes_[plane];
>  }
>  
>  size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
Tomasz Figa Aug. 27, 2021, 9:33 a.m. UTC | #2 
On Tue, Aug 24, 2021 at 6:23 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Mon, Aug 23, 2021 at 09:43:19PM +0900, Hirokazu Honda wrote:
> > buffer_hanlde_t doesn't provide sufficient info to map a buffer
>
> s/buffer_hanlde_t/buffer_handle_t/
>
> > properly. cros::CameraBufferManager enables handling the buffer on
> > ChromeOS, but no way is provided for other platforms.
> >
> > Therefore, we put the assumption that planes are in the same buffer
> > and they are consecutive. This modifies the way of mapping in
> > generic_camera_buffer with the assumption.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/android/camera_buffer.h              | 14 +++-
> >  src/android/camera_stream.cpp            |  4 +-
> >  src/android/mm/cros_camera_buffer.cpp    |  7 +-
> >  src/android/mm/generic_camera_buffer.cpp | 82 +++++++++++++++++-------
> >  4 files changed, 78 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> > index c4e3a9e7..87df2570 100644
> > --- a/src/android/camera_buffer.h
> > +++ b/src/android/camera_buffer.h
> > @@ -11,13 +11,17 @@
> >
> >  #include <libcamera/base/class.h>
> >  #include <libcamera/base/span.h>
> > +#include <libcamera/geometry.h>
> > +#include <libcamera/pixel_format.h>
> >
> >  class CameraBuffer final : public libcamera::Extensible
> >  {
> >       LIBCAMERA_DECLARE_PRIVATE()
> >
> >  public:
> > -     CameraBuffer(buffer_handle_t camera3Buffer, int flags);
> > +     CameraBuffer(buffer_handle_t camera3Buffer,
> > +                  libcamera::PixelFormat pixelFormat,
> > +                  const libcamera::Size &size, int flags);
>
> I'm not thrilled by the idea of having to pass a format and size, but as
> you correctly explained, buffer_handle_t isn't enough, so I don't see
> any other option :-(
>
> >       ~CameraBuffer();
> >
> >       bool isValid() const;
> > @@ -31,8 +35,12 @@ public:
> >  };
> >
> >  #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION                          \
> > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \
> > -     : Extensible(std::make_unique<Private>(this, camera3Buffer, flags)) \
> > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer,            \
> > +                        libcamera::PixelFormat pixelFormat,          \
> > +                        const libcamera::Size &size, int flags)      \
> > +     : Extensible(std::make_unique<Private>(this, camera3Buffer,     \
> > +                                            pixelFormat, size,       \
> > +                                            flags))                  \
> >  {                                                                    \
> >  }                                                                    \
> >  CameraBuffer::~CameraBuffer()                                                \
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 61b44183..01909ec7 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,
> >        * \todo Buffer mapping and processing should be moved to a
> >        * separate thread.
> >        */
> > -     CameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);
> > +     const StreamConfiguration &output = configuration();
> > +     CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
> > +                       PROT_READ | PROT_WRITE);
> >       if (!dest.isValid()) {
> >               LOG(HAL, Error) << "Failed to map android blob buffer";
> >               return -EINVAL;
> > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> > index e8783ff8..50732637 100644
> > --- a/src/android/mm/cros_camera_buffer.cpp
> > +++ b/src/android/mm/cros_camera_buffer.cpp
> > @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private
> >       LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)
> >
> >  public:
> > -     Private(CameraBuffer *cameraBuffer,
> > -             buffer_handle_t camera3Buffer, int flags);
> > +     Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,
> > +             libcamera::PixelFormat pixelFormat, const libcamera::Size &size,
> > +             int flags);
> >       ~Private();
> >
> >       bool isValid() const { return valid_; }
> > @@ -46,6 +47,8 @@ private:
> >
> >  CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> >                              buffer_handle_t camera3Buffer,
> > +                            [[maybe_unused]] libcamera::PixelFormat pixelFormat,
> > +                            [[maybe_unused]] const libcamera::Size &size,
> >                              [[maybe_unused]] int flags)
> >       : handle_(camera3Buffer), numPlanes_(0), valid_(false),
> >         registered_(false)
> > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> > index b3af194c..3127069f 100644
> > --- a/src/android/mm/generic_camera_buffer.cpp
> > +++ b/src/android/mm/generic_camera_buffer.cpp
> > @@ -12,6 +12,7 @@
> >
> >  #include <libcamera/base/log.h>
> >
> > +#include "libcamera/internal/formats.h"
> >  #include "libcamera/internal/mapped_framebuffer.h"
> >
> >  using namespace libcamera;
> > @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,
> >       LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)
> >
> >  public:
> > -     Private(CameraBuffer *cameraBuffer,
> > -             buffer_handle_t camera3Buffer, int flags);
> > +     Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,
> > +             libcamera::PixelFormat pixelFormat, const libcamera::Size &size,
> > +             int flags);
> >       ~Private();
> >
> >       unsigned int numPlanes() const;
> > @@ -33,35 +35,69 @@ public:
> >       Span<uint8_t> plane(unsigned int plane);
> >
> >       size_t jpegBufferSize(size_t maxJpegBufferSize) const;
> > +
> > +private:
> > +     /* \todo remove planes_ is added to MappedBuffer. */
>
> I'm not sure to follow you. Do you mean "Remove planes_ when it will be
> added to MappedBuffer" maybe ?
>
> > +     std::vector<Span<uint8_t>> planes_;
> >  };
> >
> >  CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> > -                            buffer_handle_t camera3Buffer, int flags)
> > +                            buffer_handle_t camera3Buffer,
> > +                            libcamera::PixelFormat pixelFormat,
> > +                            const libcamera::Size &size, int flags)
> >  {
> > -     maps_.reserve(camera3Buffer->numFds);
> >       error_ = 0;
> >
>
> I'd add a comment here:
>
>         /*
>          * As Android doesn't offer an API to query buffer layouts, assume for
>          * now that the buffer is backed by a single dmabuf, with planes being
>          * stored contiguously.
>          */
>
> > +     int fd = -1;
> >       for (int i = 0; i < camera3Buffer->numFds; i++) {
> > -             if (camera3Buffer->data[i] == -1)
> > -                     continue;
> > -
> > -             off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);
> > -             if (length < 0) {
> > -                     error_ = -errno;
> > -                     LOG(HAL, Error) << "Failed to query plane length";
> > +             if (camera3Buffer->data[i] != -1) {
> > +                     fd = camera3Buffer->data[i];
> >                       break;
> >               }
> > +     }
>
> Could we verify the assumption ? Maybe something like
>
>         int fd = -1;
>         for (int i = 0; i < camera3Buffer->numFds; i++) {
>                 if (camera3Buffer->data[i] == -1 ||
>                     camera3Buffer->data[i] == fd)
>                         continue;
>
>                 if (fd != -1)
>                         LOG(HAL, Fatal)
>                                 << "Discontiguous planes are not supported";
>                 fd = camera3Buffer->data[i];
>         }
>

If numFds > 1, generally the FDs will be different, because the handle
is sent over Binder, which would create new FDs for each array element
when unflattening the handle on the receiver side.

> >
> > -             void *address = mmap(nullptr, length, flags, MAP_SHARED,
> > -                                  camera3Buffer->data[i], 0);
> > -             if (address == MAP_FAILED) {
> > -                     error_ = -errno;
> > -                     LOG(HAL, Error) << "Failed to mmap plane";
> > -                     break;
> > -             }
> > +     if (fd != -1) {
>
> That should be
>
>         if (fd == -1) {
>
> > +             error_ = EINVAL;
>
> -EINVAL
>
> > +             LOG(HAL, Error) << "No valid file descriptor";
> > +             return;
> > +     }
> > +
> > +     const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);
> > +     if (!info.isValid()) {
> > +             error_ = EINVAL;
>
> -EINVAL
>
> > +             LOG(HAL, Error) << "Invalid pixel format: "
> > +                             << pixelFormat.toString();
> > +             return;
> > +     }
> > +
> > +     size_t bufferLength = lseek(fd, 0, SEEK_END);
> > +     if (bufferLength < 0) {
>
> This comparison is always false, as size_t is unsigned. The correct data
> type if off_t for the return value of lseek.

Is perhaps the auto type the right thing to use to avoid issues like this?

Best regards,
Tomasz

>
> > +             error_ = -errno;
> > +             LOG(HAL, Error) << "Failed to get buffer length";
> > +             return;
> > +     }
> > +
> > +     void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);
> > +     if (address == MAP_FAILED) {
> > +             error_ = -errno;
> > +             LOG(HAL, Error) << "Failed to mmap plane";
> > +             return;
> > +     }
> > +     maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
> > +
> > +     const unsigned int numPlanes = info.numPlanes();
> > +     planes_.resize(numPlanes);
> > +     unsigned int offset = 0;
> > +     for (unsigned int i = 0; i < numPlanes; ++i) {
> > +             const unsigned int vertSubSample = info.planes[i].verticalSubSampling;
> > +             const unsigned int stride = info.stride(size.width, i, 1u);
> > +             const unsigned int planeSize =
> > +                     stride * ((size.height + vertSubSample - 1) / vertSubSample);
>
> It may make sense to move this calculation to a new helper function in
> libcamera::PixelFormatInfo at some point, but it's fine for now.
>
> > +
> > +             planes_[i] = libcamera::Span<uint8_t>(
> > +                     static_cast<uint8_t *>(address) + offset, planeSize);
> >
> > -             maps_.emplace_back(static_cast<uint8_t *>(address),
> > -                                static_cast<size_t>(length));
> > +             offset += planeSize;
> >       }
> >  }
> >
> > @@ -71,15 +107,15 @@ CameraBuffer::Private::~Private()
> >
> >  unsigned int CameraBuffer::Private::numPlanes() const
> >  {
> > -     return maps_.size();
> > +     return planes_.size();
> >  }
> >
> >  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> >  {
> > -     if (plane >= maps_.size())
> > +     if (plane >= planes_.size())
> >               return {};
> >
> > -     return maps_[plane];
> > +     return planes_[plane];
> >  }
> >
> >  size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
index c4e3a9e7..87df2570 100644
--- a/src/android/camera_buffer.h
+++ b/src/android/camera_buffer.h
@@ -11,13 +11,17 @@ 
 
 #include <libcamera/base/class.h>
 #include <libcamera/base/span.h>
+#include <libcamera/geometry.h>
+#include <libcamera/pixel_format.h>
 
 class CameraBuffer final : public libcamera::Extensible
 {
 	LIBCAMERA_DECLARE_PRIVATE()
 
 public:
-	CameraBuffer(buffer_handle_t camera3Buffer, int flags);
+	CameraBuffer(buffer_handle_t camera3Buffer,
+		     libcamera::PixelFormat pixelFormat,
+		     const libcamera::Size &size, int flags);
 	~CameraBuffer();
 
 	bool isValid() const;
@@ -31,8 +35,12 @@  public:
 };
 
 #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION				\
-CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)	\
-	: Extensible(std::make_unique<Private>(this, camera3Buffer, flags)) \
+CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer,		\
+			   libcamera::PixelFormat pixelFormat,		\
+			   const libcamera::Size &size, int flags)	\
+	: Extensible(std::make_unique<Private>(this, camera3Buffer,	\
+					       pixelFormat, size,	\
+					       flags))			\
 {									\
 }									\
 CameraBuffer::~CameraBuffer()						\
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index 61b44183..01909ec7 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -110,7 +110,9 @@  int CameraStream::process(const libcamera::FrameBuffer &source,
 	 * \todo Buffer mapping and processing should be moved to a
 	 * separate thread.
 	 */
-	CameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);
+	const StreamConfiguration &output = configuration();
+	CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
+			  PROT_READ | PROT_WRITE);
 	if (!dest.isValid()) {
 		LOG(HAL, Error) << "Failed to map android blob buffer";
 		return -EINVAL;
diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
index e8783ff8..50732637 100644
--- a/src/android/mm/cros_camera_buffer.cpp
+++ b/src/android/mm/cros_camera_buffer.cpp
@@ -20,8 +20,9 @@  class CameraBuffer::Private : public Extensible::Private
 	LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)
 
 public:
-	Private(CameraBuffer *cameraBuffer,
-		buffer_handle_t camera3Buffer, int flags);
+	Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,
+		libcamera::PixelFormat pixelFormat, const libcamera::Size &size,
+		int flags);
 	~Private();
 
 	bool isValid() const { return valid_; }
@@ -46,6 +47,8 @@  private:
 
 CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
 			       buffer_handle_t camera3Buffer,
+			       [[maybe_unused]] libcamera::PixelFormat pixelFormat,
+			       [[maybe_unused]] const libcamera::Size &size,
 			       [[maybe_unused]] int flags)
 	: handle_(camera3Buffer), numPlanes_(0), valid_(false),
 	  registered_(false)
diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
index b3af194c..3127069f 100644
--- a/src/android/mm/generic_camera_buffer.cpp
+++ b/src/android/mm/generic_camera_buffer.cpp
@@ -12,6 +12,7 @@ 
 
 #include <libcamera/base/log.h>
 
+#include "libcamera/internal/formats.h"
 #include "libcamera/internal/mapped_framebuffer.h"
 
 using namespace libcamera;
@@ -24,8 +25,9 @@  class CameraBuffer::Private : public Extensible::Private,
 	LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)
 
 public:
-	Private(CameraBuffer *cameraBuffer,
-		buffer_handle_t camera3Buffer, int flags);
+	Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,
+		libcamera::PixelFormat pixelFormat, const libcamera::Size &size,
+		int flags);
 	~Private();
 
 	unsigned int numPlanes() const;
@@ -33,35 +35,69 @@  public:
 	Span<uint8_t> plane(unsigned int plane);
 
 	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
+
+private:
+	/* \todo remove planes_ is added to MappedBuffer. */
+	std::vector<Span<uint8_t>> planes_;
 };
 
 CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
-			       buffer_handle_t camera3Buffer, int flags)
+			       buffer_handle_t camera3Buffer,
+			       libcamera::PixelFormat pixelFormat,
+			       const libcamera::Size &size, int flags)
 {
-	maps_.reserve(camera3Buffer->numFds);
 	error_ = 0;
 
+	int fd = -1;
 	for (int i = 0; i < camera3Buffer->numFds; i++) {
-		if (camera3Buffer->data[i] == -1)
-			continue;
-
-		off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);
-		if (length < 0) {
-			error_ = -errno;
-			LOG(HAL, Error) << "Failed to query plane length";
+		if (camera3Buffer->data[i] != -1) {
+			fd = camera3Buffer->data[i];
 			break;
 		}
+	}
 
-		void *address = mmap(nullptr, length, flags, MAP_SHARED,
-				     camera3Buffer->data[i], 0);
-		if (address == MAP_FAILED) {
-			error_ = -errno;
-			LOG(HAL, Error) << "Failed to mmap plane";
-			break;
-		}
+	if (fd != -1) {
+		error_ = EINVAL;
+		LOG(HAL, Error) << "No valid file descriptor";
+		return;
+	}
+
+	const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);
+	if (!info.isValid()) {
+		error_ = EINVAL;
+		LOG(HAL, Error) << "Invalid pixel format: "
+				<< pixelFormat.toString();
+		return;
+	}
+
+	size_t bufferLength = lseek(fd, 0, SEEK_END);
+	if (bufferLength < 0) {
+		error_ = -errno;
+		LOG(HAL, Error) << "Failed to get buffer length";
+		return;
+	}
+
+	void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);
+	if (address == MAP_FAILED) {
+		error_ = -errno;
+		LOG(HAL, Error) << "Failed to mmap plane";
+		return;
+	}
+	maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
+
+	const unsigned int numPlanes = info.numPlanes();
+	planes_.resize(numPlanes);
+	unsigned int offset = 0;
+	for (unsigned int i = 0; i < numPlanes; ++i) {
+		const unsigned int vertSubSample = info.planes[i].verticalSubSampling;
+		const unsigned int stride = info.stride(size.width, i, 1u);
+		const unsigned int planeSize =
+			stride * ((size.height + vertSubSample - 1) / vertSubSample);
+
+		planes_[i] = libcamera::Span<uint8_t>(
+			static_cast<uint8_t *>(address) + offset, planeSize);
 
-		maps_.emplace_back(static_cast<uint8_t *>(address),
-				   static_cast<size_t>(length));
+		offset += planeSize;
 	}
 }
 
@@ -71,15 +107,15 @@  CameraBuffer::Private::~Private()
 
 unsigned int CameraBuffer::Private::numPlanes() const
 {
-	return maps_.size();
+	return planes_.size();
 }
 
 Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
 {
-	if (plane >= maps_.size())
+	if (plane >= planes_.size())
 		return {};
 
-	return maps_[plane];
+	return planes_[plane];
 }
 
 size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const