[libcamera-devel] [PATCH v5 0/4] qcam: accelerate format conversion by OpenGL shader

[Laurent Pinchart]

On Tue, Sep 08, 2020 at 05:35:27PM +0800, Show Liu wrote:
> On Mon, Sep 7, 2020 at 12:25 AM Laurent Pinchart wrote:
> > Hi Show,
> >
> > Thank you for the patches.
> >
> > On Fri, Sep 04, 2020 at 04:43:11PM +0800, Show Liu wrote:
> > > This is patch set v5 for qcam accelerated format conversion by OpenGL shader.
> > > It's based on viewfinderGL(patch set v3), please skip the v4.
> > >
> > > In this version, I changed the original viewfinder hierarchy including created
> > > viewfinder base and move the original viewfinder as default Qt rendering and
> > > created new viewfinderGL to handle OpenGL stuff and also move the OpenGL shader
> > > code as Qt resource. All the changes are according to the previous review comments.
> > >
> > > Known issue:
> > > * It's running well at start time, but when the stop button is pressed and
> > >   starts again, there is no more capture event being triggered.
> > >
> > > Todo:
> > > * Show the No camera icon when the capture stops being pressed.
> >
> > Nice work ! My head hurts with opengl now :-)
> >
> > Patches 1/4, 2/4 and 4/4 are mostly fine. I've sent more comments for
> > patch 3/4, but nothing that should affect the architecture.
> 
> Thanks for the review, I am trying to modify it according to your comments.
> 
> > If there are
> > issue you're not sure how to address, please let me know. I can try to
> > help with the integration, letting you focus on the GL part. For
> > instance I can have a look at the issues mentioned above,
> 
> If so, that would be grateful!!  ...:-)
> 
> > as well as the
> > QT_NO_OPENGL conditional compilation. We also need to fall back to
> > ViewFinderQt when the format isn't supported by ViewFinderGL, and that's
> > also something I can handle.
>
> Actually I have thinks about this problem when I generating this series,
> but the ViewFinderGL been generated before we got the format, currently
> I am just have warning message and suggest to use render qt if setFormat()
> is failed

I'll handle this on top of the next version you'll submit.

One point I failed to mention during the review, would it be possible to
license the shaders and ViewFinderGL under (GPL-2.0-or-later OR
LGPL-2.1-or-later) ? I foresee a possibly need to use that code inside
libcamera for a GPU-based ISP in the future, and also possibly in a
generic format conversion library that could be part of the libcamera
project but implemented in a separate .so. I have no plan to work on
these in the very near future, but having that option would avoid going
through a relicensing process in the future.

> > > Show Liu (4):
> > >   qcam: add OpenGL shader code as Qt resource
> > >   qcam: new viewfinder hierarchy
> > >   qcam: add viewfinderGL class to accelerate the format convert
> > >   qcam: add additional command line option to select the render type
> > >
> > >  src/qcam/assets/shader/NV_2_planes_UV_f.glsl  |  32 ++
> > >  src/qcam/assets/shader/NV_2_planes_VU_f.glsl  |  32 ++
> > >  src/qcam/assets/shader/NV_3_planes_UV_f.glsl  |  33 ++
> > >  src/qcam/assets/shader/NV_3_planes_VU_f.glsl  |  33 ++
> > >  src/qcam/assets/shader/NV_vertex_shader.glsl  |  16 +
> > >  src/qcam/assets/shader/shaders.qrc            |  10 +
> > >  src/qcam/main.cpp                             |   3 +
> > >  src/qcam/main_window.cpp                      |  29 +-
> > >  src/qcam/main_window.h                        |   6 +
> > >  src/qcam/meson.build                          |   7 +-
> > >  src/qcam/viewfinder.h                         |  60 +--
> > >  src/qcam/viewfinder_gl.cpp                    | 441 ++++++++++++++++++
> > >  src/qcam/viewfinder_gl.h                      |  97 ++++
> > >  .../{viewfinder.cpp => viewfinder_qt.cpp}     |  24 +-
> > >  src/qcam/viewfinder_qt.h                      |  67 +++
> > >  15 files changed, 824 insertions(+), 66 deletions(-)
> > >  create mode 100644 src/qcam/assets/shader/NV_2_planes_UV_f.glsl
> > >  create mode 100644 src/qcam/assets/shader/NV_2_planes_VU_f.glsl
> > >  create mode 100644 src/qcam/assets/shader/NV_3_planes_UV_f.glsl
> > >  create mode 100644 src/qcam/assets/shader/NV_3_planes_VU_f.glsl
> > >  create mode 100644 src/qcam/assets/shader/NV_vertex_shader.glsl
> > >  create mode 100644 src/qcam/assets/shader/shaders.qrc
> > >  create mode 100644 src/qcam/viewfinder_gl.cpp
> > >  create mode 100644 src/qcam/viewfinder_gl.h
> > >  rename src/qcam/{viewfinder.cpp => viewfinder_qt.cpp} (86%)
> > >  create mode 100644 src/qcam/viewfinder_qt.h

-- 
Regards,

Laurent Pinchart