From 0127706e09ab6e4d4573e1663ac5c0d495aa574f Mon Sep 17 00:00:00 2001 From: Thibault Saunier Date: Thu, 18 Jul 2013 17:49:44 -0400 Subject: [PATCH] qa-report: Avoid reporting tons of times the exact same issue to users Some of the issue can be reported once and for all. We are here avoiding to flood the user with the same information repeated infinitely. --- validate/gst/qa/gst-qa-monitor.c | 36 +++++++++++++++++++---- validate/gst/qa/gst-qa-monitor.h | 43 ++++++++++++++-------------- validate/gst/qa/gst-qa-pad-monitor.c | 39 +++++++++++++------------ validate/gst/qa/gst-qa-report.c | 4 ++- validate/gst/qa/gst-qa-report.h | 5 +++- 5 files changed, 80 insertions(+), 47 deletions(-) diff --git a/validate/gst/qa/gst-qa-monitor.c b/validate/gst/qa/gst-qa-monitor.c index 3992969849..a7f4567e84 100644 --- a/validate/gst/qa/gst-qa-monitor.c +++ b/validate/gst/qa/gst-qa-monitor.c @@ -76,6 +76,8 @@ gst_qa_monitor_dispose (GObject * object) g_object_weak_unref (G_OBJECT (monitor->target), (GWeakNotify) _target_freed_cb, monitor); + g_hash_table_unref (monitor->reports); + G_OBJECT_CLASS (parent_class)->dispose (object); } @@ -131,10 +133,20 @@ gst_qa_monitor_constructor (GType type, guint n_construct_params, return (GObject *) monitor; } +static inline gchar * +_qa_report_id (GstQaReport * report) +{ + return g_strdup_printf ("%i-%i-%i-%s", + report->level, report->area, report->subarea, report->id); +} + static void gst_qa_monitor_init (GstQaMonitor * monitor) { g_mutex_init (&monitor->mutex); + + monitor->reports = g_hash_table_new_full (g_str_hash, g_str_equal, + g_free, (GDestroyNotify) gst_qa_report_unref); } /** @@ -228,15 +240,27 @@ gst_qa_monitor_get_property (GObject * object, guint prop_id, } void -gst_qa_monitor_do_report_valist (GstQaMonitor * monitor, +gst_qa_monitor_do_report_valist (GstQaMonitor * monitor, gboolean repeat, GstQaReportLevel level, GstQaReportArea area, gint subarea, const gchar * format, va_list var_args) { - gchar *message; GstQaReport *report; + gchar *message, *report_id = NULL; message = g_strdup_vprintf (format, var_args); - report = gst_qa_report_new (monitor, level, area, subarea, message); + report = gst_qa_report_new (monitor, level, area, subarea, format, message); + + if (repeat == FALSE) { + report_id = _qa_report_id (report); + + if (g_hash_table_lookup (monitor->reports, report_id)) { + GST_DEBUG ("Report %s already present", report_id); + g_free (report_id); + return; + } + + g_hash_table_insert (monitor->reports, report_id, report); + } GST_INFO_OBJECT (monitor, "Received error report %d : %d : %d : %s", level, area, subarea, message); @@ -251,15 +275,15 @@ gst_qa_monitor_do_report_valist (GstQaMonitor * monitor, } void -gst_qa_monitor_do_report (GstQaMonitor * monitor, +gst_qa_monitor_do_report (GstQaMonitor * monitor, gboolean repeat, GstQaReportLevel level, GstQaReportArea area, gint subarea, const gchar * format, ...) { va_list var_args; va_start (var_args, format); - gst_qa_monitor_do_report_valist (monitor, level, area, subarea, format, - var_args); + gst_qa_monitor_do_report_valist (monitor, repeat, level, area, subarea, + format, var_args); va_end (var_args); } diff --git a/validate/gst/qa/gst-qa-monitor.h b/validate/gst/qa/gst-qa-monitor.h index 94f92a0879..82c06f9399 100644 --- a/validate/gst/qa/gst-qa-monitor.h +++ b/validate/gst/qa/gst-qa-monitor.h @@ -45,61 +45,61 @@ G_BEGIN_DECLS #define GST_QA_MONITOR_UNLOCK(m) (g_mutex_unlock (&GST_QA_MONITOR_CAST(m)->mutex)) #ifdef G_HAVE_ISO_VARARGS -#define GST_QA_MONITOR_REPORT(m, status, area, subarea, ...) \ +#define GST_QA_MONITOR_REPORT(m, repeat, status, area, subarea, ...) \ G_STMT_START { \ - gst_qa_monitor_do_report (GST_QA_MONITOR (m), \ + gst_qa_monitor_do_report (GST_QA_MONITOR (m), repeat, \ GST_QA_REPORT_LEVEL_ ## status, GST_QA_AREA_ ## area, \ GST_QA_AREA_ ## area ## _ ## subarea, __VA_ARGS__ ); \ } G_STMT_END -#define GST_QA_MONITOR_REPORT_CRITICAL(m, area, subarea, ...) \ +#define GST_QA_MONITOR_REPORT_CRITICAL(m, repeat, area, subarea, ...) \ G_STMT_START { \ GST_ERROR_OBJECT (m, "Critical report: %s: %s: %s", \ #area, #subarea, __VA_ARGS__); \ - GST_QA_MONITOR_REPORT(m, CRITICAL, area, subarea, __VA_ARGS__); \ + GST_QA_MONITOR_REPORT(m, repeat, CRITICAL, area, subarea, __VA_ARGS__); \ } G_STMT_END -#define GST_QA_MONITOR_REPORT_WARNING(m, area, subarea, ...) \ +#define GST_QA_MONITOR_REPORT_WARNING(m, repeat, area, subarea, ...) \ G_STMT_START { \ GST_WARNING_OBJECT (m, "Warning report: %s: %s: %s", \ #area, #subarea, __VA_ARGS__); \ - GST_QA_MONITOR_REPORT(m, WARNING, area, subarea, __VA_ARGS__); \ + GST_QA_MONITOR_REPORT(m, repeat, WARNING, area, subarea, __VA_ARGS__); \ } G_STMT_END -#define GST_QA_MONITOR_REPORT_ISSUE(m, area, subarea, ...) \ +#define GST_QA_MONITOR_REPORT_ISSUE(m, repeat, area, subarea, ...) \ G_STMT_START { \ GST_WARNING_OBJECT (m, "Issue report: %s: %s: %s", \ #area, #subarea, __VA_ARGS__); \ - GST_QA_MONITOR_REPORT(m, ISSUE, area, subarea, __VA_ARGS__); \ + GST_QA_MONITOR_REPORT(m, repeat, ISSUE, area, subarea, __VA_ARGS__); \ } G_STMT_END #else /* G_HAVE_GNUC_VARARGS */ #ifdef G_HAVE_GNUC_VARARGS -#define GST_QA_MONITOR_REPORT(m, status, area, subarea, args...) \ +#define GST_QA_MONITOR_REPORT(m, repeat, status, area, subarea, args...) \ G_STMT_START { \ gst_qa_monitor_do_report (GST_QA_MONITOR (m), \ GST_QA_REPORT_LEVEL_ ## status, GST_QA_AREA_ ## area, \ - GST_QA_AREA_ ## area ## _ ## subarea, ##args ); \ + GST_QA_AREA_ ## area ## _ ## subarea, ##args ); \ } G_STMT_END -#define GST_QA_MONITOR_REPORT_CRITICAL(m, area, subarea, args...) \ +#define GST_QA_MONITOR_REPORT_CRITICAL(m, repeat, area, subarea, args...) \ G_STMT_START { \ GST_ERROR_OBJECT (m, "Critical report: %s: %s: %s", \ - #area, #subarea, ##args); \ - GST_QA_MONITOR_REPORT(m, CRITICAL, area, subarea, ##args); \ + #area, #subarea, ##args); \ + GST_QA_MONITOR_REPORT(m, repeat, CRITICAL, area, subarea, ##args); \ } G_STMT_END -#define GST_QA_MONITOR_REPORT_WARNING(m, area, subarea, args...) \ +#define GST_QA_MONITOR_REPORT_WARNING(m, repeat, area, subarea, args...) \ G_STMT_START { \ GST_WARNING_OBJECT (m, "Warning report: %s: %s: %s", \ - #area, #subarea, ##args); \ - GST_QA_MONITOR_REPORT(m, WARNING, area, subarea, ##args); \ + #area, #subarea, ##args); \ + GST_QA_MONITOR_REPORT(m, repeat, WARNING, area, subarea, ##args); \ } G_STMT_END -#define GST_QA_MONITOR_REPORT_ISSUE(m, area, subarea, args...) \ +#define GST_QA_MONITOR_REPORT_ISSUE(m, repeat, area, subarea, args...) \ G_STMT_START { \ GST_WARNING_OBJECT (m, "Issue report: %s: %s: %s", \ - #area, #subarea, ##args); \ - GST_QA_MONITOR_REPORT(m, ISSUE, area, subarea, ##args); \ + #area, #subarea, ##args); \ + GST_QA_MONITOR_REPORT(m, repeat, ISSUE, area, subarea, ##args); \ } G_STMT_END #endif /* G_HAVE_ISO_VARARGS */ #endif /* G_HAVE_GNUC_VARARGS */ @@ -132,6 +132,7 @@ struct _GstQaMonitor { GstQaRunner *runner; /*< private >*/ + GHashTable *reports; }; /** @@ -149,11 +150,11 @@ struct _GstQaMonitorClass { /* normal GObject stuff */ GType gst_qa_monitor_get_type (void); -void gst_qa_monitor_do_report (GstQaMonitor * monitor, +void gst_qa_monitor_do_report (GstQaMonitor * monitor, gboolean repeat, GstQaReportLevel level, GstQaReportArea area, gint subarea, const gchar * format, ...); -void gst_qa_monitor_do_report_valist (GstQaMonitor * monitor, +void gst_qa_monitor_do_report_valist (GstQaMonitor * monitor, gboolean repeat, GstQaReportLevel level, GstQaReportArea area, gint subarea, const gchar *format, va_list var_args); diff --git a/validate/gst/qa/gst-qa-pad-monitor.c b/validate/gst/qa/gst-qa-pad-monitor.c index 3e7299a17b..0d64a60c0d 100644 --- a/validate/gst/qa/gst-qa-pad-monitor.c +++ b/validate/gst/qa/gst-qa-pad-monitor.c @@ -88,14 +88,16 @@ _structure_is_raw_audio (GstStructure * structure) || gst_structure_has_name (structure, "audio/x-raw-float"); } + + #define CHECK_FIELD_TYPE(m,structure,field,type,multtype) \ G_STMT_START { \ if (!gst_structure_has_field (structure, field)) { \ - GST_QA_MONITOR_REPORT_WARNING (monitor, CAPS_NEGOTIATION, MISSING_FIELD, \ + GST_QA_MONITOR_REPORT_WARNING (monitor, FALSE, CAPS_NEGOTIATION, MISSING_FIELD, \ #field " is missing from structure: %" GST_PTR_FORMAT, structure); \ } else if (!gst_structure_has_field_typed (structure, field, type) && \ !gst_structure_has_field_typed (structure, field, multtype)) { \ - GST_QA_MONITOR_REPORT_CRITICAL (monitor, CAPS_NEGOTIATION, BAD_FIELD_TYPE, \ + GST_QA_MONITOR_REPORT_CRITICAL (monitor, FALSE, CAPS_NEGOTIATION, BAD_FIELD_TYPE, \ #field " has wrong type %s in structure '%" GST_PTR_FORMAT \ "'. Expected: %s or %s", \ g_type_name (gst_structure_get_field_type (structure, field)), \ @@ -311,7 +313,7 @@ gst_qa_pad_monitor_check_buffer_timestamp_in_received_range (GstQaPadMonitor * "internal linked pad was found"); } if (!found) { - GST_QA_MONITOR_REPORT_WARNING (monitor, BUFFER, TIMESTAMP, + GST_QA_MONITOR_REPORT_WARNING (monitor, FALSE, BUFFER, TIMESTAMP, "Timestamp is out of range of received input"); } } @@ -324,14 +326,14 @@ gst_qa_pad_monitor_check_first_buffer (GstQaPadMonitor * pad_monitor, pad_monitor->first_buffer = FALSE; if (!pad_monitor->has_segment) { - GST_QA_MONITOR_REPORT_WARNING (pad_monitor, EVENT, EXPECTED, + GST_QA_MONITOR_REPORT_WARNING (pad_monitor, FALSE, EVENT, EXPECTED, "Received buffer before Segment event"); } if (GST_CLOCK_TIME_IS_VALID (GST_BUFFER_TIMESTAMP (buffer))) { gint64 running_time = gst_segment_to_running_time (&pad_monitor->segment, pad_monitor->segment.format, GST_BUFFER_TIMESTAMP (buffer)); if (running_time != 0) { - GST_QA_MONITOR_REPORT_WARNING (pad_monitor, BUFFER, TIMESTAMP, + GST_QA_MONITOR_REPORT_WARNING (pad_monitor, FALSE, BUFFER, TIMESTAMP, "First buffer running time is not 0, it is: %" GST_TIME_FORMAT, GST_TIME_ARGS (running_time)); } @@ -436,7 +438,7 @@ gst_qa_pad_monitor_check_aggregated_return (GstQaPadMonitor * monitor, } if (aggregated != ret) { /* TODO review this error code */ - GST_QA_MONITOR_REPORT_CRITICAL (monitor, BUFFER, UNEXPECTED, + GST_QA_MONITOR_REPORT_CRITICAL (monitor, TRUE, BUFFER, UNEXPECTED, "Wrong combined flow return %s(%d). Expected: %s(%d)", gst_flow_get_name (ret), ret, gst_flow_get_name (aggregated), aggregated); @@ -459,7 +461,7 @@ gst_qa_pad_monitor_add_expected_newsegment (GstQaPadMonitor * monitor, case GST_ITERATOR_OK: othermonitor = g_object_get_data ((GObject *) otherpad, "qa-monitor"); if (othermonitor->expected_segment) { - GST_QA_MONITOR_REPORT_WARNING (othermonitor, EVENT, EXPECTED, + GST_QA_MONITOR_REPORT_WARNING (othermonitor, FALSE, EVENT, EXPECTED, "expected newsegment event never pushed"); gst_event_unref (othermonitor->expected_segment); } @@ -528,7 +530,8 @@ gst_qa_pad_monitor_sink_event_check (GstQaPadMonitor * pad_monitor, || (exp_rate * exp_applied_rate != rate * applied_rate) || exp_start != start || exp_stop != stop || exp_position != position) { - GST_QA_MONITOR_REPORT_WARNING (pad_monitor, EVENT, EXPECTED, + GST_QA_MONITOR_REPORT_WARNING (pad_monitor, TRUE, EVENT, + EXPECTED, "Expected segment didn't match received segment event"); } } @@ -543,7 +546,7 @@ gst_qa_pad_monitor_sink_event_check (GstQaPadMonitor * pad_monitor, if (seqnum == pad_monitor->pending_flush_start_seqnum) { pad_monitor->pending_flush_start_seqnum = 0; } else { - GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, EVENT, SEQNUM, + GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, TRUE, EVENT, SEQNUM, "The expected flush-start seqnum should be the same as the " "one from the event that caused it (probably a seek). Got: %u." " Expected: %u", seqnum, pad_monitor->pending_flush_start_seqnum); @@ -551,7 +554,7 @@ gst_qa_pad_monitor_sink_event_check (GstQaPadMonitor * pad_monitor, } if (pad_monitor->pending_flush_stop) { - GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, EVENT, UNEXPECTED, + GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, TRUE, EVENT, UNEXPECTED, "Received flush-start when flush-stop was expected"); } } @@ -562,7 +565,7 @@ gst_qa_pad_monitor_sink_event_check (GstQaPadMonitor * pad_monitor, if (seqnum == pad_monitor->pending_flush_stop_seqnum) { pad_monitor->pending_flush_stop_seqnum = 0; } else { - GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, EVENT, SEQNUM, + GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, TRUE, EVENT, SEQNUM, "The expected flush-stop seqnum should be the same as the " "one from the event that caused it (probably a seek). Got: %u." " Expected: %u", seqnum, pad_monitor->pending_flush_stop_seqnum); @@ -570,7 +573,7 @@ gst_qa_pad_monitor_sink_event_check (GstQaPadMonitor * pad_monitor, } if (!pad_monitor->pending_flush_stop) { - GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, EVENT, UNEXPECTED, + GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, TRUE, EVENT, UNEXPECTED, "Unexpected flush-stop"); } } @@ -658,18 +661,18 @@ gst_qa_pad_monitor_src_event_check (GstQaPadMonitor * pad_monitor, if (seqnum == pad_monitor->pending_flush_start_seqnum) { pad_monitor->pending_flush_start_seqnum = 0; } else { - GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, EVENT, SEQNUM, + GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, TRUE, EVENT, SEQNUM, "The expected flush-start seqnum should be the same as the " "one from the event that caused it (probably a seek). Got: %u." " Expected: %u", seqnum, pad_monitor->pending_flush_start_seqnum); } } else { - GST_QA_MONITOR_REPORT_CRITICAL (pad_monitor, EVENT, UNEXPECTED, + GST_QA_MONITOR_REPORT_CRITICAL (pad_monitor, TRUE, EVENT, UNEXPECTED, "Received unexpected flush-start"); } if (pad_monitor->pending_flush_stop) { - GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, EVENT, UNEXPECTED, + GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, TRUE, EVENT, UNEXPECTED, "Received flush-start when flush-stop was expected"); } } @@ -680,7 +683,7 @@ gst_qa_pad_monitor_src_event_check (GstQaPadMonitor * pad_monitor, if (seqnum == pad_monitor->pending_flush_stop_seqnum) { pad_monitor->pending_flush_stop_seqnum = 0; } else { - GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, EVENT, SEQNUM, + GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, TRUE, EVENT, SEQNUM, "The expected flush-stop seqnum should be the same as the " "one from the event that caused it (probably a seek). Got: %u." " Expected: %u", seqnum, pad_monitor->pending_flush_stop_seqnum); @@ -688,7 +691,7 @@ gst_qa_pad_monitor_src_event_check (GstQaPadMonitor * pad_monitor, } if (!pad_monitor->pending_flush_stop) { - GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, EVENT, UNEXPECTED, + GST_QA_MONITOR_REPORT_ISSUE (pad_monitor, TRUE, EVENT, UNEXPECTED, "Unexpected flush-stop"); } } @@ -826,7 +829,7 @@ gst_qa_pad_monitor_buffer_probe (GstPad * pad, GstBuffer * buffer, GST_BUFFER_TIMESTAMP (buffer), GST_BUFFER_TIMESTAMP (buffer) + GST_BUFFER_DURATION (buffer), NULL, NULL)) { /* TODO is this a timestamp issue? */ - GST_QA_MONITOR_REPORT_ISSUE (monitor, BUFFER, TIMESTAMP, + GST_QA_MONITOR_REPORT_ISSUE (monitor, FALSE, BUFFER, TIMESTAMP, "buffer is out of segment and shouldn't be pushed. Timestamp: %" GST_TIME_FORMAT " - duration: %" GST_TIME_FORMAT ". Range: %" GST_TIME_FORMAT " - %" GST_TIME_FORMAT, diff --git a/validate/gst/qa/gst-qa-report.c b/validate/gst/qa/gst-qa-report.c index 998957fcfd..55702a4554 100644 --- a/validate/gst/qa/gst-qa-report.c +++ b/validate/gst/qa/gst-qa-report.c @@ -177,7 +177,7 @@ gst_qa_report_check_abort (GstQaReport * report) GstQaReport * gst_qa_report_new (GstQaMonitor * monitor, GstQaReportLevel level, - GstQaReportArea area, gint subarea, const gchar * message) + GstQaReportArea area, gint subarea, const gchar * id, const gchar * message) { GstQaReport *report = g_slice_new0 (GstQaReport); @@ -186,6 +186,7 @@ gst_qa_report_new (GstQaMonitor * monitor, GstQaReportLevel level, report->subarea = subarea; report->source_name = g_strdup (monitor->target_name); report->message = g_strdup (message); + report->id = g_strdup (id); report->timestamp = gst_util_get_timestamp () - _gst_qa_report_start_time; /* we might abort here if asked */ @@ -199,6 +200,7 @@ gst_qa_report_unref (GstQaReport * report) { if (G_UNLIKELY (g_atomic_int_dec_and_test (&report->refcount))) { g_free (report->message); + g_free (report->id); g_free (report->source_name); g_slice_free (GstQaReport, report); } diff --git a/validate/gst/qa/gst-qa-report.h b/validate/gst/qa/gst-qa-report.h index 14689b4f81..5f012c80be 100644 --- a/validate/gst/qa/gst-qa-report.h +++ b/validate/gst/qa/gst-qa-report.h @@ -92,6 +92,7 @@ typedef struct { GstQaReportArea area; gint subarea; gchar *message; + gchar *id; gchar *source_name; guint64 timestamp; @@ -108,7 +109,9 @@ typedef struct { void gst_qa_report_init (void); GstQaReport * gst_qa_report_new (GstQaMonitor * monitor, GstQaReportLevel level, GstQaReportArea area, - gint subarea, const gchar * message); + gint subarea, + const gchar *format, + const gchar * message); void gst_qa_report_unref (GstQaReport * report); GstQaReport * gst_qa_report_ref (GstQaReport * report);