GNOME Bugzilla – Bug 735665
gst-validate should concatenate its issue reporting in some cases.
Last modified: 2014-11-08 14:15:08 UTC
Output from gst-validate can very easily reach hundreds of issues reported in some cases. An example of a case where the same issue is reported for successive elements is: gst-validate-1.0 gnlurisource uri=file:///home/meh/gst-validate/gst-qa-assets/medias/mp4/mp3_h264.0.mp4 inpoint=0 duration=1000000000 ! fakesink Here, we can see that the "buffer was received after EOS" issue is reported for the h264 parser, the capsfilter and the h264 decoder. My proposal here is to make it so the issue gets reported for the whole subchain, in that format: issue: buffer was received after EOS First detected on : <h264parse0:sink> at 0:00:00.023533513 Same issue was detected on: <capsfilter0:sink> at 0:00:00.023541286 <avdec_h264-0:sink> at 0:00:00.023543930 Details : Received buffer 0x7ff9080b6610A after EOS Description : a pad shouldn't receive any more buffers after it gets EOS I'm not sure about the "Same issue was detected" format, it could also be "Last issue was detected on". We could add a --verbose switch to gst-validate to still obtain the previous behaviour. I don't know yet how that should be implemented, let's discuss that here :)
The first problem to solve here is how can we detect than 2 reported issues are actually the same? In this case, checking if report->issue and report->message are equals should be enough. But this implies that, for a specific issue, we assume that report->message will always be different for issues having different root causes. That's the case here as the message includes the address of the buffer, but can we rely on this for all the issues?
(In reply to comment #1) > The first problem to solve here is how can we detect than 2 reported issues are > actually the same? > > In this case, checking if report->issue and report->message are equals should > be enough. But this implies that, for a specific issue, we assume that > report->message will always be different for issues having different root > causes. That's the case here as the message includes the address of the buffer, > but can we rely on this for all the issues? Currently we have a pretty simple logic to detect if a GstValidateReporter already contains a GstValidateReport of the same kind. I think that what is needed to implement that feature is to be able to instead of blocking the repetition of an action for a particular monitor, it would be to 'block' it for a particular 'branch/chain' of the pipeline. (actually you would not inhibit the repetition but instead you would add the name of the element to a list of monitor/GstValidateReporter + metadatas where that issue happen on). I guess the simplest way of implementing that "per branch" issue logic would be to just go up following the pipeline upstream checking each monitor on the way. Then if one monitor has already notified that it received that type of issue, add the information about that downstream monitor also having the same issue on that specific report structure. And if we can not find any similar issue upstream, report the issue. Also the report types already have the notion of "repeat" which aims at letting the reporting system know if a particular issue should be repeated in the runner if it happens several time (currently no issue type is repeatable iirc).
Created attachment 287551 [details] [review] validate: Add a way to cleanly repeat failures in reports The same failure might happen and even if the user most probably to be overwelmed with information, we should try to expose him as much informations as possible in the reports.
Created attachment 287552 [details] [review] validate-reporter: [API] gst_validate_reporter_get_report. + Add locking.
Created attachment 287553 [details] [review] validate-pad-monitor: Reimplement reporter interface. + Do nothing there for now, except chain up.
Created attachment 287554 [details] [review] validate-reporter: Add return value to intercept_report. It will allow to drop, keep or report reports.
Created attachment 287555 [details] [review] validate-report: Make the ref / unref functions safer.
Created attachment 287556 [details] [review] validate-report: Add the notion of master / shadow reports. A master report is a report that has been detected by a monitor to stem from the same issue. It thus contains a list of "shadow reports" which it will browse when printing itself.
Created attachment 287557 [details] [review] validate-reporter: [API] + [API] gst_validate_reporter_get_reports + [API] gst_validate_reporter_get_reports_count
Created attachment 287558 [details] [review] test-utils: add a create_and_monitor element function.
Created attachment 287559 [details] [review] validate-pad-monitor: concatenate issues. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=735665 The process is to check for a similar report in intercept_report on the pads of the upstream element, set that report as the master report of the intercepted report, and return REPORTER_KEEP instead of REPORTER_REPORT.
This series of patches allows to concatenate issues, on complex test cases such as in the GES test suite, issue number is divided by an average factor of 3. Note that I based my work on a commit by Thibault that is not yet in master.
I will next open a new bug with the design for a "set-reporting-level" switch on -validate and link it there.
Review of attachment 287555 [details] [review]: ::: validate/gst/validate/gst-validate-report.c @@ -493,2 +493,3 @@ gst_validate_report_unref (GstValidateReport * report) { + if (report == NULL) Should be g_return_if_fail, nowhere in APIs NULL is tolerated in refing funcs @@ -502,2 +505,3 @@ gst_validate_report_ref (GstValidateReport * report) { + if (report == NULL) Same here
Review of attachment 287551 [details] [review]: Actually we do *not* want it as we should in the end not expose the repeating API on the ActionTypes -- the API was added only in here.
Review of attachment 287552 [details] [review]: OK
Review of attachment 287553 [details] [review]: Impressive work :P
Review of attachment 287554 [details] [review]: I would have made intercept_report return the Report itself, but that is good too.
Review of attachment 287556 [details] [review]: Add some locking, and it should be good. ::: validate/gst/validate/gst-validate-report.c @@ +679,3 @@ + report->master_report = gst_validate_report_ref (master_report); + + for (tmp = master_report->shadow_reports; tmp; tmp = tmp->next) { You will need locking around here.
Review of attachment 287557 [details] [review]: Please rework your commit message, it is not actual API as it is all internal.
Review of attachment 287558 [details] [review]: OK
Review of attachment 287559 [details] [review]: Minor comments, otherwize it looks good. ::: validate/gst/validate/gst-validate-pad-monitor.c @@ +150,3 @@ + + /* For some reason this pad isn't monitored */ + if (pad_monitor == NULL) Does it actually happen? (I guess it can racily happen) @@ -125,0 +125,101 @@ +static gboolean +_find_master_report_on_pad (GstPad * pad, GstValidateReport * report) +{ ... 98 more ... I would return right away if peerpad == NULL ... anyway. @@ +254,3 @@ + } + } + gst_object_unref (peerpad); You can unref a NULL pointer here @@ -125,0 +125,157 @@ +static gboolean +_find_master_report_on_pad (GstPad * pad, GstValidateReport * report) +{ ... 154 more ... I think you are done = TRUE here.
Review of attachment 287559 [details] [review]: ::: validate/gst/validate/gst-validate-pad-monitor.c @@ +150,3 @@ + + /* For some reason this pad isn't monitored */ + if (pad_monitor == NULL) It can happen if you decide to only monitor certain pads, as I had done by mistake while implementing the test :) @@ -125,0 +125,101 @@ +static gboolean +_find_master_report_on_pad (GstPad * pad, GstValidateReport * report) +{ ... 98 more ... indeed @@ +254,3 @@ + } + } + gst_object_unref (peerpad); True @@ -125,0 +125,157 @@ +static gboolean +_find_master_report_on_pad (GstPad * pad, GstValidateReport * report) +{ ... 154 more ... True
Created attachment 287626 [details] [review] validate-reporter: add gst_validate_reporter_get_report. + Add locking.
Created attachment 287627 [details] [review] validate-pad-monitor: Reimplement reporter interface. + Do nothing there for now, except chain up.
Created attachment 287628 [details] [review] validate-reporter: Add return value to intercept_report. It will allow to drop, keep or report reports.
Created attachment 287629 [details] [review] validate-report: Make the ref / unref functions safer.
Created attachment 287630 [details] [review] validate-report: Add the notion of master / shadow reports. A master report is a report that has been detected by a monitor to stem from the same issue. It thus contains a list of "shadow reports" which it will browse when printing itself.
Created attachment 287631 [details] [review] validate-reporter: Add some methods + gst_validate_reporter_get_reports + gst_validate_reporter_get_reports_count
Created attachment 287632 [details] [review] test-utils: add a create_and_monitor element function.
Created attachment 287633 [details] [review] validate-pad-monitor: concatenate issues. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=735665 The process is to check for a similar report in intercept_report on the pads of the upstream element, set that report as the master report of the intercepted report, and return REPORTER_KEEP instead of REPORTER_REPORT.
A branch is also up at https://github.com/MathieuDuponchelle/gst-devtools/commits/issue_concatenation
Review of attachment 287626 [details] [review]: OK
Please tell me what the changes are inside the commit when you reupload a new set of commits!
Review of attachment 287629 [details] [review]: OK
Review of attachment 287630 [details] [review]: ::: validate/gst/validate/gst-validate-report.c @@ +51,3 @@ #define GST_CAT_DEFAULT gst_validate_report_debug +#define GST_VALIDATE_REPORT_SHADOW_REPORTS_LOCK(r) \ Are you sure we want to dedicated lock names? @@ +448,3 @@ if (G_UNLIKELY (g_atomic_int_dec_and_test (&report->refcount))) { g_free (report->message); + g_list_free_full (report->shadow_reports, (GDestroyNotify) gst_validate_report_unref); Missing the lock here. @@ +628,3 @@ + gboolean add_shadow_report = TRUE; + + report->master_report = gst_validate_report_ref (master_report); What happens if we already had one? You should probably g_return_if_fail(report->master_report); @@ +655,3 @@ + 12, "", gst_validate_reporter_get_name (report->reporter)); + + for (tmp = report->shadow_reports; tmp; tmp = tmp->next) { Forgot to get the lock here?
Review of attachment 287631 [details] [review]: Just change the status please....
Review of attachment 287632 [details] [review]: Already accepted, please try not to reupload patches that were already accepted.
Review of attachment 287633 [details] [review]: Great :)
Review of attachment 287627 [details] [review]: Already accepted...
Review of attachment 287628 [details] [review]: Already accepted...
Created attachment 287714 [details] [review] Removes a cycling reference
Review of attachment 287626 [details] [review]: commited
Review of attachment 287627 [details] [review]: commited
Review of attachment 287628 [details] [review]: commited
Review of attachment 287629 [details] [review]: commited
Review of attachment 287631 [details] [review]: commited
Review of attachment 287632 [details] [review]: commited
Review of attachment 287633 [details] [review]: commited
Review of attachment 287714 [details] [review]: commited
Thanks for the review, closing.