| Message ID | 20221201092733.2042078-1-chenghaoyang@google.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey, Quoting Harvey Yang via libcamera-devel (2022-12-01 09:27:27) > Rebased and fixed some merge conflicts on the master branch. > Please check if it's ready to merge. > Thanks! One of our usual key requirements for merge are at least two reviewers having reviewed and provided reviewed-by tags (after working through any identified issues and discussions). As this component is greatly affecting the Android layer which is used extensively by ChromeOS ... could we have one of the required reviewers and testers being from Google or Chromium teams please? Providing a reference to results from running CTS would also be beneficial here. -- Kieran > BR, > Harvey > > > Harvey Yang (6): > Allow inheritance of FrameBuffer > Add HALFrameBuffer and replace FrameBuffer in src/android > Add meson.build in src/android/jpeg > Move generateThumbnail from PostProcessorJpeg to Encoder > Pass StreamBuffer to Encoder::encoder > Add JEA implementation > > include/libcamera/framebuffer.h | 3 +- > src/android/camera_device.cpp | 3 +- > src/android/cros/camera3_hal.cpp | 2 + > src/android/cros_mojo_token.h | 12 ++ > src/android/frame_buffer_allocator.h | 7 +- > src/android/hal_framebuffer.cpp | 22 +++ > src/android/hal_framebuffer.h | 26 ++++ > src/android/jpeg/encoder.h | 10 +- > src/android/jpeg/encoder_jea.cpp | 93 ++++++++++++ > src/android/jpeg/encoder_jea.h | 35 +++++ > src/android/jpeg/encoder_libjpeg.cpp | 133 ++++++++++++++---- > src/android/jpeg/encoder_libjpeg.h | 33 ++++- > src/android/jpeg/meson.build | 16 +++ > src/android/jpeg/post_processor_jpeg.cpp | 65 ++------- > src/android/jpeg/post_processor_jpeg.h | 11 +- > src/android/meson.build | 6 +- > .../mm/cros_frame_buffer_allocator.cpp | 9 +- > .../mm/generic_frame_buffer_allocator.cpp | 11 +- > 18 files changed, 385 insertions(+), 112 deletions(-) > create mode 100644 src/android/cros_mojo_token.h > create mode 100644 src/android/hal_framebuffer.cpp > create mode 100644 src/android/hal_framebuffer.h > create mode 100644 src/android/jpeg/encoder_jea.cpp > create mode 100644 src/android/jpeg/encoder_jea.h > create mode 100644 src/android/jpeg/meson.build > > -- > 2.38.1.584.g0f3c55d4c2-goog >
Hi Kieran, I see, while I actually found that the patch leads to a flaky crash in cros camera service. I'm still debugging with Han-lin. After I find the root cause, I'll ask Tomasz to take a look. And I'll also provide the CTS result. On Thu, Dec 1, 2022 at 6:25 PM Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Harvey, > > Quoting Harvey Yang via libcamera-devel (2022-12-01 09:27:27) > > Rebased and fixed some merge conflicts on the master branch. > > Please check if it's ready to merge. > > Thanks! > > One of our usual key requirements for merge are at least two reviewers > having reviewed and provided reviewed-by tags (after working through any > identified issues and discussions). > > As this component is greatly affecting the Android layer which is used > extensively by ChromeOS ... could we have one of the required reviewers > and testers being from Google or Chromium teams please? > > Providing a reference to results from running CTS would also be > beneficial here. > > -- > Kieran > > > > BR, > > Harvey > > > > > > Harvey Yang (6): > > Allow inheritance of FrameBuffer > > Add HALFrameBuffer and replace FrameBuffer in src/android > > Add meson.build in src/android/jpeg > > Move generateThumbnail from PostProcessorJpeg to Encoder > > Pass StreamBuffer to Encoder::encoder > > Add JEA implementation > > > > include/libcamera/framebuffer.h | 3 +- > > src/android/camera_device.cpp | 3 +- > > src/android/cros/camera3_hal.cpp | 2 + > > src/android/cros_mojo_token.h | 12 ++ > > src/android/frame_buffer_allocator.h | 7 +- > > src/android/hal_framebuffer.cpp | 22 +++ > > src/android/hal_framebuffer.h | 26 ++++ > > src/android/jpeg/encoder.h | 10 +- > > src/android/jpeg/encoder_jea.cpp | 93 ++++++++++++ > > src/android/jpeg/encoder_jea.h | 35 +++++ > > src/android/jpeg/encoder_libjpeg.cpp | 133 ++++++++++++++---- > > src/android/jpeg/encoder_libjpeg.h | 33 ++++- > > src/android/jpeg/meson.build | 16 +++ > > src/android/jpeg/post_processor_jpeg.cpp | 65 ++------- > > src/android/jpeg/post_processor_jpeg.h | 11 +- > > src/android/meson.build | 6 +- > > .../mm/cros_frame_buffer_allocator.cpp | 9 +- > > .../mm/generic_frame_buffer_allocator.cpp | 11 +- > > 18 files changed, 385 insertions(+), 112 deletions(-) > > create mode 100644 src/android/cros_mojo_token.h > > create mode 100644 src/android/hal_framebuffer.cpp > > create mode 100644 src/android/hal_framebuffer.h > > create mode 100644 src/android/jpeg/encoder_jea.cpp > > create mode 100644 src/android/jpeg/encoder_jea.h > > create mode 100644 src/android/jpeg/meson.build > > > > -- > > 2.38.1.584.g0f3c55d4c2-goog > > >
Hi Harvey, On Wed, Dec 07, 2022 at 01:59:15PM +0800, Cheng-Hao Yang via libcamera-devel wrote: > Hi Kieran, > > I see, while I actually found that the patch leads to a flaky crash in cros > camera service. I'm still debugging with Han-lin. After I find the root cause, > I'll ask Tomasz to take a look. And I'll also provide the CTS result. Thank you. I was wondering if you'd like any help with the review comments I've sent, in particular with the proposed refactoring of the libjpeg-based encoder. > On Thu, Dec 1, 2022 at 6:25 PM Kieran Bingham wrote: > > Quoting Harvey Yang via libcamera-devel (2022-12-01 09:27:27) > > > Rebased and fixed some merge conflicts on the master branch. > > > Please check if it's ready to merge. > > > Thanks! > > > > One of our usual key requirements for merge are at least two reviewers > > having reviewed and provided reviewed-by tags (after working through any > > identified issues and discussions). > > > > As this component is greatly affecting the Android layer which is used > > extensively by ChromeOS ... could we have one of the required reviewers > > and testers being from Google or Chromium teams please? > > > > Providing a reference to results from running CTS would also be > > beneficial here. > > > > > Harvey Yang (6): > > > Allow inheritance of FrameBuffer > > > Add HALFrameBuffer and replace FrameBuffer in src/android > > > Add meson.build in src/android/jpeg > > > Move generateThumbnail from PostProcessorJpeg to Encoder > > > Pass StreamBuffer to Encoder::encoder > > > Add JEA implementation > > > > > > include/libcamera/framebuffer.h | 3 +- > > > src/android/camera_device.cpp | 3 +- > > > src/android/cros/camera3_hal.cpp | 2 + > > > src/android/cros_mojo_token.h | 12 ++ > > > src/android/frame_buffer_allocator.h | 7 +- > > > src/android/hal_framebuffer.cpp | 22 +++ > > > src/android/hal_framebuffer.h | 26 ++++ > > > src/android/jpeg/encoder.h | 10 +- > > > src/android/jpeg/encoder_jea.cpp | 93 ++++++++++++ > > > src/android/jpeg/encoder_jea.h | 35 +++++ > > > src/android/jpeg/encoder_libjpeg.cpp | 133 ++++++++++++++---- > > > src/android/jpeg/encoder_libjpeg.h | 33 ++++- > > > src/android/jpeg/meson.build | 16 +++ > > > src/android/jpeg/post_processor_jpeg.cpp | 65 ++------- > > > src/android/jpeg/post_processor_jpeg.h | 11 +- > > > src/android/meson.build | 6 +- > > > .../mm/cros_frame_buffer_allocator.cpp | 9 +- > > > .../mm/generic_frame_buffer_allocator.cpp | 11 +- > > > 18 files changed, 385 insertions(+), 112 deletions(-) > > > create mode 100644 src/android/cros_mojo_token.h > > > create mode 100644 src/android/hal_framebuffer.cpp > > > create mode 100644 src/android/hal_framebuffer.h > > > create mode 100644 src/android/jpeg/encoder_jea.cpp > > > create mode 100644 src/android/jpeg/encoder_jea.h > > > create mode 100644 src/android/jpeg/meson.build
On Wed, Dec 14, 2022 at 11:12:29AM +0200, Laurent Pinchart wrote: > Hi Harvey, Sorry, I still need to wake up :-) The comment about help with refactoring was for Harvey, but I of course meant Cheng-Hao here. > On Wed, Dec 07, 2022 at 01:59:15PM +0800, Cheng-Hao Yang via libcamera-devel wrote: > > Hi Kieran, > > > > I see, while I actually found that the patch leads to a flaky crash in cros > > camera service. I'm still debugging with Han-lin. After I find the root cause, > > I'll ask Tomasz to take a look. And I'll also provide the CTS result. > > Thank you. > > I was wondering if you'd like any help with the review comments I've > sent, in particular with the proposed refactoring of the libjpeg-based > encoder. > > > On Thu, Dec 1, 2022 at 6:25 PM Kieran Bingham wrote: > > > Quoting Harvey Yang via libcamera-devel (2022-12-01 09:27:27) > > > > Rebased and fixed some merge conflicts on the master branch. > > > > Please check if it's ready to merge. > > > > Thanks! > > > > > > One of our usual key requirements for merge are at least two reviewers > > > having reviewed and provided reviewed-by tags (after working through any > > > identified issues and discussions). > > > > > > As this component is greatly affecting the Android layer which is used > > > extensively by ChromeOS ... could we have one of the required reviewers > > > and testers being from Google or Chromium teams please? > > > > > > Providing a reference to results from running CTS would also be > > > beneficial here. > > > > > > > Harvey Yang (6): > > > > Allow inheritance of FrameBuffer > > > > Add HALFrameBuffer and replace FrameBuffer in src/android > > > > Add meson.build in src/android/jpeg > > > > Move generateThumbnail from PostProcessorJpeg to Encoder > > > > Pass StreamBuffer to Encoder::encoder > > > > Add JEA implementation > > > > > > > > include/libcamera/framebuffer.h | 3 +- > > > > src/android/camera_device.cpp | 3 +- > > > > src/android/cros/camera3_hal.cpp | 2 + > > > > src/android/cros_mojo_token.h | 12 ++ > > > > src/android/frame_buffer_allocator.h | 7 +- > > > > src/android/hal_framebuffer.cpp | 22 +++ > > > > src/android/hal_framebuffer.h | 26 ++++ > > > > src/android/jpeg/encoder.h | 10 +- > > > > src/android/jpeg/encoder_jea.cpp | 93 ++++++++++++ > > > > src/android/jpeg/encoder_jea.h | 35 +++++ > > > > src/android/jpeg/encoder_libjpeg.cpp | 133 ++++++++++++++---- > > > > src/android/jpeg/encoder_libjpeg.h | 33 ++++- > > > > src/android/jpeg/meson.build | 16 +++ > > > > src/android/jpeg/post_processor_jpeg.cpp | 65 ++------- > > > > src/android/jpeg/post_processor_jpeg.h | 11 +- > > > > src/android/meson.build | 6 +- > > > > .../mm/cros_frame_buffer_allocator.cpp | 9 +- > > > > .../mm/generic_frame_buffer_allocator.cpp | 11 +- > > > > 18 files changed, 385 insertions(+), 112 deletions(-) > > > > create mode 100644 src/android/cros_mojo_token.h > > > > create mode 100644 src/android/hal_framebuffer.cpp > > > > create mode 100644 src/android/hal_framebuffer.h > > > > create mode 100644 src/android/jpeg/encoder_jea.cpp > > > > create mode 100644 src/android/jpeg/encoder_jea.h > > > > create mode 100644 src/android/jpeg/meson.build
Hi Laurent, On Wed, Dec 14, 2022 at 5:13 PM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > On Wed, Dec 14, 2022 at 11:12:29AM +0200, Laurent Pinchart wrote: > > Hi Harvey, > > Sorry, I still need to wake up :-) The comment about help with > refactoring was for Harvey, but I of course meant Cheng-Hao here. > > > On Wed, Dec 07, 2022 at 01:59:15PM +0800, Cheng-Hao Yang via > libcamera-devel wrote: > > > Hi Kieran, > > > > > > I see, while I actually found that the patch leads to a flaky crash in > cros > > > camera service. I'm still debugging with Han-lin. After I find the > root cause, > > > I'll ask Tomasz to take a look. And I'll also provide the CTS result. > > > > Thank you. > > > > I was wondering if you'd like any help with the review comments I've > > sent, in particular with the proposed refactoring of the libjpeg-based > > encoder. > > Definitely. Sorry for the belated reply. I wanted to solve the flaky issue altogether before sending the new version of patches. Sorry for the delay! > > > > On Thu, Dec 1, 2022 at 6:25 PM Kieran Bingham wrote: > > > > Quoting Harvey Yang via libcamera-devel (2022-12-01 09:27:27) > > > > > Rebased and fixed some merge conflicts on the master branch. > > > > > Please check if it's ready to merge. > > > > > Thanks! > > > > > > > > One of our usual key requirements for merge are at least two > reviewers > > > > having reviewed and provided reviewed-by tags (after working through > any > > > > identified issues and discussions). > > > > > > > > As this component is greatly affecting the Android layer which is > used > > > > extensively by ChromeOS ... could we have one of the required > reviewers > > > > and testers being from Google or Chromium teams please? > > > > > > > > Providing a reference to results from running CTS would also be > > > > beneficial here. > > > > > > > > > Harvey Yang (6): > > > > > Allow inheritance of Fra Harvey Yang <chenghaoyang@chromium.org> Thu, Dec 1, 5:27 PM (13 days ago)  to libcamera-devel, Harvey  Rebased and fixed some merge conflicts on the master branch. Please check if it's ready to merge. Thanks! > meBuffer > > > > > Add HALFrameBuffer and replace FrameBuffer in src/android > > > > > Add meson.build in src/android/jpeg > > > > > Move generateThumbnail from PostProcessorJpeg to Encoder > > > > > Pass StreamBuffer to Encoder::encoder > > > > > Add JEA implementation > > > > > > > > > > include/libcamera/framebuffer.h | 3 +- > > > > > src/android/camera_device.cpp | 3 +- > > > > > src/android/cros/camera3_hal.cpp | 2 + > > > > > src/android/cros_mojo_token.h | 12 ++ > > > > > src/android/frame_buffer_allocator.h | 7 +- > > > > > src/android/hal_framebuffer.cpp | 22 +++ > > > > > src/android/hal_framebuffer.h | 26 ++++ > > > > > src/android/jpeg/encoder.h | 10 +- > > > > > src/android/jpeg/encoder_jea.cpp | 93 ++++++++++++ > > > > > src/android/jpeg/encoder_jea.h | 35 +++++ > > > > > src/android/jpeg/encoder_libjpeg.cpp | 133 > ++++++++++++++---- > > > > > src/android/jpeg/encoder_libjpeg.h | 33 ++++- > > > > > src/android/jpeg/meson.build | 16 +++ > > > > > src/android/jpeg/post_processor_jpeg.cpp | 65 ++------- > > > > > src/android/jpeg/post_processor_jpeg.h | 11 +- > > > > > src/android/meson.build | 6 +- > > > > > .../mm/cros_frame_buffer_allocator.cpp | 9 +- > > > > > .../mm/generic_frame_buffer_allocator.cpp | 11 +- > > > > > 18 files changed, 385 insertions(+), 112 deletions(-) > > > > > create mode 100644 src/android/cros_mojo_token.h > > > > > create mode 100644 src/android/hal_framebuffer.cpp > > > > > create mode 100644 src/android/hal_framebuffer.h > > > > > create mode 100644 src/android/jpeg/encoder_jea.cpp > > > > > create mode 100644 src/android/jpeg/encoder_jea.h > > > > > create mode 100644 src/android/jpeg/meson.build > > -- > Regards, > > Laurent Pinchart >
Hi Kieran, On Wed, Dec 14, 2022 at 5:34 PM Cheng-Hao Yang <chenghaoyang@chromium.org> wrote: > Hi Laurent, > > > > On Wed, Dec 14, 2022 at 5:13 PM Laurent Pinchart < > laurent.pinchart@ideasonboard.com> wrote: > >> On Wed, Dec 14, 2022 at 11:12:29AM +0200, Laurent Pinchart wrote: >> > Hi Harvey, >> >> Sorry, I still need to wake up :-) The comment about help with >> refactoring was for Harvey, but I of course meant Cheng-Hao here. >> >> > On Wed, Dec 07, 2022 at 01:59:15PM +0800, Cheng-Hao Yang via >> libcamera-devel wrote: >> > > Hi Kieran, >> > > >> > > I see, while I actually found that the patch leads to a flaky crash >> in cros >> > > camera service. I'm still debugging with Han-lin. After I find the >> root cause, >> > > I'll ask Tomasz to take a look. And I'll also provide the CTS result. >> > >> > Thank you. >> > >> > I was wondering if you'd like any help with the review comments I've >> > sent, in particular with the proposed refactoring of the libjpeg-based >> > encoder. >> > > > > Definitely. Sorry for the belated reply. I wanted to solve the flaky issue > altogether > before sending the new version of patches. > Sorry for the delay! > > >> >> > > On Thu, Dec 1, 2022 at 6:25 PM Kieran Bingham wrote: >> > > > Quoting Harvey Yang via libcamera-devel (2022-12-01 09:27:27) >> > > > > Rebased and fixed some merge conflicts on the master branch. >> > > > > Please check if it's ready to merge. >> > > > > Thanks! >> > > > >> > > > One of our usual key requirements for merge are at least two >> reviewers >> > > > having reviewed and provided reviewed-by tags (after working >> through any >> > > > identified issues and discussions). >> > > > >> > > > As this component is greatly affecting the Android layer which is >> used >> > > > extensively by ChromeOS ... could we have one of the required >> reviewers >> > > > and testers being from Google or Chromium teams please? >> > > > >> > As Laurent has reviewed the patches, and Han-lin from CrOS also took a look, I guess I don't need to ask Tomasz to review anymore, right? > > > > Providing a reference to results from running CTS would also be >> > > > beneficial here. >> > > > >> > > > > Harvey Yang (6): >> > > > > Allow inheritance of Fra > > Harvey Yang <chenghaoyang@chromium.org> > Thu, Dec 1, 5:27 PM (13 days ago) >  > to libcamera-devel, Harvey >  > Rebased and fixed some merge conflicts on the master branch. > Please check if it's ready to merge. > Thanks! > >> meBuffer >> > > > > Add HALFrameBuffer and replace FrameBuffer in src/android >> > > > > Add meson.build in src/android/jpeg >> > > > > Move generateThumbnail from PostProcessorJpeg to Encoder >> > > > > Pass StreamBuffer to Encoder::encoder >> > > > > Add JEA implementation >> > > > > >> > > > > include/libcamera/framebuffer.h | 3 +- >> > > > > src/android/camera_device.cpp | 3 +- >> > > > > src/android/cros/camera3_hal.cpp | 2 + >> > > > > src/android/cros_mojo_token.h | 12 ++ >> > > > > src/android/frame_buffer_allocator.h | 7 +- >> > > > > src/android/hal_framebuffer.cpp | 22 +++ >> > > > > src/android/hal_framebuffer.h | 26 ++++ >> > > > > src/android/jpeg/encoder.h | 10 +- >> > > > > src/android/jpeg/encoder_jea.cpp | 93 ++++++++++++ >> > > > > src/android/jpeg/encoder_jea.h | 35 +++++ >> > > > > src/android/jpeg/encoder_libjpeg.cpp | 133 >> ++++++++++++++---- >> > > > > src/android/jpeg/encoder_libjpeg.h | 33 ++++- >> > > > > src/android/jpeg/meson.build | 16 +++ >> > > > > src/android/jpeg/post_processor_jpeg.cpp | 65 ++------- >> > > > > src/android/jpeg/post_processor_jpeg.h | 11 +- >> > > > > src/android/meson.build | 6 +- >> > > > > .../mm/cros_frame_buffer_allocator.cpp | 9 +- >> > > > > .../mm/generic_frame_buffer_allocator.cpp | 11 +- >> > > > > 18 files changed, 385 insertions(+), 112 deletions(-) >> > > > > create mode 100644 src/android/cros_mojo_token.h >> > > > > create mode 100644 src/android/hal_framebuffer.cpp >> > > > > create mode 100644 src/android/hal_framebuffer.h >> > > > > create mode 100644 src/android/jpeg/encoder_jea.cpp >> > > > > create mode 100644 src/android/jpeg/encoder_jea.h >> > > > > create mode 100644 src/android/jpeg/meson.build >> >> -- >> Regards, >> >> Laurent Pinchart >> >