GNOME Bugzilla – Bug 787443
Add GL_ARB_shader_texture_lod support
Last modified: 2021-06-10 11:19:45 UTC
I am packaging Deepin Desktop Environment for Fedora. Some component needs to use this feature. Currently, I using a new package (deepin-cogl) for the patch. But it doesn't fit the Fedora packaging guides. Could you add these patches to cogl? Patches: https://github.com/linuxdeepin/deepin-cogl/commit/78636289b073d67209a20145ef0dc003f2d77db6 Add GL_ARB_shader_texture_lod support https://github.com/linuxdeepin/deepin-cogl/commit/d8b34ab0604d80d0be22b8b78e9aa6bf4fac7db0 texture: Support copy_sub_image Fedora Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1421055
Thanks! Is Deepin Desktop using Mutter? Because then you want to have those patches applied to the internal copy of Cogl inside Mutter. Additionally, it would help if you could attach those patches to Bugzilla instead of linking to GitHub commits; another helpful thing would be to have an actual commit instead of "support copy_sub_image" and "add GL_ARB_shader_texture_lod support". Why are these changes needed?
(In reply to Emmanuele Bassi (:ebassi) from comment #1) > Thanks! > > Is Deepin Desktop using Mutter? Because then you want to have those patches > applied to the internal copy of Cogl inside Mutter. Yes, deepin desktop use Mutter. In fact, the deepin create a mutter 'fork' to custom features. I think it can not merge to upstream anytime. https://github.com/linuxdeepin/deepin-mutter > Additionally, it would help if you could attach those patches to Bugzilla > instead of linking to GitHub commits; another helpful thing would be to have > an actual commit instead of "support copy_sub_image" and "add > GL_ARB_shader_texture_lod support". Why are these changes needed? add the developer to cc list.
(In reply to Emmanuele Bassi (:ebassi) from comment #1) > Thanks! > > Is Deepin Desktop using Mutter? Because then you want to have those patches > applied to the internal copy of Cogl inside Mutter. > deepin-mutter is forked off from 3.20 branch, which has no internal copy of cogl yet. and since a lot of customizations had been built based on 3.20, it's not easy to port to lastest branch. > Additionally, it would help if you could attach those patches to Bugzilla > instead of linking to GitHub commits; another helpful thing would be to have > an actual commit instead of "support copy_sub_image" and "add > GL_ARB_shader_texture_lod support". Why are these changes needed? "support copy_sub_image" is used to implement feature similar with kwin blur effect by being abel to copy partial of framebuffer contents as texture and do post blurring. "add GL_ARB_shader_texture_lod support" is used to do lod biased texturing. I can achieve faster blurring of images instead of using large blur radius.
Created attachment 359589 [details] [review] Add GL_ARB_shader_texture_lod support
Created attachment 359590 [details] [review] texture support copy_sub_image
(In reply to Emmanuele Bassi (:ebassi) from comment #1) > Thanks! > > Is Deepin Desktop using Mutter? Because then you want to have those patches > applied to the internal copy of Cogl inside Mutter. > > Additionally, it would help if you could attach those patches to Bugzilla > instead of linking to GitHub commits; another helpful thing would be to have > an actual commit instead of "support copy_sub_image" and "add > GL_ARB_shader_texture_lod support". Why are these changes needed? I have attached those patches to bugzilla. What do you think about the patches? @ebassi We need another patch with 'clutter' to make Deepin Desktop works well.
Hi GNOME maintainers, any updates/concern to these patch?
Cogl is largely unmaintained, these days; that's another reason why it was merged into the Mutter tree. The patch is trivial enough, so it could be merged, but it's unlikely I'll ever do a new release (I couldn't even make distcheck pass, the last time I tried).
Review of attachment 359589 [details] [review]: The patch looks good to me. The commit message is a bit terse; how much faster is "faster"?
Review of attachment 359590 [details] [review]: Looks good; minor coding style fixes. ::: cogl/cogl-texture-2d.c @@ +630,3 @@ static CoglBool +_cogl_texture_2d_copy_sub_image (CoglTexture *tex, + GLint xoffset, GLint yoffset, Coding style: please, align arguments correctly; only one argument per line. ::: cogl/cogl-texture.c @@ +526,3 @@ + + if (!texture->vtable->copy_sub_image) + CoglBool status; This should be `FALSE`. ::: cogl/cogl-texture.h @@ +402,3 @@ +CoglBool +cogl_texture_copy_sub_image (CoglTexture *texture, + int xoffset, int yoffset, Coding style: please, align arguments correctly; only one argument per line. ::: cogl/driver/gl/cogl-texture-2d-gl-private.h @@ +112,3 @@ +CoglBool +_cogl_texture_2d_gl_copy_sub_image (CoglTexture2D *tex_2d, + GLint xoffset, GLint yoffset, Coding style: please, align arguments correctly; only one argument per line. ::: cogl/driver/gl/cogl-texture-2d-gl.c @@ +723,3 @@ +CoglBool +_cogl_texture_2d_gl_copy_sub_image (CoglTexture2D *tex_2d, + GLint xoffset, GLint yoffset, Coding style: please, align arguments correctly; only one argument per line. @@ +730,3 @@ + + if (ctx->current_read_buffer == NULL) + CoglError **error) The should be `FALSE`. @@ +733,3 @@ + + if (ctx->current_draw_buffer) +{ Coding style: too much indentation. @@ +736,3 @@ + + if (ctx->current_read_buffer != NULL && + Coding style: too much indentation. @@ +737,3 @@ + if (ctx->current_read_buffer != NULL && + ctx->current_read_buffer != ctx->current_draw_buffer) + Coding style: too much indentation.
Created attachment 360285 [details] [review] texture support copy_sub_image
(In reply to Emmanuele Bassi (:ebassi) from comment #10) Thank you for your reply. @ebassi I fixed the coding style. > The patch is trivial enough, so it could be merged, but it's unlikely I'll ever do a new release (I couldn't even make distcheck pass, the last time I tried). Packager should be able to add the patches to package.
Review of attachment 360285 [details] [review]: Thanks! There are typos in the commit message; could you reword it a bit? Something like: ``` We need copy_sub_image to implement blurring effects of partial framebuffer contents as texture sources. ``` We don't use Signed-off-by's, but if we were, you'd need one Signed-off-by line for each email address. In any case, you can just drop them. The patch itself looks good.
Created attachment 361056 [details] [review] texture support copy_sub_image
Thanks, I fixed.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version of cogl, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a ticket at https://gitlab.gnome.org/GNOME/cogl/-/issues/ Thank you for your understanding and your help.