<div dir="ltr"><div dir="ltr">Hi Laurent,<br clear="all"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"></div></div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 8, 2020 at 7:23 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, Sep 08, 2020 at 05:35:27PM +0800, Show Liu wrote:<br>
> On Mon, Sep 7, 2020 at 12:25 AM Laurent Pinchart wrote:<br>
> > Hi Show,<br>
> ><br>
> > Thank you for the patches.<br>
> ><br>
> > On Fri, Sep 04, 2020 at 04:43:11PM +0800, Show Liu wrote:<br>
> > > This is patch set v5 for qcam accelerated format conversion by OpenGL shader.<br>
> > > It's based on viewfinderGL(patch set v3), please skip the v4.<br>
> > ><br>
> > > In this version, I changed the original viewfinder hierarchy including created<br>
> > > viewfinder base and move the original viewfinder as default Qt rendering and<br>
> > > created new viewfinderGL to handle OpenGL stuff and also move the OpenGL shader<br>
> > > code as Qt resource. All the changes are according to the previous review comments.<br>
> > ><br>
> > > Known issue:<br>
> > > * It's running well at start time, but when the stop button is pressed and<br>
> > >   starts again, there is no more capture event being triggered.<br>
> > ><br>
> > > Todo:<br>
> > > * Show the No camera icon when the capture stops being pressed.<br>
> ><br>
> > Nice work ! My head hurts with opengl now :-)<br>
> ><br>
> > Patches 1/4, 2/4 and 4/4 are mostly fine. I've sent more comments for<br>
> > patch 3/4, but nothing that should affect the architecture.<br>
> <br>
> Thanks for the review, I am trying to modify it according to your comments.<br>
> <br>
> > If there are<br>
> > issue you're not sure how to address, please let me know. I can try to<br>
> > help with the integration, letting you focus on the GL part. For<br>
> > instance I can have a look at the issues mentioned above,<br>
> <br>
> If so, that would be grateful!!  ...:-)<br>
> <br>
> > as well as the<br>
> > QT_NO_OPENGL conditional compilation. We also need to fall back to<br>
> > ViewFinderQt when the format isn't supported by ViewFinderGL, and that's<br>
> > also something I can handle.<br>
><br>
> Actually I have thinks about this problem when I generating this series,<br>
> but the ViewFinderGL been generated before we got the format, currently<br>
> I am just have warning message and suggest to use render qt if setFormat()<br>
> is failed<br>
<br>
I'll handle this on top of the next version you'll submit.<br>
<br>
One point I failed to mention during the review, would it be possible to<br>
license the shaders and ViewFinderGL under (GPL-2.0-or-later OR<br>
LGPL-2.1-or-later) ?</blockquote><div>Sure. No problem. I will change to "LGPL-2.1-or-later" .</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> I foresee a possibly need to use that code inside<br>
libcamera for a GPU-based ISP in the future</blockquote><div>That sounds interesting.</div><div>And I believe the offscreen rendering would be more suitable for that.... just an idea.</div><div><br></div><div>Regards,</div><div>Show</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">, and also possibly in a<br>
generic format conversion library that could be part of the libcamera<br>
project but implemented in a separate .so. I have no plan to work on<br>
these in the very near future, but having that option would avoid going<br>
through a relicensing process in the future.<br>
<br>
> > > Show Liu (4):<br>
> > >   qcam: add OpenGL shader code as Qt resource<br>
> > >   qcam: new viewfinder hierarchy<br>
> > >   qcam: add viewfinderGL class to accelerate the format convert<br>
> > >   qcam: add additional command line option to select the render type<br>
> > ><br>
> > >  src/qcam/assets/shader/NV_2_planes_UV_f.glsl  |  32 ++<br>
> > >  src/qcam/assets/shader/NV_2_planes_VU_f.glsl  |  32 ++<br>
> > >  src/qcam/assets/shader/NV_3_planes_UV_f.glsl  |  33 ++<br>
> > >  src/qcam/assets/shader/NV_3_planes_VU_f.glsl  |  33 ++<br>
> > >  src/qcam/assets/shader/NV_vertex_shader.glsl  |  16 +<br>
> > >  src/qcam/assets/shader/shaders.qrc            |  10 +<br>
> > >  src/qcam/main.cpp                             |   3 +<br>
> > >  src/qcam/main_window.cpp                      |  29 +-<br>
> > >  src/qcam/main_window.h                        |   6 +<br>
> > >  src/qcam/meson.build                          |   7 +-<br>
> > >  src/qcam/viewfinder.h                         |  60 +--<br>
> > >  src/qcam/viewfinder_gl.cpp                    | 441 ++++++++++++++++++<br>
> > >  src/qcam/viewfinder_gl.h                      |  97 ++++<br>
> > >  .../{viewfinder.cpp => viewfinder_qt.cpp}     |  24 +-<br>
> > >  src/qcam/viewfinder_qt.h                      |  67 +++<br>
> > >  15 files changed, 824 insertions(+), 66 deletions(-)<br>
> > >  create mode 100644 src/qcam/assets/shader/NV_2_planes_UV_f.glsl<br>
> > >  create mode 100644 src/qcam/assets/shader/NV_2_planes_VU_f.glsl<br>
> > >  create mode 100644 src/qcam/assets/shader/NV_3_planes_UV_f.glsl<br>
> > >  create mode 100644 src/qcam/assets/shader/NV_3_planes_VU_f.glsl<br>
> > >  create mode 100644 src/qcam/assets/shader/NV_vertex_shader.glsl<br>
> > >  create mode 100644 src/qcam/assets/shader/shaders.qrc<br>
> > >  create mode 100644 src/qcam/viewfinder_gl.cpp<br>
> > >  create mode 100644 src/qcam/viewfinder_gl.h<br>
> > >  rename src/qcam/{viewfinder.cpp => viewfinder_qt.cpp} (86%)<br>
> > >  create mode 100644 src/qcam/viewfinder_qt.h<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>