GNOME Bugzilla – Bug 738124
validate: Design and implement an interface for setting a report level.
Last modified: 2014-11-08 14:15:10 UTC
This first comment will not deal with implementation details. Problem: Currently, the output of gst_validate_runner_printf is * Too verbose, even though the subchain issue concatenation implemented at https://bugzilla.gnome.org/show_bug.cgi?id=735665 will partly solve that situation. Some tests in the GES validation suite report over 600 issues, trimmed down to 200 by the subchain concatenation, but that's still thousands of lines that a normal human brain such as mine is not prepared to parse. * Not configurable. When trying to validate a single element, the user should not have to dig in the reports to find the ones related to that element. Aside from that, validate currently concatenates issues that have the same type inside an element, as part of the "no-flooding" effort. However, in certain situations, the user might want to see all these reports, for example if the element pushes 10 buffers after it pushed an EOS, he/she might want to have informations about all these buffers, their timestamps et caetera. Use case: The user wants to validate the behaviour of an arbitrary object. Validate should provide a way to let the user specify: * "please give me a report about the issues this object has in a synthetic way, and don't talk to me about the rest of the pipeline, that's not my problem". * "please give me a detailed report about the issues this object has, and provide me with a synthetic report of the issues of the pipeline it's contained in" This object can be a bin, an element or a pad. It can be specified by name or by type (h264parse / my_h264_parser_12). Proposed solution: An environment variable akin to GST_DEBUG, and API so that this can be set on the fly, for use in scenarios for example. four possible levels of reporting, applicable globally or per object: * none: no report at all, useful in combination with a per-element reporting level * synthetic: Summary of the distinct issues found, with no details: In the global case: warning: buffer was received after EOS Detected X times on <audioconvert6:sink, audioconvert6:src, audioresample7:sink, audioresample7:src, smart-adder-adder:sink_2> Detected X times on <audioconvert7:sink, audioconvert7:src, audioresample8:sink, audioresample8:src, smart-adder-adder:sink_2> Description : a pad shouldn't receive any more buffers after it gets EOS In the per-object case: warning on <object>: a pad shouldn't receive any more buffers after it gets EOS, detected X times. * subchain: In the global case, same behaviour as with the patches in https://bugzilla.gnome.org/show_bug.cgi?id=735665, ie distinct issues on different subchains / pads are reported, for example: warning: buffer was received after EOS Detected X times on <audioconvert6:sink, audioconvert6:src, audioresample7:sink, audioresample7:src, smart-adder-adder:sink_2> Details: Received buffer 0xdeadbeef with timestamp 42:42:424242 after EOS. Description : a pad shouldn't receive any more buffers after it gets EOS In the per-object case, reports the cases where an issue was first reported on that object, for example in the above case, the issue will be reported if the requested object is audioconvert, but not if it is audioresample. * monitor: In the global case, same behaviour as currently. In the per-object case, reports separately the cases where an issue was noticed for an object, be this object the start of a chain or not. Issues still get concatenated inside the object. * all: All reports, with no intra-object concatenating. Will be extremely verbose when set globally but ¯\_(ツ)_/¯ . Environment variable: GST_VALIDATE_REPORT_LEVEL, a comma-separated list of (optional) debug categories and levels. No debug category means global level. Example: GST_VALIDATE_REPORT_LEVEL=synthetic,h264parse:all API: gst_validate_set_report_level_from_string (const gchar *);
Created attachment 288155 [details] [review] Initial work Submitting this patch for early review
Created attachment 288156 [details] [review] Ad code to parse the environment variable Submitting this patch for early review
Created attachment 288314 [details] [review] validate-runner: report-level initial work. + Defines reporting levels and document them. + Add API to get the default level. + fix indentation. + fix some typos. + Add the beginning of a reporting test.
Created attachment 288315 [details] [review] validate-runner: Add code to parse GST_VALIDATE_REPORT_LEVEL. + Extend the tests. + [API] gst_validate_runner_get_default_reporting_level + [API] gst_validate_runner_get_reporting_level_for_name
Created attachment 288316 [details] [review] validate-monitor: Determine the reporting level at setup.
Created attachment 288317 [details] [review] tests: Test reports refcounts. + Set the element monitor on the element as qdata.
Created attachment 288318 [details] [review] validate-runner / reporter: Sanitize reports refcounting. The previous code worked but was confusing, the runner didn't actually take the ref it was releasing later. + Fix indentation.
Created attachment 288319 [details] [review] validate-runner / monitor: Let the user single out pads. That's some pretty specific code but it should be helpful. The following syntax can be used : element-name__sink-name, separator can be discussed. + Free return of gst_object_get_name.
Created attachment 288320 [details] [review] tests: Check monitors correctly determine their reporting level. + [API] gst_validate_reporter_get_reporting_level
Created attachment 288321 [details] [review] validate-runner: Implement REPORT_NONE for global reporting. Yeah that was tough. Helpful already though, for example: GST_VALIDATE_REPORT_LEVEL=none,x:all gst-validate src name=x ! sink will only report issues reported by the source. + Add test.
Created attachment 288322 [details] [review] validate-report: split up printf function. + And expose resulting subroutines.
Created attachment 288323 [details] [review] validate-runner: implement synthetic report. + Fix criticals logic in validate_runner_printf + Update padmonitor tests
Created attachment 288324 [details] [review] validate-report: Add a reporting level field and setter.
Created attachment 288325 [details] [review] validate-report: Set conditions in which a report can't be master.
Created attachment 288326 [details] [review] pad-monitor: simplify and correct issue concatenation. We hereby consider that only immediately upstream pads have to be checked for a master report.
Created attachment 288327 [details] [review] pad-monitor: check set_master_report return value.
Created attachment 288328 [details] [review] validate-pad-monitor / runner: Check per-object reporting levels.
Created attachment 288329 [details] [review] pad-monitor: Check last flow return for EOS.
Created attachment 288330 [details] [review] validate-report / reporter: rework the way we repeat issues. + runner: update reports count algorithm.
Review of attachment 288155 [details] [review]: Rejecting as it is obsoleted
Review of attachment 288156 [details] [review]: rejected as it is obsoleted
These commits implement the proposed interface. Three of them were developed on the way but don't have a direct relation to the problem: tests: Test reports refcounts. validate-runner / reporter: Sanitize reports refcounting. pad-monitor: Check last flow return for EOS.
Review of attachment 288317 [details] [review]: ::: 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.
Review of attachment 288314 [details] [review]: ::: 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...
Review of attachment 288315 [details] [review]: OK
Review of attachment 288316 [details] [review]: ::: 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 :)
Review of attachment 288318 [details] [review]: OK
Review of attachment 288319 [details] [review]: It should actually be element-name::pad-name and then in the code you do envvar.replace("::", "__") before actually splitting it.
Review of attachment 288320 [details] [review]: Looks good.
Review of attachment 288321 [details] [review]: ::: 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? 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 ?
Review of attachment 288322 [details] [review]: 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).
Review of attachment 288323 [details] [review]: ::: 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 @@ +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? ::: 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? :)
Review of attachment 288324 [details] [review]: OK
Review of attachment 288325 [details] [review]: Isn't that going to cause issue in the end?
Review of attachment 288326 [details] [review]: Why is that?
Review of attachment 288327 [details] [review]: OK, so that should probably be merged with the patch where you make it possible for set_master to return FALSE.
Review of attachment 288328 [details] [review]: OK
Review of attachment 288329 [details] [review]: I think we already do that ourself, don't we?!
Review of attachment 288330 [details] [review]: ::: validate/gst/validate/gst-validate-reporter.c @@ +177,3 @@ + GST_VALIDATE_REPORTING_LEVEL_UNKNOWN; + + if (priv->runner) runners are mandatory ::: 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? :)
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] [review]: > > ::: 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] [review]: > > ::: 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] [review]: > > ::: 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] [review]: > > 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] [review]: > > ::: 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] [review]: > > 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] [review]: > > ::: 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] [review]: > > 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] [review]: > > 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] [review]: > > 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] [review]: > > I think we already do that ourself, don't we?! That patch was incorrect (In reply to comment #42) > Review of attachment 288330 [details] [review]: > > ::: 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 :)
Review of attachment 288314 [details] [review]: commited
Review of attachment 288315 [details] [review]: commited
Review of attachment 288316 [details] [review]: commited
Review of attachment 288317 [details] [review]: commited
Review of attachment 288318 [details] [review]: commited
Review of attachment 288319 [details] [review]: commited
Review of attachment 288320 [details] [review]: commited
Review of attachment 288321 [details] [review]: commited
Review of attachment 288322 [details] [review]: commited
Review of attachment 288323 [details] [review]: commited
Review of attachment 288324 [details] [review]: commited
Review of attachment 288325 [details] [review]: commited
Review of attachment 288326 [details] [review]: commited
Review of attachment 288327 [details] [review]: commited
Review of attachment 288328 [details] [review]: commited
Review of attachment 288329 [details] [review]: commited
Review of attachment 288330 [details] [review]: commited
Review of attachment 288329 [details] [review]: actually rejected
Thanks for the review, closing