pad-monitor: Check that an ERROR GstMessage has been posted on GST_FLOW_ERROR

Summary:
Before returning GST_FLOW_ERROR, an element must post an ERROR GstMessage,
enforce that.

Reviewers: thiblahute, Mathieu_Du

Differential Revision: http://phabricator.freedesktop.org/D201

validate/gst/validate/gst-validate-pad-monitor.c

@@ -28,6 +28,7 @@
 #include "gst-validate-internal.h"
 #include "gst-validate-pad-monitor.h"
 #include "gst-validate-element-monitor.h"
+#include "gst-validate-pipeline-monitor.h"
 #include "gst-validate-reporter.h"
 #include <string.h>
 #include <stdarg.h>
@@ -1985,6 +1986,34 @@ gst_validate_pad_monitor_check_right_buffer (GstValidatePadMonitor *
   return ret;
 }
 
+static void
+gst_validate_pad_monitor_check_return (GstValidatePadMonitor * pad_monitor,
+    GstFlowReturn ret)
+{
+  GstValidateMonitor *parent = GST_VALIDATE_MONITOR (pad_monitor);
+
+  if (ret != GST_FLOW_ERROR)
+    return;
+
+  while (GST_VALIDATE_MONITOR_GET_PARENT (parent))
+    parent = GST_VALIDATE_MONITOR_GET_PARENT (parent);
+
+  if (GST_IS_VALIDATE_PIPELINE_MONITOR (parent)) {
+    GstValidatePipelineMonitor *m = GST_VALIDATE_PIPELINE_MONITOR (parent);
+
+    GST_VALIDATE_MONITOR_LOCK (m);
+    if (m->got_error == FALSE) {
+      GST_VALIDATE_REPORT (pad_monitor, FLOW_ERROR_WITHOUT_ERROR_MESSAGE,
+          "Pad return GST_FLOW_ERROR but no GST_MESSAGE_ERROR was received on"
+          " the bus");
+
+      /* Only report it the first time */
+      m->got_error = TRUE;
+    }
+    GST_VALIDATE_MONITOR_UNLOCK (m);
+  }
+}
+
 static GstFlowReturn
 gst_validate_pad_monitor_chain_func (GstPad * pad, GstObject * parent,
     GstBuffer * buffer)
@@ -2008,6 +2037,8 @@ gst_validate_pad_monitor_chain_func (GstPad * pad, GstObject * parent,
 
   ret = pad_monitor->chain_func (pad, parent, buffer);
 
+  gst_validate_pad_monitor_check_return (pad_monitor, ret);
+
   GST_VALIDATE_PAD_MONITOR_PARENT_LOCK (pad_monitor);
   GST_VALIDATE_MONITOR_LOCK (pad_monitor);
 

validate/gst/validate/gst-validate-pipeline-monitor.c

@@ -113,6 +113,9 @@ _bus_handler (GstBus * bus, GstMessage * message,
         GST_VALIDATE_REPORT (monitor, ERROR_ON_BUS,
             "Got error: %s -- Debug message: %s", err->message, debug);
       }
+      GST_VALIDATE_MONITOR_LOCK (monitor);
+      monitor->got_error = TRUE;
+      GST_VALIDATE_MONITOR_UNLOCK (monitor);
       g_error_free (err);
       g_free (debug);
       break;
@@ -139,7 +142,7 @@ _bus_handler (GstBus * bus, GstMessage * message,
           if (monitor->print_pos_srcid
               && g_source_remove (monitor->print_pos_srcid))
             monitor->print_pos_srcid = 0;
-
+          monitor->got_error = FALSE;
         }
       }
 

validate/gst/validate/gst-validate-pipeline-monitor.h

@@ -57,6 +57,7 @@ struct _GstValidatePipelineMonitor {
   gulong element_added_id;
   guint print_pos_srcid;
   gboolean buffering;
+  gboolean got_error;
 };
 
 /**

validate/gst/validate/gst-validate-report.c

@@ -195,6 +195,10 @@ gst_validate_report_load_issues (void)
   REGISTER_VALIDATE_ISSUE (ISSUE, BUFFER_AFTER_EOS,
       _("buffer was received after EOS"),
       _("a pad shouldn't receive any more buffers after it gets EOS"));
+  REGISTER_VALIDATE_ISSUE (WARNING, FLOW_ERROR_WITHOUT_ERROR_MESSAGE,
+      _("GST_FLOW_ERROR returned without posting an ERROR on the bus"),
+      _("Element MUST post a GST_MESSAGE_ERROR with GST_ELEMENT_ERROR before"
+          " returning GST_FLOW_ERROR"));
 
   REGISTER_VALIDATE_ISSUE (ISSUE, CAPS_IS_MISSING_FIELD,
       _("caps is missing a required field for its type"),

validate/gst/validate/gst-validate-report.h

@@ -63,6 +63,7 @@ typedef enum {
 #define WRONG_FLOW_RETURN                        _QUARK("buffer::wrong-flow-return")
 #define BUFFER_AFTER_EOS                         _QUARK("buffer::after-eos")
 #define WRONG_BUFFER                             _QUARK("buffer::not-expected-one")
+#define FLOW_ERROR_WITHOUT_ERROR_MESSAGE         _QUARK("buffer::flow-error-without-error-message")
 
 #define CAPS_IS_MISSING_FIELD                    _QUARK("caps::is-missing-field")
 #define CAPS_FIELD_HAS_BAD_TYPE                  _QUARK("caps::field-has-bad-type")

validate/tests/check/validate/padmonitor.c

@@ -17,6 +17,7 @@
  * Boston, MA 02110-1301, USA.
  */
 
+#include <gio/gio.h>
 #include <gst/validate/validate.h>
 #include <gst/validate/gst-validate-pad-monitor.h>
 #include <gst/validate/media-descriptor-parser.h>
@@ -975,6 +976,99 @@ GST_START_TEST (buffer_timestamp_out_of_received_range)
 
 GST_END_TEST;
 
+
+GST_START_TEST (flow_error_without_message)
+{
+  GstElement *decoder = fake_decoder_new ();
+  GstElement *src = gst_element_factory_make ("fakesrc", NULL);
+  GstElement *sink = gst_element_factory_make ("fakesink", NULL);
+  GstBin *pipeline = GST_BIN (gst_pipeline_new ("validate-pipeline"));
+  GList *reports;
+  GstValidateRunner *runner;
+  GstValidateReport *report;
+
+  fail_unless (g_setenv ("GST_VALIDATE_REPORTING_DETAILS", "all", TRUE));
+  runner = _start_monitoring_bin (pipeline);
+
+  gst_bin_add_many (pipeline, src, decoder, sink, NULL);
+  fail_unless (gst_element_link_many (src, decoder, sink, NULL));
+
+
+  FAKE_DECODER (decoder)->return_value = GST_FLOW_ERROR;
+  ASSERT_SET_STATE (GST_ELEMENT (pipeline), GST_STATE_PLAYING,
+      GST_STATE_CHANGE_ASYNC);
+
+  gst_element_get_state (GST_ELEMENT (pipeline), NULL, NULL,
+      GST_CLOCK_TIME_NONE);
+
+  reports = gst_validate_runner_get_reports (runner);
+
+  fail_unless (g_list_length (reports) >= 1);
+  report = reports->data;
+  fail_unless_equals_int (report->level, GST_VALIDATE_REPORT_LEVEL_WARNING);
+  fail_unless_equals_int (report->issue->issue_id,
+      FLOW_ERROR_WITHOUT_ERROR_MESSAGE);
+  g_list_free_full (reports, (GDestroyNotify) gst_validate_report_unref);
+
+  gst_element_set_state (GST_ELEMENT (pipeline), GST_STATE_NULL);
+
+  _stop_monitoring_bin (pipeline, runner);
+}
+
+GST_END_TEST;
+
+
+GST_START_TEST (flow_error_with_message)
+{
+  GstElement *decoder = fake_decoder_new ();
+  GstElement *src = gst_element_factory_make ("fakesrc", NULL);
+  GstElement *sink = gst_element_factory_make ("fakesink", NULL);
+  GstBin *pipeline = GST_BIN (gst_pipeline_new ("validate-pipeline"));
+  GList *reports;
+  GstValidateRunner *runner;
+  GstValidateReport *report;
+  const GError gerror =
+      { G_IO_ERROR, G_IO_ERROR_FAILED, (gchar *) "fake error" };
+
+  fail_unless (g_setenv ("GST_VALIDATE_REPORTING_DETAILS", "all", TRUE));
+  runner = _start_monitoring_bin (pipeline);
+
+  gst_bin_add_many (pipeline, src, decoder, sink, NULL);
+  fail_unless (gst_element_link_many (src, decoder, sink, NULL));
+
+  g_object_set (src, "is-live", TRUE, NULL);
+
+  FAKE_DECODER (decoder)->return_value = GST_FLOW_ERROR;
+
+  ASSERT_SET_STATE (GST_ELEMENT (pipeline), GST_STATE_PAUSED,
+      GST_STATE_CHANGE_NO_PREROLL);
+
+  gst_element_post_message (decoder,
+      gst_message_new_error (GST_OBJECT (decoder),
+          (GError *) & gerror, "This is a fake error"));
+
+  ASSERT_SET_STATE (GST_ELEMENT (pipeline), GST_STATE_PLAYING,
+      GST_STATE_CHANGE_ASYNC);
+
+  gst_element_get_state (GST_ELEMENT (pipeline), NULL, NULL,
+      GST_CLOCK_TIME_NONE);
+
+  reports = gst_validate_runner_get_reports (runner);
+
+  assert_equals_int (g_list_length (reports), 1);
+  report = reports->data;
+  fail_unless_equals_int (report->issue->issue_id, ERROR_ON_BUS);
+  g_list_free_full (reports, (GDestroyNotify) gst_validate_report_unref);
+
+  gst_element_set_state (GST_ELEMENT (pipeline), GST_STATE_NULL);
+
+  _stop_monitoring_bin (pipeline, runner);
+}
+
+GST_END_TEST;
+
+
+
 static Suite *
 gst_validate_suite (void)
 {
@@ -992,6 +1086,8 @@ gst_validate_suite (void)
   tcase_add_test (tc_chain, check_media_info);
   tcase_add_test (tc_chain, eos_without_segment);
   tcase_add_test (tc_chain, caps_events);
+  tcase_add_test (tc_chain, flow_error_without_message);
+  tcase_add_test (tc_chain, flow_error_with_message);
 
   gst_validate_deinit ();
   return s;