[Bug 703093] videomeasure: port to 1.0

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=703093
  GStreamer | gst-plugins-bad | git

Olivier Crete (Tester) <olivier.crete> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #249566|none                        |needs-work
             status|                            |

--- Comment #5 from Olivier Crete (Tester) <olivier.crete at ocrete.ca> 2013-12-06 03:40:47 UTC ---
Review of attachment 249566:
 --> (https://bugzilla.gnome.org/review?bug=703093&attachment=249566)

A couple more review comments.

::: gst/videomeasure/gstvideomeasure_ssim.c
@@ +220,3 @@
+    caps = gst_pad_get_pad_template_caps (ssim->orig);
+  } else {
+    caps = gst_caps_copy (ssim->sinkcaps);

You can use gst_caps_ref() here, no need to make a copy

@@ +500,3 @@
       }
+      sigma_o = sigma_o / elsumm; /*sigma_o = sqrt (sigma_o / elsumm)*/
+      sigma_m = sigma_m / elsumm; /*sigma_m = sqrt (sigma_m / elsumm)*/

Why did you remove the sqrt() call here ?

@@ +504,3 @@
       tmp1 = (2 * mu_o * mu_m + ssim->const1) * (2 * sigma_om + ssim->const2)
/
           ((mu_o * mu_o + mu_m * mu_m + ssim->const1) *
+          (sigma_o + sigma_m + ssim->const2)); /* (sigma_o^2 + sigma_m^2 +
ssim->const2) */

And the squaring here..

@@ +548,3 @@
+  GST_DEBUG_OBJECT (ssim, "format is %s", gst_video_format_to_string (fmt));
+  if (!GST_VIDEO_INFO_IS_YUV (&ssim->vi))
+    goto not_supported;

This isn't required, the caps will always be a subset of the pad template caps.

@@ +550,3 @@
+    goto not_supported;
+
+  GST_INFO_OBJECT (ssim, "parse_caps sets ssim to yuv format "

Use GST_DEBUG_OBJECT()

@@ +566,3 @@
+      break;
+    default:
+      goto not_supported;

Again, not required, this is specified in the pad template.

@@ +594,3 @@
      * look kinda funny :)
      */
+    gst_caps_append_structure (ssim->srccaps,

You can just do:
gst_caps_new_simple(), no need to do new_emptoy then append a structure

@@ +852,3 @@
+        gst_query_set_caps_result (query, caps);
+        res = TRUE;
+      } else

Use if {} else {}, don't with and without {}

@@ +857,3 @@
     default:
       /* FIXME, needs a custom query handler because we have multiple
+       * sinkpads */

Just call gst_pad_query_default(), the default handler handles multiple pads
now

@@ +907,3 @@
       GST_OBJECT_UNLOCK (ssim->collect);

+      result = gst_ssim_push_sink_event (ssim, event);

call gst_collect_pad_src_event_default(), it will do the forwarding for you

@@ +916,3 @@
     default:
       /* just forward the rest for now */
+      result = gst_ssim_push_sink_event (ssim, event);

same here

@@ +1136,3 @@
     goto unnamed_pad;

+  if (strncmp (padname, "modified_", 9) == 0) {

g_str_has_prefix()

@@ +1157,1 @@
 #if GLIB_CHECK_VERSION(2,29,5)

Remove this, GStreamer 1.0 requires GLib 2.32

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.