| Message ID | 20210826112539.170694-1-hiroh@chromium.org |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, On Thu, Aug 26, 2021 at 08:25:30PM +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 aligns some code of mapping and > creating FrameBuffer::Plane with the new offset. > > One of the pateches fixes V4L2Device to make a returned FrameBuffer > color format planes from v4l2 format planes. However, V4L2 API doesn't > provide a way of getting a dmabuf plane offset, so a multi planar > format e.g. V4L2_PIX_FMT_NV12M is not supported with this patch series. > > This series must be applied on top of the pacth series whose id is 2384. > cf.) https://patchwork.libcamera.org/project/libcamera/list/?series=2384 Very nice series. I've replied with a few minor comments on patches 1/9 to 6/9, and 8/9 to 9/9. I can handle those when pushing the series. Patch 7/9 is still problematic, as it breaks a unit test. If it's the only patch that needs a new version, you can resubmit that one only (for instance as v3.1, in a reply to v3) if it's easier for you. If you prefer sending a v4 of the whole series, please address the minor comments on the other patches as well. > Change in v3: > - address review comments to v2 > - squash 01/10 and 02/10 (serialization change) of v2 > > Change in v2: > - adress review comments to v1 > > Hirokazu Honda (9): > libcamera: framebuffer: Add offset to FrameBuffer::Plane > 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 > V4L2VideDevice::createBuffer() creates the same number of > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > format single is single-planar format, the created number of > FrameBuffer::Planes is 1. It should rather create the same number of > FrameBuffer::Planes as the color format planes. > 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 | 10 +- > src/android/camera_device.cpp | 52 ++++--- > src/android/camera_device.h | 6 +- > src/android/mm/generic_camera_buffer.cpp | 2 - > src/cam/file_sink.cpp | 20 ++- > src/cam/file_sink.h | 1 + > src/gstreamer/gstlibcameraallocator.cpp | 4 +- > src/ipa/rkisp1/rkisp1.cpp | 33 ++-- > src/libcamera/framebuffer.cpp | 29 +++- > src/libcamera/ipa_data_serializer.cpp | 5 +- > src/libcamera/mapped_framebuffer.cpp | 71 +++++++-- > src/libcamera/v4l2_videodevice.cpp | 147 ++++++++++++++---- > src/qcam/main_window.cpp | 15 +- > src/qcam/main_window.h | 1 + > test/v4l2_videodevice/buffer_cache.cpp | 6 +- > 17 files changed, 311 insertions(+), 107 deletions(-)
Hi Laurent, On Thu, Aug 26, 2021 at 10:11 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Thu, Aug 26, 2021 at 08:25:30PM +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 aligns some code of mapping and > > creating FrameBuffer::Plane with the new offset. > > > > One of the pateches fixes V4L2Device to make a returned FrameBuffer > > color format planes from v4l2 format planes. However, V4L2 API doesn't > > provide a way of getting a dmabuf plane offset, so a multi planar > > format e.g. V4L2_PIX_FMT_NV12M is not supported with this patch series. > > > > This series must be applied on top of the pacth series whose id is 2384. > > cf.) https://patchwork.libcamera.org/project/libcamera/list/?series=2384 > > Very nice series. I've replied with a few minor comments on patches 1/9 > to 6/9, and 8/9 to 9/9. I can handle those when pushing the series. > > Patch 7/9 is still problematic, as it breaks a unit test. If it's the > only patch that needs a new version, you can resubmit that one only (for > instance as v3.1, in a reply to v3) if it's easier for you. If you > prefer sending a v4 of the whole series, please address the minor > comments on the other patches as well. > Thanks for testing. I uploaded 7/9 patch only in v4. -Hiro > > Change in v3: > > - address review comments to v2 > > - squash 01/10 and 02/10 (serialization change) of v2 > > > > Change in v2: > > - adress review comments to v1 > > > > Hirokazu Honda (9): > > libcamera: framebuffer: Add offset to FrameBuffer::Plane > > 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 > > V4L2VideDevice::createBuffer() creates the same number of > > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > > format single is single-planar format, the created number of > > FrameBuffer::Planes is 1. It should rather create the same number of > > FrameBuffer::Planes as the color format planes. > > 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 | 10 +- > > src/android/camera_device.cpp | 52 ++++--- > > src/android/camera_device.h | 6 +- > > src/android/mm/generic_camera_buffer.cpp | 2 - > > src/cam/file_sink.cpp | 20 ++- > > src/cam/file_sink.h | 1 + > > src/gstreamer/gstlibcameraallocator.cpp | 4 +- > > src/ipa/rkisp1/rkisp1.cpp | 33 ++-- > > src/libcamera/framebuffer.cpp | 29 +++- > > src/libcamera/ipa_data_serializer.cpp | 5 +- > > src/libcamera/mapped_framebuffer.cpp | 71 +++++++-- > > src/libcamera/v4l2_videodevice.cpp | 147 ++++++++++++++---- > > src/qcam/main_window.cpp | 15 +- > > src/qcam/main_window.h | 1 + > > test/v4l2_videodevice/buffer_cache.cpp | 6 +- > > 17 files changed, 311 insertions(+), 107 deletions(-) > > -- > Regards, > > Laurent Pinchart
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 aligns some code of mapping and creating FrameBuffer::Plane with the new offset. One of the pateches fixes V4L2Device to make a returned FrameBuffer color format planes from v4l2 format planes. However, V4L2 API doesn't provide a way of getting a dmabuf plane offset, so a multi planar format e.g. V4L2_PIX_FMT_NV12M is not supported with this patch series. This series must be applied on top of the pacth series whose id is 2384. cf.) https://patchwork.libcamera.org/project/libcamera/list/?series=2384 Change in v3: - address review comments to v2 - squash 01/10 and 02/10 (serialization change) of v2 Change in v2: - adress review comments to v1 Hirokazu Honda (9): libcamera: framebuffer: Add offset to FrameBuffer::Plane 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 V4L2VideDevice::createBuffer() creates the same number of FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 format single is single-planar format, the created number of FrameBuffer::Planes is 1. It should rather create the same number of FrameBuffer::Planes as the color format planes. 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 | 10 +- src/android/camera_device.cpp | 52 ++++--- src/android/camera_device.h | 6 +- src/android/mm/generic_camera_buffer.cpp | 2 - src/cam/file_sink.cpp | 20 ++- src/cam/file_sink.h | 1 + src/gstreamer/gstlibcameraallocator.cpp | 4 +- src/ipa/rkisp1/rkisp1.cpp | 33 ++-- src/libcamera/framebuffer.cpp | 29 +++- src/libcamera/ipa_data_serializer.cpp | 5 +- src/libcamera/mapped_framebuffer.cpp | 71 +++++++-- src/libcamera/v4l2_videodevice.cpp | 147 ++++++++++++++---- src/qcam/main_window.cpp | 15 +- src/qcam/main_window.h | 1 + test/v4l2_videodevice/buffer_cache.cpp | 6 +- 17 files changed, 311 insertions(+), 107 deletions(-) -- 2.33.0.rc2.250.ged5fa647cd-goog