[libcamera-devel] [RFC PATCH 00/10] Add offset to FrameBuffer::Plane

[Laurent Pinchart]

Hi Hiro,

On Mon, Aug 16, 2021 at 01:31:28PM +0900, Hirokazu Honda wrote:
> Since FrameBuffer::Plane doesn't have an offset variable, it is
> impossible to map the FrameBuffer::Plane correctly. This series fixes
> the issue by adding an offset and alignes some code of mapping and
> creating FrameBuffer::Plane with the new offset.
> 
> I probably miss some code of using FrameBuffer::Plane. To detect the
> bug, I add the assertion to FrameBuffer.

Nice patch series ! Thanks a lot.

Overall, I think this is good. I've made comments to individual patches,
and none change the direction.

There's one part that makes me feel a bit uneasy though. With this
series applied, we're moving to better support of multi-planar formats.
We don't support all possible options, for instance, we don't support
the V4L2_PIX_FMT_NV12M format, but that's fine, there's no need to
implement everything in one go. However, it would be nice if you could
summarize somewhere, maybe in the cover letter, what is expected to be
supported (e.g. allocating FrameBuffer from a V4L2VideDevice for both
the single-planar and multi-planar V4L2 APIs, for the single V4L2 plane
formats (i.e. not NV12M) only, or queuing to V4L2VideoDevice a
multi-planar FrameBuffer with all planes referring to the same dmabuf)
and what isn't supported (e.g. queuing to a V4L2VideDevice a
multi-planar FrameBuffer with different dmabufs, or with non-default
offsets). It would help reviewing the next version, to ensure the
implementation matches the expectations.

> Hirokazu Honda (10):
>   libcamera: framebuffer: Add offset to FrameBuffer::Plane
>   libcamera: ipa_data_serializer: Modify (de)serialization for offset
>   libcamera: mapped_framebuffer: Return plane begin address by
>     MappedBuffer::maps()
>   cam: file_sink: Use offset in mapping FrameBuffer
>   ipa: rkisp1: Use offset in mapping IPABuffer
>   qcam: main_window: Use offset mapping FrameBuffer
>   gstreamer: gstlibcameraallocator: Use offset in creating a buffer
>   libcamera: v4l2_videodevice: Create color-format planes in
>     CreateBuffer()
>   android: camera_device: Fill offset and right length in
>     CreateFrameBuffer()
>   libcamera: framebuffer: Add assertion to detect offset is unfilled
> 
>  include/libcamera/framebuffer.h               | 12 ++++-
>  .../libcamera/internal/mapped_framebuffer.h   |  4 +-
>  include/libcamera/internal/v4l2_videodevice.h |  4 +-
>  src/android/camera_device.cpp                 | 38 +++++++-------
>  src/cam/file_sink.cpp                         |  5 +-
>  src/cam/file_sink.h                           |  1 +
>  src/gstreamer/gstlibcameraallocator.cpp       |  3 +-
>  src/ipa/rkisp1/rkisp1.cpp                     |  4 +-
>  src/libcamera/framebuffer.cpp                 | 11 ++--
>  src/libcamera/ipa_data_serializer.cpp         |  5 +-
>  src/libcamera/mapped_framebuffer.cpp          | 51 +++++++++++++++----
>  src/libcamera/v4l2_videodevice.cpp            | 28 +++++++++-
>  src/qcam/main_window.cpp                      | 15 ++++--
>  src/qcam/main_window.h                        |  1 +
>  14 files changed, 136 insertions(+), 46 deletions(-)

-- 
Regards,

Laurent Pinchart