Created attachment 247788 [details] [review]
Port videomeasure to 1.0

The code is still a bit banged up (i could never figure out the caps and query thing, so that code is just stitched together from the one i've copied from the `interleave' and 'videomixer' elements), but it works.

Cool, thanks.

Review of attachment 247788 [details] [review]:

Stopped review at some point, but the negotiation here looks rather complicated

::: gst/videomeasure/gstvideomeasure_collector.c
@@ +345,3 @@
+  gst_element_class_set_static_metadata (element_class,
+      "Video rate adjuster", "Filter/Effect/Video",
+      "Drops/duplicates/adjusts timestamps on video frames to make a perfect stream",

Copy & paste from videorate it seems, can you fix that while you're at it? ;)

::: gst/videomeasure/gstvideomeasure_ssim.c
@@ +227,2 @@
+static gboolean
+gst_ssim_sink_getcaps (GstPad *pad, GstObject *parent, GstQuery *query)

For consistency, maybe make the signature GstCaps * getcaps (GstSSim *ssim, GstCaps *filter);

@@ +243,3 @@
   GST_OBJECT_UNLOCK (ssim);
 
+  if (filter && caps) {

caps should always be != NULL here

@@ -431,3 @@
             for (ix = winstart_x; ix <= winend_x; ix++) {
               weight = ssim->weights[weight_offset + ix];
-              mu_o += weight * org[pixel_offset + ix];

Why do you change the algorithm, should be a separate commit

@@ +515,3 @@
               tmp1 = org_with_offset[ix] - mu_o;
               tmp2 = mod_with_offset[ix] - mu_m;
+              weight = weights_with_offset[ix];

Same here

@@ +530,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 here

@@ +613,2 @@
     GST_DEBUG_OBJECT (ssim, "checking caps on pad %p", otherpad);
     if (direction == GST_PAD_SINK) {

Caps are only set in one direction nowadays: downstream

@@ +626,2 @@
       GST_DEBUG_OBJECT (ssim, "old caps on pad %p,%s were %s", otherpad,
+          padname, capstr);

Just use GST_PTR_FORMAT here to print caps. This will evne work on windows with 1.1

@@ +658,3 @@
+  ssim->height = height;
+  ssim->frame_rate = fps_n;
+  ssim->frame_rate_base = fps_d;

Maybe just store the GstVideoInfo in the instance struct

Created attachment 249566 [details] [review]
Port videomeasure to 1.0 v2

Made all the adjustments suggested so far.

I have a feeling that this is going to take a lo-o-ong time...

Review of attachment 249566 [details] [review]:

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

> @@ +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..

Exactly because it's squared later. No sense sqrt'ing it, and then squaring it back.

As for other comments - ok, i'll try to work myself up to fix these.

Do such changes in a separate patch please, and only port to 1.0 in the main one

Created attachment 302403 [details] [review]
videomeasure: port to 1.0 v3

Made all the adjustments suggested by ocrete.

Created attachment 302404 [details] [review]
videomeasure: optimize SSIM calculation slightly

Split the micro-optimization part into separate patch

Created attachment 302406 [details] [review]
videomeasure: optimize SSIM calculation slightly v2

Forgot to add this change to the other function as well. D'oh!

Created attachment 302407 [details] [review]
videomeasure: port to 1.0 v4

Forgot to remove videomeasure from the list of unported plugins...

Did some testing, this microoptimization reduces average SSIM calculation time for a frame by ~500 microseconds (average time from 53089542 nanoseconds to 52541066 nanoseconds).

Yay...?

Created attachment 302524 [details] [review]
videomeasure: port to 1.0 v5

Style fixed by gst-indent

Created attachment 302525 [details] [review]
videomeasure: optimize SSIM calculation slightly v3

Style fixed by gst-indent

Created attachment 302614 [details] [review]
videomeasure: port to 1.0 v6

Used fixed gst-indent to format the patch

Created attachment 302615 [details] [review]
videomeasure: optimize SSIM calculation slightly v4

Used fixed gst-indent to format the patch

We decided to got with the new IQA (https://bugzilla.gnome.org/show_bug.cgi?id=751324) as it is simpler, it is based on GstVideoAggregator which is more future proof. Also the lib used (dssim) is faster from some experiment we made.

Thanks a lot for that anyway.

I spent few minutes to make a comparison. I think stating that IQA covers videomeasurement is an over statement. Here's what I found:

- IQA pretends but don't really seem to support 1+N streams
- IQA is strictly DSSIM and does not help adding anything else
- IQA does not support non RGB output

Without addressing that, why do we trash this effort ? -bad is like the staging area, I believe both solution can coexist until we have a full featured solution.

IQA can be made to work with some refactoring. Don't try to have a single class that can do multiple metrics, have multiple classes, like GstIQADSSIM, GstIQAPSNR, one per metric; that way GstVideoAggregator and its inflexibility won't be a problem - GstIQAGMS will just have different caps template, one that accepts YUV instead of RGB.

1+N won't happen that way, but my intention behind having 1+N in videomeasure was mostly due to performance concerns (SSIM includes a blur pass over each video stream before comparison; 1+N allows us to do blur of the original stream once for 1+1 comparison, then re-use it for all other 1+(N-1) comparisons). Other algorithms don't seem to need that, and DSSIM can't do that, so this is not something i'd fight about.

I *would* like IQA to be able to push measurements as events, and have a dedicated element for catching these and dumping them into CSV files. That way it would be possible to do measurements *entirely* with gst-launch, no need to write an application to listen for bus messages (where IQA currently sends its measurements) or dump them to stdout with -m and then have to parse the output. These approaches are not mutually exclusive, IQA can do both (events and bus messages).

Stride problems will remain in IQADSSIM, i'll let IQA maintainer handle that. I'll try to ensure that IQAGMS doesn't have these.

RGB-only output...depends on how much code different IQA classes will have in common. If they will have common output code, they might all output RGB (a heat map currently). The code for GMS that i have outputs GRAY8 (just 0..1 mapped into 0..255, no fancy heatmaps; this can be changed, if needed).

(In reply to Nicolas Dufresne (stormer) from comment #18)
> I spent few minutes to make a comparison. I think stating that IQA covers
> videomeasurement is an over statement. Here's what I found:
> 
> - IQA pretends but don't really seem to support 1+N streams

That is not the case. If you read a little better you'll see it does output messages for each pad, the implementation is very straightforward that's easy enough to figure.

> - IQA is strictly DSSIM and does not help adding anything else

I don't know what help you'd expect, you get n video frames and the ability to post messages, if you have more suggestions don't hesitate.

> - IQA does not support non RGB output

That's not the problem here, but OK

> 
> Without addressing that, why do we trash this effort ? -bad is like the
> staging area, I believe both solution can coexist until we have a full
> featured solution.

I never said *anything* about trashing one solution or another, I just implemented this one year ago, dssim works measurably (no pun intended) better than what we had previously in the videomeasure element, as the correlation against annotated datasets is significantly higher, while being 15 times faster.

Inheriting from videoaggregator and wrapping dssim means the actual code is less than 400 lines, which also counts as a win in my book.

Now I proposed this against *-bad*, intentionally because I had neither proof-tested it extensively, nor tried to implement different metrics in there. The fact that input is restricted to RGBA because of dssim is indeed inconvenient, anyway I'm pretty sure all the state of the art algorithms can be implemented against RGBA images.

Anyway, here's my personal opinion:

All contributions to -bad should be welcome with open arms, as in the case of an API breakage (gst 2.0), porting the -bad plugins isn't an obligation, right?

I tend to think the solution I picked has the advantage of simplicity, and ideally will implement more metrics, now there is the constraint of input formats, which isn't easily solvable, and I don't have anything against people doing their own thing, as long as they let me do mine too!

See bug 774944

Poked around a bit. If we start from the assumption that videomeasure needs to use GstAggregator (GstVideoAggregator is already deemed too highlevel due to its tendency to convert stuff) to reduce the amount of code to maintain, it means that it can't ever produce more than one stream, because GstAggregator has only one src pad.

That means that information-lossless performance can only happen when one metric is computed on two input streams (a reference stream and a modified stream), producing one output stream (it could be 1-channel height-map (GRAY8) or heat-map (RGB) or multi-channel height-map (RGB or Y444), that depends on the implementation and could be user-configurable, if needed).

Three input streams (1 ref, 2 mods) -> need 2 output streams (one for each mod), and GstAggregator won't allow that. An earlier suggestion to stitch multiple output streams into one big stream is ridiculous (stitching -> more code, splitting -> more code). Or output has to be simply dropped -> information loss.

Two metrics simultaneously -> twice as much output streams (one for each mod, multiplied by 2, for each metric), again, not supported by GstAggregator. Stitching won't work at all, since output might require completely different data formats.

That is why the only option within the must-use-GstAggregator paradigm that i would agree on is to have a 2->1 element that can only compute one metric at a time. It might support multiple metrics, but only one can be enabled.

Conversion is done by the user, using videoconvert.
Multiple metrics or multiple mod streams are done by the user, using tee (this can also be faster, because tees can be paired with queues, separating each measuring element into its own thread).

Not using GstAggregator means going all the way back to original videomeasure that inherited from GstElement directly and did everything by itself.

It looks like we wanted to make iqa a sink, and we didn't know what to do with the src pad. I believe we need to find a solution to this ambiguity. It's rather confusing. I think I'd prefer having a simpler element doing 2 -> 1 with a message per metric, or a more complex one that does 1 + N -> N which covers all the use cases.

Looking deeper, it might not be that complex to create an bin that aggregates the messages, wrapping 2 -> 1 lower complexity elements. The change on IQA would be minor and in fact would simply remove some corner cases that are not really handled. That been would handle a tee to multiplex the reference properly. 

Though, I must admit, I had a first impression was that such element should be using GST_EVENT_SINK_MESSAGE, which is not possible if you aggregate, because you'd get duplicates. I'm a bit ambivalent on this feature. What I care the most is about providing new elements that does not have such ambiguity.

1 + N -> N can't be based off GstAggregator, so unless someone is willing to maintain a whole new codebase for it, that idea is off the table.

A bin that wraps a collection of one-metric-per-instance 2->1 elements sounds neat.

I'm not sure what the deal with GST_EVENT_SINK_MESSAGE is. First generation of videomeasure pushed GST_EVENT_VIDEO_MEASURE events that carried the measurement result, these events flowed downstream and could be caught by special passthrough elements (i had the measurecollector element for this), which then did something (like, write the result into a file). IQA emits bus messages with the measurement results, leaving it up to the application (if any) to catch them on the bus and do something. I prefer the event approach, but there's no reason why both can't be used simultaneously. Though there's the question of how these events (and event-catching elements) would work in the aggregate-bin mentioned above. Or would they be outside? Are there N->N passthrough base element classes (to avoid using multiple instances that do essentially the same thing)?

A GST_EVENT_SINK_MESSAGE, is a serialized event, that carry a message to be posted to the application. This message is expected to be posted right before the associated frame is to be rendered. It is a mechanism to keep the messages on sync with the streams. In this case, a sink message enabled both use cases, and keep the metric on sync with the heatmap rendering. Thinking of it, I can't figure-out how that can works with the current iqa code. How do you know which input is which metric ...

(In reply to Nicolas Dufresne (stormer) from comment #25)
> Thinking
> of it, I can't figure-out how that can works with the current iqa code. How
> do you know which input is which metric ...

You don't. Each message comes with a timestamp, so you can only link frames to metric values via timestamps (videomeasure used buffer offsets instead; again, it's possible to include both). Now that i'm thinking about it, i guess it might be valuable to be able to do so, for example, to overlay current frame-wide metric value over the video frame. Though that means, again, a tee to split the modified stream into two, put one stream through the measurement, link up the output to the overlayer, link up the second stream to the overlayer, and then somehow sync it all correctly...

That said, many metrics are very non-realtime, so any synchronization tactic is acceptable, even if it means rendering delays...

Another solution could be to:

* implement a GstAggregatorSink class, shouldn't be very difficult (GstAggregator would simply expose a vmethod to create a source pad, with the current behaviour as its default implementation, the subclass would override it and return NULL)

* Instead of outputting a heat map of the differences, these could be somehow attached to the messages, I don't know exactly how doable that would be

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/98.