[libcamera-devel,RFC,00/10] Add offset to FrameBuffer::Plane
mbox series

Message ID 20210816043138.957984-1-hiroh@chromium.org
Headers show
Series
  • Add offset to FrameBuffer::Plane
Related show

Message

Hirokazu Honda Aug. 16, 2021, 4:31 a.m. UTC 
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.

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(-)

--
2.33.0.rc1.237.g0d66db33f3-goog

Comments

Laurent Pinchart Aug. 18, 2021, 1:59 a.m. UTC | #1 
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(-)