After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 632885 - Gaudi Effects dynamically controllable parameters [review]
Gaudi Effects dynamically controllable parameters [review]
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal enhancement
: 0.10.21
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-10-22 11:42 UTC by Luis de Bethencourt
Modified: 2010-10-28 16:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
git format-patch style patch (27.13 KB, patch)
2010-10-22 14:06 UTC, Luis de Bethencourt
rejected Details | Review
[PATCH] gaudi effects: basing on GstBaseTransform instead of GstElement. (18.01 KB, patch)
2010-10-22 14:57 UTC, Luis de Bethencourt
needs-work Details | Review
[PATCH] gaudi effects: made filter parameters dynamic and controllable (11.57 KB, patch)
2010-10-22 14:57 UTC, Luis de Bethencourt
needs-work Details | Review
[PATCH] gaudi effects: made filter parameters dynamic and controllable (35.64 KB, patch)
2010-10-27 13:05 UTC, Luis de Bethencourt
none Details | Review
[PATCH] gaudi effects: made filter parameters dynamic and controllable (35.52 KB, patch)
2010-10-27 13:17 UTC, Luis de Bethencourt
none Details | Review
git format-patch style patch (36.37 KB, patch)
2010-10-28 15:24 UTC, Luis de Bethencourt
committed Details | Review

Description Luis de Bethencourt 2010-10-22 11:42:53 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.
Comment 1 Luis de Bethencourt 2010-10-22 14:06:33 UTC
Created attachment 173006 [details] [review]
git format-patch style patch
Comment 2 Luis de Bethencourt 2010-10-22 14:57:08 UTC
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(-)
Comment 3 Luis de Bethencourt 2010-10-22 14:57:13 UTC
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(-)
Comment 4 Luis de Bethencourt 2010-10-22 15:00:06 UTC
Review of attachment 173006 [details] [review]:

IGNORE this patch
Comment 5 Luis de Bethencourt 2010-10-22 15:06:58 UTC
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
Comment 6 Sebastian Dröge (slomo) 2010-10-23 19:18:29 UTC
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
Comment 7 Sebastian Dröge (slomo) 2010-10-23 19:22:09 UTC
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
Comment 8 Luis de Bethencourt 2010-10-23 20:32:38 UTC
(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.
Comment 9 Luis de Bethencourt 2010-10-23 20:35:26 UTC
(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! :)
Comment 10 Luis de Bethencourt 2010-10-27 13:05:28 UTC
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(-)
Comment 11 Luis de Bethencourt 2010-10-27 13:17:07 UTC
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(-)
Comment 12 Sebastian Dröge (slomo) 2010-10-28 13:27:02 UTC
Looks good, now make this in git format-patch format and I'll push it :)
Comment 13 Luis de Bethencourt 2010-10-28 15:24:27 UTC
Created attachment 173403 [details] [review]
git format-patch style patch

Thanks a million for the review Sebastian! :)
Comment 14 Sebastian Dröge (slomo) 2010-10-28 16:51:47 UTC
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