gst/videocrop/gstvideocrop.*: Fix renegotiation when changing properties using the new basetransform features. Fixes ...

Original commit message from CVS:
* gst/videocrop/gstvideocrop.c: (gst_video_crop_init),
(gst_video_crop_transform), (gst_video_crop_transform_caps),
(gst_video_crop_set_caps), (gst_video_crop_set_property):
* gst/videocrop/gstvideocrop.h:
Fix renegotiation when changing properties using the new basetransform
features. Fixes #561502.
* tests/icles/Makefile.am:
* tests/icles/videocrop2-test.c: (make_pipeline), (main):
Add crazy interactive test unit for dynamically changing properties.

ChangeLog

@@ -1,3 +1,16 @@
+2008-11-25  Wim Taymans  <wim.taymans@collabora.co.uk>
+
+	* gst/videocrop/gstvideocrop.c: (gst_video_crop_init),
+	(gst_video_crop_transform), (gst_video_crop_transform_caps),
+	(gst_video_crop_set_caps), (gst_video_crop_set_property):
+	* gst/videocrop/gstvideocrop.h:
+	Fix renegotiation when changing properties using the new basetransform
+	features. Fixes #561502.
+
+	* tests/icles/Makefile.am:
+	* tests/icles/videocrop2-test.c: (make_pipeline), (main):
+	Add crazy interactive test unit for dynamically changing properties.
+
 2008-11-24  Wim Taymans  <wim.taymans@collabora.co.uk>
 
 	* gst/rtsp/gstrtspsrc.c: (new_session_pad),

gst/videocrop/gstvideocrop.c

@@ -193,8 +193,6 @@ gst_video_crop_init (GstVideoCrop * vcrop, GstVideoCropClass * klass)
   vcrop->crop_left = 0;
   vcrop->crop_top = 0;
   vcrop->crop_bottom = 0;
-  vcrop->noop = TRUE;
-  GST_BASE_TRANSFORM (vcrop)->passthrough = vcrop->noop;
 }
 
 static gboolean
@@ -432,17 +430,6 @@ gst_video_crop_transform (GstBaseTransform * trans, GstBuffer * inbuf,
 {
   GstVideoCrop *vcrop = GST_VIDEO_CROP (trans);
 
-  /* we should be operating in passthrough mode if there's nothing to do */
-  g_assert (vcrop->noop == FALSE);
-
-  GST_OBJECT_LOCK (vcrop);
-
-  if (G_UNLIKELY ((vcrop->crop_left + vcrop->crop_right) >= vcrop->in.width ||
-          (vcrop->crop_top + vcrop->crop_bottom) >= vcrop->in.height)) {
-    GST_OBJECT_UNLOCK (vcrop);
-    goto cropping_too_much;
-  }
-
   switch (vcrop->in.packing) {
     case VIDEO_CROP_PIXEL_FORMAT_PACKED_SIMPLE:
       gst_video_crop_transform_packed_simple (vcrop, inbuf, outbuf);
@@ -457,17 +444,7 @@ gst_video_crop_transform (GstBaseTransform * trans, GstBuffer * inbuf,
       g_assert_not_reached ();
   }
 
-  GST_OBJECT_UNLOCK (vcrop);
-
   return GST_FLOW_OK;
-
-cropping_too_much:
-  {
-    /* is there a better error code for this? */
-    GST_ELEMENT_ERROR (vcrop, LIBRARY, SETTINGS, (NULL),
-        ("Can't crop more pixels than there are"));
-    return GST_FLOW_ERROR;
-  }
 }
 
 static gint
@@ -537,14 +514,8 @@ gst_video_crop_transform_caps (GstBaseTransform * trans,
 
   GST_OBJECT_LOCK (vcrop);
 
-  GST_LOG_OBJECT (vcrop, "l=%d,r=%d,b=%d,t=%d noop=%d",
-      vcrop->crop_left, vcrop->crop_right, vcrop->crop_bottom,
-      vcrop->crop_top, vcrop->noop);
-
-  if (vcrop->noop) {
-    GST_OBJECT_UNLOCK (vcrop);
-    return gst_caps_ref (caps);
-  }
+  GST_LOG_OBJECT (vcrop, "l=%d,r=%d,b=%d,t=%d",
+      vcrop->crop_left, vcrop->crop_right, vcrop->crop_bottom, vcrop->crop_top);
 
   if (direction == GST_PAD_SRC) {
     dx = vcrop->crop_left + vcrop->crop_right;
@@ -606,39 +577,48 @@ gst_video_crop_set_caps (GstBaseTransform * trans, GstCaps * incaps,
 {
   GstVideoCrop *crop = GST_VIDEO_CROP (trans);
 
-  if (!gst_video_crop_get_image_details_from_caps (crop, &crop->in, incaps)) {
-    GST_DEBUG_OBJECT (crop, "failed to parse input caps %" GST_PTR_FORMAT,
-        incaps);
-    return FALSE;
-  }
+  if (!gst_video_crop_get_image_details_from_caps (crop, &crop->in, incaps))
+    goto wrong_input;
 
-  if (!gst_video_crop_get_image_details_from_caps (crop, &crop->out, outcaps)) {
-    GST_DEBUG_OBJECT (crop, "failed to parse output caps %" GST_PTR_FORMAT,
-        outcaps);
-    return FALSE;
-  }
+  if (!gst_video_crop_get_image_details_from_caps (crop, &crop->out, outcaps))
+    goto wrong_output;
+
+  if (G_UNLIKELY ((crop->crop_left + crop->crop_right) >= crop->in.width ||
+          (crop->crop_top + crop->crop_bottom) >= crop->in.height))
+    goto cropping_too_much;
 
   GST_LOG_OBJECT (crop, "incaps = %" GST_PTR_FORMAT ", outcaps = %"
       GST_PTR_FORMAT, incaps, outcaps);
 
-  return TRUE;
-}
+  if ((crop->crop_left | crop->crop_right | crop->crop_top | crop->
+          crop_bottom) == 0) {
+    GST_LOG_OBJECT (crop, "we are using passthrough");
+    gst_base_transform_set_passthrough (GST_BASE_TRANSFORM (crop), TRUE);
+  } else {
+    GST_LOG_OBJECT (crop, "we are not using passthrough");
+    gst_base_transform_set_passthrough (GST_BASE_TRANSFORM (crop), FALSE);
+  }
 
-/* This is extremely hackish, but the only way to force basetransform to
- * renegotiated at the moment. There should really be a basetransform
- * function for this */
-static void
-gst_videocrop_clear_negotiated_caps_locked (GstVideoCrop * crop)
-{
-  GST_LOG_OBJECT (crop, "clearing negotiated caps");
-  GST_BASE_TRANSFORM (crop)->negotiated = FALSE;
-  gst_caps_replace (&GST_PAD_CAPS (GST_BASE_TRANSFORM (crop)->srcpad), NULL);
-  gst_caps_replace (&GST_PAD_CAPS (GST_BASE_TRANSFORM (crop)->sinkpad), NULL);
-  gst_caps_replace (&GST_BASE_TRANSFORM (crop)->cache_caps1, NULL);
-  GST_BASE_TRANSFORM (crop)->cache_caps1_size = 0;
-  gst_caps_replace (&GST_BASE_TRANSFORM (crop)->cache_caps2, NULL);
-  GST_BASE_TRANSFORM (crop)->cache_caps2_size = 0;
-  GST_LOG_OBJECT (crop, "clearing caps done");
+  return TRUE;
+
+  /* ERROR */
+wrong_input:
+  {
+    GST_DEBUG_OBJECT (crop, "failed to parse input caps %" GST_PTR_FORMAT,
+        incaps);
+    return FALSE;
+  }
+wrong_output:
+  {
+    GST_DEBUG_OBJECT (crop, "failed to parse output caps %" GST_PTR_FORMAT,
+        outcaps);
+    return FALSE;
+  }
+cropping_too_much:
+  {
+    GST_DEBUG_OBJECT (crop, "we are cropping too much");
+    return FALSE;
+  }
 }
 
 static void
@@ -649,6 +629,10 @@ gst_video_crop_set_property (GObject * object, guint prop_id,
 
   video_crop = GST_VIDEO_CROP (object);
 
+  /* don't modify while we are transforming */
+  GST_BASE_TRANSFORM_LOCK (GST_BASE_TRANSFORM_CAST (video_crop));
+
+  /* protect with the object lock so that we can read them */
   GST_OBJECT_LOCK (video_crop);
   switch (prop_id) {
     case ARG_LEFT:
@@ -667,17 +651,14 @@ gst_video_crop_set_property (GObject * object, guint prop_id,
       G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
       break;
   }
-
-  video_crop->noop = ((video_crop->crop_left | video_crop->crop_right |
-          video_crop->crop_top | video_crop->crop_bottom) == 0);
-
-  GST_LOG_OBJECT (video_crop, "l=%d,r=%d,b=%d,t=%d noop=%d",
-      video_crop->crop_left, video_crop->crop_right, video_crop->crop_bottom,
-      video_crop->crop_top, video_crop->noop);
-
-  GST_BASE_TRANSFORM (video_crop)->passthrough = video_crop->noop;
-  gst_videocrop_clear_negotiated_caps_locked (video_crop);
   GST_OBJECT_UNLOCK (video_crop);
+
+  GST_LOG_OBJECT (video_crop, "l=%d,r=%d,b=%d,t=%d",
+      video_crop->crop_left, video_crop->crop_right, video_crop->crop_bottom,
+      video_crop->crop_top);
+
+  gst_base_transform_reconfigure (GST_BASE_TRANSFORM (video_crop));
+  GST_BASE_TRANSFORM_UNLOCK (GST_BASE_TRANSFORM_CAST (video_crop));
 }
 
 static void

gst/videocrop/gstvideocrop.h

@@ -70,8 +70,6 @@ struct _GstVideoCrop
   GstBaseTransform basetransform;
 
   /*< private >*/
-  gboolean noop;                /* TRUE if crop_left,_right,_top and _bottom are all 0 */
-
   gint crop_left;
   gint crop_right;
   gint crop_top;

tests/icles/Makefile.am

@@ -40,5 +40,10 @@ videobox_test_CFLAGS  = $(GST_CFLAGS)
 videobox_test_LDADD   = $(GST_LIBS)
 videobox_test_LDFLAGS = $(GST_PLUGIN_LDFLAGS)
 
-noinst_PROGRAMS = $(GTK_TESTS) $(V4L2_TESTS) $(X_TESTS) videocrop-test videobox-test
+videocrop2_test_SOURCES = videocrop2-test.c
+videocrop2_test_CFLAGS  = $(GST_CFLAGS)
+videocrop2_test_LDADD   = $(GST_LIBS)
+videocrop2_test_LDFLAGS = $(GST_PLUGIN_LDFLAGS)
+
+noinst_PROGRAMS = $(GTK_TESTS) $(V4L2_TESTS) $(X_TESTS) videocrop-test videobox-test videocrop2-test
 

tests/icles/videocrop2-test.c

@@ -0,0 +1,141 @@
+/* GStreamer interactive videocrop test
+ *
+ * Copyright (C) 2008 Wim Taymans <wim.taymans@gmail.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Library General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Library General Public License for more details.
+ *
+ * You should have received a copy of the GNU Library General Public
+ * License along with this library; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 02111-1307, USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include <stdlib.h>
+
+#include <gst/gst.h>
+
+static GstElement *
+make_pipeline (gint type)
+{
+  GstElement *result;
+  gchar *pstr;
+
+  switch (type) {
+    case 0:
+      pstr = g_strdup_printf ("videotestsrc ! videocrop name=crop ! "
+          "xvimagesink");
+      break;
+    default:
+      return NULL;
+  }
+
+  result = gst_parse_launch_full (pstr, NULL, GST_PARSE_FLAG_NONE, NULL);
+  g_print ("created test %d: \"%s\"\n", type, pstr);
+  g_free (pstr);
+
+  return result;
+}
+
+#define MAX_ROUND 500
+
+int
+main (int argc, char **argv)
+{
+  GstElement *pipe, *crop;
+  gint left, right;
+  gint top, bottom;
+  gint ldir, rdir;
+  gint tdir, bdir;
+  gint round, type, stop;
+
+  gst_init (&argc, &argv);
+
+  type = 0;
+  stop = -1;
+
+  if (argc > 1) {
+    type = atoi (argv[1]);
+    stop = type + 1;
+  }
+
+  while (TRUE) {
+    GstMessage *message;
+
+    pipe = make_pipeline (type);
+    if (pipe == NULL)
+      break;
+
+    crop = gst_bin_get_by_name (GST_BIN (pipe), "crop");
+    g_assert (crop);
+
+    top = bottom = left = right = 0;
+    tdir = bdir = 10;
+    ldir = rdir = 10;
+
+    for (round = 0; round < MAX_ROUND; round++) {
+      g_print ("crop to %4d %4d %4d %4d (%d/%d)   \r", top, bottom, left, right,
+          round, MAX_ROUND);
+
+      g_object_set (crop, "top", top, "bottom", bottom, "left", left, "right",
+          right, NULL);
+
+      if (round == 0)
+        gst_element_set_state (pipe, GST_STATE_PLAYING);
+
+      top += tdir;
+      if (top >= 80)
+        tdir = -10;
+      else if (top < 10)
+        tdir = 10;
+
+      bottom += bdir;
+      if (bottom >= 60)
+        bdir = -10;
+      else if (bottom < 10)
+        bdir = 10;
+
+      left += ldir;
+      if (left >= 100)
+        ldir = -10;
+      else if (left < 10)
+        ldir = 10;
+
+      right += rdir;
+      if (right >= 80)
+        rdir = -10;
+      else if (right < 10)
+        rdir = 10;
+
+      message =
+          gst_bus_poll (GST_ELEMENT_BUS (pipe), GST_MESSAGE_ERROR,
+          50 * GST_MSECOND);
+      if (message) {
+        g_print ("got error           \n");
+
+        gst_message_unref (message);
+      }
+    }
+    g_print ("test %d done                    \n", type);
+
+    gst_object_unref (crop);
+    gst_element_set_state (pipe, GST_STATE_NULL);
+    gst_object_unref (pipe);
+
+    type++;
+    if (type == stop)
+      break;
+  }
+  return 0;
+}