| Message ID | 20200904084316.7319-1-show.liu@linaro.org |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
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. 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, 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. > 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
Hi Laurent, On Mon, Sep 7, 2020 at 12:25 AM Laurent Pinchart < laurent.pinchart@ideasonboard.com> 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 Best Regards, Show Liu > > > 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 >
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
Hi Laurent, On Tue, Sep 8, 2020 at 7:23 PM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > 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) ? Sure. No problem. I will change to "LGPL-2.1-or-later" . I foresee a possibly need to use that code inside > libcamera for a GPU-based ISP in the future That sounds interesting. And I believe the offscreen rendering would be more suitable for that.... just an idea. Regards, Show , 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 >
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. BR, Show 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