GNOME Bugzilla – Bug 632885
Gaudi Effects dynamically controllable parameters [review]
Last modified: 2010-10-28 16:51:50 UTC
Hi everyone. I've done some work to improve the Gaudi Effects, the video filter set in gst-plugins-bad. The parameters of the effects are dynamically controllable with gstcontroller now, so the properties of the elements can be changed at pipeline construction and during playback. Also, after some review of the current code I have changed the base class of the element to be GstBaseTransform instead of GstElement. The code change can be found at: http://github.com/luisbg/gst-plugins-bad/commit/62930fccaebf4a6f733004ceb224f501bb123b2e Please review/comment and merge! Thank you.
Created attachment 173006 [details] [review] git format-patch style patch
Created attachment 173011 [details] [review] [PATCH] gaudi effects: basing on GstBaseTransform instead of GstElement. gst/gaudieffects/gstburn.c | 31 ++++++++++++++++---- gst/gaudieffects/gstburn.h | 1 + gst/gaudieffects/gstchromium.c | 46 ++++++++++++++++++++++++------ gst/gaudieffects/gstchromium.h | 2 + gst/gaudieffects/gstexclusion.c | 29 ++++++++++++++---- gst/gaudieffects/gstexclusion.h | 1 + gst/gaudieffects/gstplugin.h | 45 ++++++++++++++++++++++------- gst/gaudieffects/gstsolarize.c | 59 +++++++++++++++++++++++++++++++++------ gst/gaudieffects/gstsolarize.h | 1 + 9 files changed, 172 insertions(+), 43 deletions(-)
Created attachment 173012 [details] [review] [PATCH] gaudi effects: made filter parameters dynamic and controllable through gstController. --- gst/gaudieffects/gstburn.c | 28 ++++++++++++++++++++++-- gst/gaudieffects/gstchromium.c | 43 +++++++++++++++++++++++++++++--------- gst/gaudieffects/gstchromium.h | 25 ++++++---------------- gst/gaudieffects/gstexclusion.c | 32 +++++++++++++++++++++++----- gst/gaudieffects/gstsolarize.c | 37 ++++++++++++++++++++++++++++----- 5 files changed, 122 insertions(+), 43 deletions(-)
Review of attachment 173006 [details] [review]: IGNORE this patch
Please review/comment/merge patches in Comment 2 and Comment 3. They are the two commits against gst-plugin-bad's git: - basing on GstBaseTransform instead of GstElement. - made filter parameters dynamic and controllable
Review of attachment 173011 [details] [review]: This patch does not do what it claims to do, it adds some properties to random effects. (But you should base the effect elements on GstVideoFilter instead of GstBaseTransform) Please change the commit message and fix the gstplugin.h changes ::: gst/gaudieffects/gstplugin.h @@ -3,3 @@ * Copyright (C) 2005 Thomas Vander Stichele <thomas@apestaart.org> * Copyright (C) 2005 Ronald S. Bultje <rbultje@ronald.bitfreak.net> - * Copyright (C) 2010 Luis de Bethencourt <luis@debethencourt.com> This change looks inverted @@ -48,1 @@ And this one too and everything else in this file
Review of attachment 173012 [details] [review]: Looks good in general... any reason why you only make some properties controllable... and only some elements? you should also call gst_controller_init() in gstplugin.c ::: gst/gaudieffects/gstchromium.h @@ -72,3 @@ GstVideoFilter videofilter; - - /* < private > */ You should keep this comment, otherwise gtk-doc will show it
(In reply to comment #6) > Review of attachment 173011 [details] [review]: > > This patch does not do what it claims to do, it adds some properties to random > effects. > > (But you should base the effect elements on GstVideoFilter instead of > GstBaseTransform) > > Please change the commit message and fix the gstplugin.h changes > > ::: gst/gaudieffects/gstplugin.h > @@ -3,3 @@ > * Copyright (C) 2005 Thomas Vander Stichele <thomas@apestaart.org> > * Copyright (C) 2005 Ronald S. Bultje <rbultje@ronald.bitfreak.net> > - * Copyright (C) 2010 Luis de Bethencourt <luis@debethencourt.com> > > This change looks inverted > > @@ -48,1 @@ > > > And this one too and everything else in this file I see what you mean. Sorry about the commit message, I confused the difference with my previous version with the one Jan submitted to gst-plugins-bad. I will fix it, and also clean the changes on gstplugin.h. I will look into basing the effects on GstVideoFilter instead of GstBaseTransform.
(In reply to comment #7) > Review of attachment 173012 [details] [review]: > > Looks good in general... any reason why you only make some properties > controllable... and only some elements? I gave higher priority to the properties that have a bigger impact on the outcome. > > you should also call gst_controller_init() in gstplugin.c > Will do. > ::: gst/gaudieffects/gstchromium.h > @@ -72,3 @@ > GstVideoFilter videofilter; > - > - /* < private > */ > > You should keep this comment, otherwise gtk-doc will show it Ooooh, didn't thought about that. Cool. Thanks a lot for the review! :)
Created attachment 173323 [details] [review] [PATCH] gaudi effects: made filter parameters dynamic and controllable gst/gaudieffects/gstburn.c | 61 +++++++++-- gst/gaudieffects/gstburn.h | 1 + gst/gaudieffects/gstchromium.c | 87 ++++++++++++--- gst/gaudieffects/gstchromium.h | 26 ++--- gst/gaudieffects/gstdilate.c | 227 ++++++++++++++++++++++++++++----------- gst/gaudieffects/gstdilate.h | 1 + gst/gaudieffects/gstdodge.c | 13 ++- gst/gaudieffects/gstexclusion.c | 61 ++++++++--- gst/gaudieffects/gstexclusion.h | 1 + gst/gaudieffects/gstplugin.c | 1 + gst/gaudieffects/gstsolarize.c | 94 ++++++++++++++--- gst/gaudieffects/gstsolarize.h | 1 + 12 files changed, 434 insertions(+), 140 deletions(-)
Created attachment 173324 [details] [review] [PATCH] gaudi effects: made filter parameters dynamic and controllable gst/gaudieffects/gstburn.c | 61 +++++++++-- gst/gaudieffects/gstburn.h | 1 + gst/gaudieffects/gstchromium.c | 87 ++++++++++++--- gst/gaudieffects/gstchromium.h | 20 ++-- gst/gaudieffects/gstdilate.c | 227 ++++++++++++++++++++++++++++----------- gst/gaudieffects/gstdilate.h | 1 + gst/gaudieffects/gstdodge.c | 13 ++- gst/gaudieffects/gstexclusion.c | 61 ++++++++--- gst/gaudieffects/gstexclusion.h | 1 + gst/gaudieffects/gstplugin.c | 1 + gst/gaudieffects/gstsolarize.c | 94 ++++++++++++++--- gst/gaudieffects/gstsolarize.h | 1 + 12 files changed, 434 insertions(+), 134 deletions(-)
Looks good, now make this in git format-patch format and I'll push it :)
Created attachment 173403 [details] [review] git format-patch style patch Thanks a million for the review Sebastian! :)
commit e46cffdb9373a11a051d16efcf941a22dc3bb37f Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Thu Oct 28 18:51:02 2010 +0200 gaudieffects: Include gstcontroller header and add the required CFLAGS commit 4e696fe54492b9e41a645d9a37d9a8c2682b30c1 Author: Luis de Bethencourt <luis@debethencourt.com> Date: Wed Oct 27 14:57:36 2010 +0200 gaudieffects: made filter parameters dynamic and controllable