GNOME Bugzilla – Bug 770158
element: Add API to more easily post messages about flowing issues
Last modified: 2016-08-27 13:31:34 UTC
In many parts of the code we raise streaming error when the flow goes wrong, and each time we create more or less similare error message. Also that message does not let the application know what has actually gone wrong. In the new API we add a "flow-return" detail field inside the GstMessage so that the application has all the information if it needs it. API: GST_ELEMENT_FLOW_ERROR
Created attachment 333707 [details] [review] element: Add API to more easily emit messages about flowing issues
Created attachment 333708 [details] [review] element: Add API to more easily post messages about flowing issues In many parts of the code we raise streaming error when the flow goes wrong, and each time we create more or less similare error message. Also that message does not let the application know what has actually gone wrong. In the new API we add a "flow-return" detail field inside the GstMessage so that the application has all the information if it needs it. And make use of it everywhere it makes sense. API: GST_ELEMENT_FLOW_ERROR
Created attachment 333709 [details] [review] element: Add API to more easily post messages about flowing issues In many parts of the code we raise streaming error when the flow goes wrong, and each time we create more or less similare error message. Also that message does not let the application know what has actually gone wrong. In the new API we add a "flow-return" detail field inside the GstMessage so that the application has all the information if it needs it. And make use of it everywhere it makes sense. API: GST_ELEMENT_FLOW_ERROR
Created attachment 333710 [details] [review] Use the new API to post flow ERROR messages on the bus
Created attachment 333711 [details] [review] Use the new API to post flow ERROR messages on the bus
Created attachment 333712 [details] [review] Use the new API to post flow ERROR messages on the bus
Created attachment 333713 [details] [review] Use the new API to post flow ERROR messages on the bus
Created attachment 333714 [details] [review] Use the new API to post flow ERROR messages on the bus
Comment on attachment 333713 [details] [review] Use the new API to post flow ERROR messages on the bus >From 4320fd26545a050936d131c62b403919548fb14c Mon Sep 17 00:00:00 2001 >From: Thibault Saunier <thibault.saunier@osg.samsung.com> >Date: Fri, 19 Aug 2016 11:12:10 -0700 >Subject: [PATCH] Use the new API to post flow ERROR messages on the bus > >https://bugzilla.gnome.org/show_bug.cgi?id=770158 >--- > ext/gme/gstgme.c | 4 +--- > ext/sndfile/gstsfsink.c | 4 +--- > ext/teletextdec/gstteletextdec.c | 4 +--- > ext/timidity/gsttimidity.c | 4 +--- > ext/timidity/gstwildmidi.c | 4 +--- > gst-libs/gst/adaptivedemux/gstadaptivedemux.c | 15 ++++++++------- > gst/aiff/aiffparse.c | 4 +--- > gst/midi/midiparse.c | 4 +--- > gst/mpegdemux/gstmpegdemux.c | 4 +--- > gst/mpegtsdemux/mpegtsbase.c | 4 +--- > gst/mxf/mxfdemux.c | 4 +--- > gst/nuvdemux/gstnuvdemux.c | 4 +--- > gst/tta/gstttaparse.c | 4 +--- > sys/androidmedia/gstamcaudiodec.c | 4 +--- > sys/androidmedia/gstamcvideodec.c | 4 +--- > sys/androidmedia/gstamcvideoenc.c | 4 +--- > sys/applemedia/avfassetsrc.m | 3 +-- > 17 files changed, 24 insertions(+), 54 deletions(-) > >diff --git a/ext/gme/gstgme.c b/ext/gme/gstgme.c >index 1c5c83c..4fb4429 100644 >--- a/ext/gme/gstgme.c >+++ b/ext/gme/gstgme.c >@@ -353,9 +353,7 @@ gst_gme_play (GstPad * pad) > if (flow_return == GST_FLOW_EOS) { > gst_pad_push_event (pad, gst_event_new_eos ()); > } else if (flow_return < GST_FLOW_EOS || flow_return == GST_FLOW_NOT_LINKED) { >- GST_ELEMENT_ERROR (gme, STREAM, FAILED, ("Internal data stream error."), >- ("stream stopped, reason %s", gst_flow_get_name (flow_return))); >- >+ GST_ELEMENT_FLOW_ERROR (gme, flow_return); > gst_pad_push_event (pad, gst_event_new_eos ()); > } > } >diff --git a/ext/sndfile/gstsfsink.c b/ext/sndfile/gstsfsink.c >index 78f7098..219f1a9 100644 >--- a/ext/sndfile/gstsfsink.c >+++ b/ext/sndfile/gstsfsink.c >@@ -433,9 +433,7 @@ paused: > if (result == GST_FLOW_UNEXPECTED) { > gst_pad_send_event (pad, gst_event_new_eos ()); > } else if (result < GST_FLOW_UNEXPECTED || result == GST_FLOW_NOT_LINKED) { >- GST_ELEMENT_ERROR (basesink, STREAM, FAILED, >- (_("Internal data stream error.")), >- ("stream stopped, reason %s", gst_flow_get_name (result))); >+ GST_ELEMENT_FLOW_ERROR (basesink, result); > gst_pad_send_event (pad, gst_event_new_eos ()); > } > gst_object_unref (this); >diff --git a/ext/teletextdec/gstteletextdec.c b/ext/teletextdec/gstteletextdec.c >index 5909a76..7f8a8a6 100644 >--- a/ext/teletextdec/gstteletextdec.c >+++ b/ext/teletextdec/gstteletextdec.c >@@ -743,9 +743,7 @@ error: > { > if (ret != GST_FLOW_OK && ret != GST_FLOW_NOT_LINKED > && ret != GST_FLOW_FLUSHING) { >- GST_ELEMENT_ERROR (teletext, STREAM, FAILED, >- ("Internal data stream error."), ("stream stopped, reason %s", >- gst_flow_get_name (ret))); >+ GST_ELEMENT_FLOW_ERROR (teletext, ret); > return GST_FLOW_ERROR; > } > return ret; >diff --git a/ext/timidity/gsttimidity.c b/ext/timidity/gsttimidity.c >index f7af843..2078694 100644 >--- a/ext/timidity/gsttimidity.c >+++ b/ext/timidity/gsttimidity.c >@@ -743,9 +743,7 @@ eos: > } > error: > { >- GST_ELEMENT_ERROR (timidity, STREAM, FAILED, >- ("Internal data stream error"), >- ("Streaming stopped, reason %s", gst_flow_get_name (ret))); >+ GST_ELEMENT_FLOW_ERROR (timidity, ret); > gst_pad_push_event (timidity->srcpad, gst_event_new_eos ()); > goto paused; > } >diff --git a/ext/timidity/gstwildmidi.c b/ext/timidity/gstwildmidi.c >index 3e82ca2..ef4cc34 100644 >--- a/ext/timidity/gstwildmidi.c >+++ b/ext/timidity/gstwildmidi.c >@@ -898,9 +898,7 @@ pause: > event = gst_event_new_eos (); > /* for fatal errors we post an error message, post the error > * first so the app knows about the error first. */ >- GST_ELEMENT_ERROR (wildmidi, STREAM, FAILED, >- ("Internal data flow error."), >- ("streaming task paused, reason %s (%d)", reason, ret)); >+ GST_ELEMENT_FLOW_ERROR (wildmidi, ret); > gst_pad_push_event (wildmidi->srcpad, event); > } > } >diff --git a/gst-libs/gst/adaptivedemux/gstadaptivedemux.c b/gst-libs/gst/adaptivedemux/gstadaptivedemux.c >index ea13658..f04689a 100644 >--- a/gst-libs/gst/adaptivedemux/gstadaptivedemux.c >+++ b/gst-libs/gst/adaptivedemux/gstadaptivedemux.c >@@ -2227,8 +2227,7 @@ _src_chain (GstPad * pad, GstObject * parent, GstBuffer * buffer) > gboolean finished = FALSE; > > if (ret < GST_FLOW_EOS) { >- GST_ELEMENT_ERROR (demux, STREAM, FAILED, (NULL), >- ("stream stopped, reason %s", gst_flow_get_name (ret))); >+ GST_ELEMENT_FLOW_ERROR (demux, ret); > > /* TODO push this on all pads */ > gst_pad_push_event (stream->pad, gst_event_new_eos ()); >@@ -3282,13 +3281,15 @@ gst_adaptive_demux_stream_download_loop (GstAdaptiveDemuxStream * stream) > break; > > case GST_FLOW_NOT_LINKED: >+ { >+ GstFlowReturn ret; > gst_task_stop (stream->download_task); >- if (gst_adaptive_demux_combine_flows (demux) >- == GST_FLOW_NOT_LINKED) { >- GST_ELEMENT_ERROR (demux, STREAM, FAILED, >- (_("Internal data stream error.")), ("stream stopped, reason %s", >- gst_flow_get_name (GST_FLOW_NOT_LINKED))); >+ >+ ret = gst_adaptive_demux_combine_flows (demux); >+ if (ret == GST_FLOW_NOT_LINKED) { >+ GST_ELEMENT_FLOW_ERROR (demux, ret); > } >+ } > break; > > case GST_FLOW_FLUSHING:{ >diff --git a/gst/aiff/aiffparse.c b/gst/aiff/aiffparse.c >index 194857b..faa2647 100644 >--- a/gst/aiff/aiffparse.c >+++ b/gst/aiff/aiffparse.c >@@ -1527,9 +1527,7 @@ pause: > } else if (ret < GST_FLOW_EOS || ret == GST_FLOW_NOT_LINKED) { > /* for fatal errors we post an error message, post the error > * first so the app knows about the error first. */ >- GST_ELEMENT_ERROR (aiff, STREAM, FAILED, >- (_("Internal data flow error.")), >- ("streaming task paused, reason %s (%d)", reason, ret)); >+ GST_ELEMENT_FLOW_ERROR (aiff, ret); > gst_pad_push_event (aiff->srcpad, gst_event_new_eos ()); > } > return; >diff --git a/gst/midi/midiparse.c b/gst/midi/midiparse.c >index d52539b..d58ea3f 100644 >--- a/gst/midi/midiparse.c >+++ b/gst/midi/midiparse.c >@@ -1282,9 +1282,7 @@ pause: > event = gst_event_new_eos (); > /* for fatal errors we post an error message, post the error > * first so the app knows about the error first. */ >- GST_ELEMENT_ERROR (midiparse, STREAM, FAILED, >- ("Internal data flow error."), >- ("streaming task paused, reason %s (%d)", reason, ret)); >+ GST_ELEMENT_FLOW_ERROR (midiparse, ret); > gst_pad_push_event (midiparse->srcpad, event); > } > } >diff --git a/gst/mpegdemux/gstmpegdemux.c b/gst/mpegdemux/gstmpegdemux.c >index 68985b2..b608db8 100644 >--- a/gst/mpegdemux/gstmpegdemux.c >+++ b/gst/mpegdemux/gstmpegdemux.c >@@ -3005,9 +3005,7 @@ pause: > } > } > } else if (ret == GST_FLOW_NOT_LINKED || ret < GST_FLOW_EOS) { >- GST_ELEMENT_ERROR (demux, STREAM, FAILED, >- ("Internal data stream error."), >- ("stream stopped, reason %s", reason)); >+ GST_ELEMENT_FLOW_ERROR (demux, ret); > gst_ps_demux_send_event (demux, gst_event_new_eos ()); > } > >diff --git a/gst/mpegtsdemux/mpegtsbase.c b/gst/mpegtsdemux/mpegtsbase.c >index cf69a18..e736f9b 100644 >--- a/gst/mpegtsdemux/mpegtsbase.c >+++ b/gst/mpegtsdemux/mpegtsbase.c >@@ -1382,9 +1382,7 @@ error: > (_("Internal data stream error.")), > ("No program activated before EOS")); > } else if (ret == GST_FLOW_NOT_LINKED || ret < GST_FLOW_EOS) { >- GST_ELEMENT_ERROR (base, STREAM, FAILED, >- (_("Internal data stream error.")), >- ("stream stopped, reason %s", reason)); >+ GST_ELEMENT_FLOW_ERROR (base, ret); > GST_MPEGTS_BASE_GET_CLASS (base)->push_event (base, gst_event_new_eos ()); > } > gst_pad_pause_task (base->sinkpad); >diff --git a/gst/mxf/mxfdemux.c b/gst/mxf/mxfdemux.c >index ca09cc7..823b1b6 100644 >--- a/gst/mxf/mxfdemux.c >+++ b/gst/mxf/mxfdemux.c >@@ -3098,9 +3098,7 @@ pause: > } else if (flow == GST_FLOW_NOT_LINKED || flow < GST_FLOW_EOS) { > GstEvent *e; > >- GST_ELEMENT_ERROR (demux, STREAM, FAILED, >- ("Internal data stream error."), >- ("stream stopped, reason %s", reason)); >+ GST_ELEMENT_FLOW_ERROR (demux, flow); > e = gst_event_new_eos (); > gst_event_set_seqnum (e, demux->seqnum); > gst_mxf_demux_push_src_event (demux, e); >diff --git a/gst/nuvdemux/gstnuvdemux.c b/gst/nuvdemux/gstnuvdemux.c >index b98d35d..88968f0 100644 >--- a/gst/nuvdemux/gstnuvdemux.c >+++ b/gst/nuvdemux/gstnuvdemux.c >@@ -729,9 +729,7 @@ pause: > if (res == GST_FLOW_UNEXPECTED) { > gst_nuv_demux_send_eos (nuv); > } else if (res == GST_FLOW_NOT_LINKED || res < GST_FLOW_UNEXPECTED) { >- GST_ELEMENT_ERROR (nuv, STREAM, FAILED, >- (_("Internal data stream error.")), >- ("streaming stopped, reason %s", gst_flow_get_name (res))); >+ GST_ELEMENT_FLOW_ERROR (nuv, res); > > gst_nuv_demux_send_eos (nuv); > } >diff --git a/gst/tta/gstttaparse.c b/gst/tta/gstttaparse.c >index 5479350..485a197 100644 >--- a/gst/tta/gstttaparse.c >+++ b/gst/tta/gstttaparse.c >@@ -468,9 +468,7 @@ pause: > if (ret == GST_FLOW_UNEXPECTED) { > gst_pad_push_event (ttaparse->srcpad, gst_event_new_eos ()); > } else if (ret < GST_FLOW_UNEXPECTED || ret == GST_FLOW_NOT_LINKED) { >- GST_ELEMENT_ERROR (ttaparse, STREAM, FAILED, >- ("Internal data stream error."), >- ("streaming stopped, reason %s", gst_flow_get_name (ret))); >+ GST_ELEMENT_FLOW_ERROR (ttaparse, ret); > gst_pad_push_event (ttaparse->srcpad, gst_event_new_eos ()); > } > } >diff --git a/sys/androidmedia/gstamcaudiodec.c b/sys/androidmedia/gstamcaudiodec.c >index 9a34c37..49c8c60 100644 >--- a/sys/androidmedia/gstamcaudiodec.c >+++ b/sys/androidmedia/gstamcaudiodec.c >@@ -670,9 +670,7 @@ flow_error: > gst_event_new_eos ()); > gst_pad_pause_task (GST_AUDIO_DECODER_SRC_PAD (self)); > } else if (flow_ret < GST_FLOW_EOS) { >- GST_ELEMENT_ERROR (self, STREAM, FAILED, >- ("Internal data stream error."), ("stream stopped, reason %s", >- gst_flow_get_name (flow_ret))); >+ GST_ELEMENT_FLOW_ERROR (self, flow_ret); > gst_pad_push_event (GST_AUDIO_DECODER_SRC_PAD (self), > gst_event_new_eos ()); > gst_pad_pause_task (GST_AUDIO_DECODER_SRC_PAD (self)); >diff --git a/sys/androidmedia/gstamcvideodec.c b/sys/androidmedia/gstamcvideodec.c >index 397b585..bd772b7 100644 >--- a/sys/androidmedia/gstamcvideodec.c >+++ b/sys/androidmedia/gstamcvideodec.c >@@ -1590,9 +1590,7 @@ flow_error: > gst_event_new_eos ()); > gst_pad_pause_task (GST_VIDEO_DECODER_SRC_PAD (self)); > } else if (flow_ret < GST_FLOW_EOS) { >- GST_ELEMENT_ERROR (self, STREAM, FAILED, >- ("Internal data stream error."), ("stream stopped, reason %s", >- gst_flow_get_name (flow_ret))); >+ GST_ELEMENT_FLOW_ERROR (self, flow_ret); > gst_pad_push_event (GST_VIDEO_DECODER_SRC_PAD (self), > gst_event_new_eos ()); > gst_pad_pause_task (GST_VIDEO_DECODER_SRC_PAD (self)); >diff --git a/sys/androidmedia/gstamcvideoenc.c b/sys/androidmedia/gstamcvideoenc.c >index 136df41..1742d44 100644 >--- a/sys/androidmedia/gstamcvideoenc.c >+++ b/sys/androidmedia/gstamcvideoenc.c >@@ -1129,9 +1129,7 @@ flow_error: > gst_event_new_eos ()); > gst_pad_pause_task (GST_VIDEO_ENCODER_SRC_PAD (self)); > } else if (flow_ret == GST_FLOW_NOT_LINKED || flow_ret < GST_FLOW_EOS) { >- GST_ELEMENT_ERROR (self, STREAM, FAILED, >- ("Internal data stream error."), ("stream stopped, reason %s", >- gst_flow_get_name (flow_ret))); >+ GST_ELEMENT_FLOW_ERROR (self, flow_ret); > gst_pad_push_event (GST_VIDEO_ENCODER_SRC_PAD (self), > gst_event_new_eos ()); > gst_pad_pause_task (GST_VIDEO_ENCODER_SRC_PAD (self)); >diff --git a/sys/applemedia/avfassetsrc.m b/sys/applemedia/avfassetsrc.m >index e589cf3..1795d09 100644 >--- a/sys/applemedia/avfassetsrc.m >+++ b/sys/applemedia/avfassetsrc.m >@@ -535,8 +535,7 @@ gst_avf_asset_src_read_data (GstAVFAssetSrc *self, GstPad *pad, > } > > if (combined_ret != GST_FLOW_OK) { >- GST_ELEMENT_ERROR (self, STREAM, FAILED, ("Internal data stream error."), >- ("stream stopped reason %s", gst_flow_get_name (ret))); >+ GST_ELEMENT_FLOW_ERROR (self, ret); > } > > gst_pad_pause_task (pad); >-- >2.9.3
Review of attachment 333709 [details] [review]: Can you split API and usage in two commits? Generally looks good to me though, thanks :) ::: gst/gstelement.h @@ +417,3 @@ + GST_ELEMENT_ERROR_WITH_DETAILS (el, STREAM, FAILED, \ + ("Internal data stream error."), \ + ("streaming stopped, reason %s (%d)", gst_flow_get_name (flow_return), flow_return), \ These are usually translated, no?
There's another bug from a couple of years ago about this (but without patches) somewhere :) My main question is: how would who/what use this? Does any application really gain anything by being able to tell whether the flow error was not-negotiated or not-linked? Currently it looks a bit like doing it just because we can, I don't understand the envisioned use case yet.
*** Bug 435263 has been marked as a duplicate of this bug. ***
(In reply to Tim-Philipp Müller from comment #11) > There's another bug from a couple of years ago about this (but without > patches) somewhere :) > > My main question is: how would who/what use this? Does any application > really gain anything by being able to tell whether the flow error was > not-negotiated or not-linked? Currently it looks a bit like doing it just > because we can, I don't understand the envisioned use case yet. My use case is validate where I am currently implementing smarter reporting when we get a NOT NEGOTIATED error trying to explain the user what happened and what lead to that error, so I need a way to actually know that the error is a NNE.
Comment on attachment 333709 [details] [review] element: Add API to more easily post messages about flowing issues Attachment 333709 [details] pushed as da73b89 - element: Add API to more easily post messages about flowing issues
Attachment 333710 [details] pushed as bc6aae6 - Use the new API to post flow ERROR messages on the bus Attachment 333711 [details] pushed as bc6aae6 - Use the new API to post flow ERROR messages on the bus Attachment 333712 [details] pushed as bc6aae6 - Use the new API to post flow ERROR messages on the bus Attachment 333713 [details] pushed as bc6aae6 - Use the new API to post flow ERROR messages on the bus Attachment 333714 [details] pushed as bc6aae6 - Use the new API to post flow ERROR messages on the bus
Comment on attachment 333711 [details] [review] Use the new API to post flow ERROR messages on the bus Attachment 333711 [details] pushed as 150edef - Use the new API to post flow ERROR messages on the bus Attachment 333712 [details] pushed as 150edef - Use the new API to post flow ERROR messages on the bus Attachment 333713 [details] pushed as 150edef - Use the new API to post flow ERROR messages on the bus Attachment 333714 [details] pushed as 150edef - Use the new API to post flow ERROR messages on the bus
Comment on attachment 333712 [details] [review] Use the new API to post flow ERROR messages on the bus Attachment 333712 [details] pushed as 3e27368 - Use the new API to post flow ERROR messages on the bus Attachment 333713 [details] pushed as 3e27368 - Use the new API to post flow ERROR messages on the bus Attachment 333714 [details] pushed as 3e27368 - Use the new API to post flow ERROR messages on the bus
Comment on attachment 333713 [details] [review] Use the new API to post flow ERROR messages on the bus Attachment 333713 [details] pushed as 2fb7164 - Use the new API to post flow ERROR messages on the bus Attachment 333714 [details] pushed as 2fb7164 - Use the new API to post flow ERROR messages on the bus
Attachment 333714 [details] pushed as 80924aa - Use the new API to post flow ERROR messages on the bus