[Bug 738124] validate: Design and implement an interface for setting a report level.

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=738124
  GStreamer | gst-devtools | git

--- Comment #43 from Mathieu Duponchelle <mathieu.duponchelle at epitech.eu> 2014-10-21 21:35:46 UTC ---
The branch rebased on thibault's post 1.4 is at
https://github.com/MathieuDuponchelle/gst-devtools/commits/post_1.4. Every step
of it from back to development builds and make checks. I've fixed the issues
from the review.

(In reply to comment #23)
> Review of attachment 288317 [details]:
> 
> ::: validate/tests/check/validate/padmonitor.c
> @@ +35,3 @@
> +
> +  for (tmp = reports; tmp; tmp = tmp->next) {
> +    if (((GstValidateReport *) tmp->data)->refcount != refcount)
> 
> You should rather fail_unless_equals_int here, makes it simpler to debug.

DONE

(In reply to comment #24)
> Review of attachment 288314 [details]:
> 
> ::: validate/gst/validate/gst-validate-enums.h
> @@ +64,3 @@
> +  GST_VALIDATE_REPORTING_LEVEL_ALL = 5,
> +  GST_VALIDATE_REPORTING_LEVEL_COUNT
> +} GstValidateReportingLevel;
> 
> I think GstValidateReportingDetails would be a better naming.
> 
> And name would be GST_VALIDATE_REPORT_ALL, GST_VALIDATE_REPORT_SYNTHETIC,
> etc...

DONE, commit on top

(In reply to comment #26)
> Review of attachment 288316 [details]:
> 
> ::: validate/gst/validate/gst-validate-monitor.c
> @@ +206,3 @@
> +    gst_object_unref (object);
> +    object = parent;
> +  } while (object && level == GST_VALIDATE_REPORTING_LEVEL_UNKNOWN);
> 
> So if I set level to h264parse:all , reports from h264parse::src will be taken
> into account? Makes sense I guess :)

Yep, pretty useful IMHO

(In reply to comment #30)
> Review of attachment 288319 [details]:
> 
> It should actually be element-name::pad-name and then in the code you do
> envvar.replace("::", "__") before actually splitting it.

DONE(In reply to comment #33)
> Review of attachment 288321 [details]:
> 
> ::: validate/tests/check/validate/reporting.c
> @@ +129,3 @@
> +static void
> +_create_issues (GstValidateRunner * runner)
> +{
> 
> Can't you just create reports yourself in the tests? Simply doing
> 
> GST_VALIDATE_REPORT("Some report") where you want?
> 

It works for my purpose, we discussed that in real life and you didn't seem
opposed to keeping it that way

> I looks a bit overkill to actually create pipeline + adding probes etc etc...
> to just test reporting filtering.
> 
> @@ +167,3 @@
> +  segment.stop = GST_SECOND;
> +
> +  fail_unless (gst_pad_push_event (srcpad1,
> 
> gst_check_setup_events ?

I can't, it takes the parent element.

(In reply to comment #34)
> Review of attachment 288322 [details]:
> 
> Why would you do that? Also I think we want to handle that whole report in one
> single string in the end (I have code for that).

I use these subroutines in a subsequent patch.(In reply to comment #35)
> Review of attachment 288323 [details]:
> 
> ::: validate/gst/validate/gst-validate-runner.c
> @@ +320,3 @@
> +
> +  issue_id = report->issue->issue_id;
> +  reports =
> 
> You need the lock even to retrieve the reoprts here

True that DONE

> 
> @@ +376,3 @@
>    GST_VALIDATE_RUNNER_LOCK (runner);
>    l = g_list_length (runner->priv->reports);
> +  l += g_hash_table_size (runner->priv->reports_by_type);
> 
> How is that?

Well if we group issues by type then only one issue will be reported, that was
my reasoning, the result seems legit to me.

> 
> ::: validate/tests/check/validate/padmonitor.c
> @@ -39,3 +41,3 @@
>    }
> 
> -  g_list_free_full (reports, (GDestroyNotify )gst_validate_report_unref);
> +  g_list_free_full (reports, (GDestroyNotify) gst_validate_report_unref);
> 
> There are many changes like that I can not parse in that file, you bought a
> gst-indent? :)

DONE, removed the reindentation of the padmonitor test.

(In reply to comment #37)
> Review of attachment 288325 [details]:
> 
> Isn't that going to cause issue in the end?

Why would it ? No there's absolutely no problem here :)

(In reply to comment #38)
> Review of attachment 288326 [details]:
> 
> Why is that?

We discussed that at the gstconf, it's the only correct way.

(In reply to comment #39)
> Review of attachment 288327 [details]:
> 
> OK, so that should probably be merged with the patch where you make it possible
> for set_master to return FALSE.

DONE

(In reply to comment #41)
> Review of attachment 288329 [details]:
> 
> I think we already do that ourself, don't we?!

That patch was incorrect

(In reply to comment #42)
> Review of attachment 288330 [details]:
> 
> ::: validate/gst/validate/gst-validate-reporter.c
> @@ +177,3 @@
> +        GST_VALIDATE_REPORTING_LEVEL_UNKNOWN;
> +
> +    if (priv->runner)
> 
> runners are mandatory
> 

Not really, there is a setter and that check is made in other places so I play
on the safe side :)

> ::: validate/tests/check/validate/reporting.c
> @@ +289,3 @@
> +  runner = gst_validate_runner_new ();
> +  _create_issues (runner);
> +  /* 2 issues repeated on the fakesink's sink */
> 
> Why don't you check the details here then? :)

Cause I don't really need to :)

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