Re: [gst-devel] error handling in GStreamer

[Thomas Vander Stichele <thomas apestaart org>]

> > a) pop up an error dialog with an advanced button, where "advanced"
> > would show (d) and (e), as well as source code file, line number,
> > function
> 
> I don't like that one, I must say. If it's a programming error, it
> should go as a g_warning, not be user-visible.

I've gotten a lot of "if it's a programming error".  The simple fact is
that I've never regarded errors as either "programming" or "user"
errors.  Especially for multimedia apps/libs, this is a wrong
distinction to make.  Consider:
a) a user tries to play a video file
b) GStreamer tries to detect type and play it
c) Suppose gst gets the type wrong and can't play the file.  The user
will get a popup saying "This file cannot be played/is broken/...".  The
user tries with some other app, and it plays.  What went wrong ?
d) Suppose gst gets the type right, but there is some error in the file
and it can't decode it properly.  User gets the same popup.  It plays
mostly fine in another app.  What went wrong ?

In c) it could be considered a programming error, but the code has no
way of verifying if it made a programming error.  IMO, g_warnings are
for cases where the code has figured out that something shouldn't have
happened and really is a programming mistake.  In c), the user will join
#gstreamer, say that his file can't be played, and ask what went wrong. 
Without having a good way of reproducing the bug, this won't be very
helpful to us.  If we then ask the user to click "Advanced" and paste
the message there, that will be helpful to us.
For example, it will say "avidemux: AVI header invalid" while the file
is an .ogm.  Hence, we know where to look for what went wrong.

In d), it isn't even really a gst programming error, but maybe an
underlying library.  The advanced button will say what is wrong (ie
mpeg2dec: invalid frame) and we can look at whether or not something is
wrong with the mpeg2dec version.

Both are very valid use cases for where it makes sense to let the user
provide this info to the developers; which is why we want to see this
implemented.

I know that for xine/mplayer this need isn't there, since most of the
interesting code/codecs is in the tarball and thus under direct
developer control.

However with GStreamer we deliberately wanted to move away from this
model and I think that the error signaling system should help us realize
this model.

Please feel free to argue these specific points as I feel they are
important.

> > b) have the application trap errors of a specific kind so that it can
> > decide to try plan b instead of popping up a dialog
> 
> That sounds good, as long as the filtering is done on the application
> side (ie. all the signals are still sent to the application, whatever
> happens).

Yeah, it is - app connects to signal, app chooses based on enum what to
do.

> > c) have the application use available translated strings (from core and
> > plugins) if it chooses to do so
> 
> > d) have the application be able to provide it's own error messages if it
> > thinks it needs to, based on the enum
> 
> (aren't c) and d) basically the same?)
no; in c) the library provides error messages, which handles the common
cases.  in d), the application can override these if it wishes to do
so.  For example, if it knows what it's doing for some specific purpose
that might not be what the plugin author intended, and it can choose to
provide an error message more appropriate to the situation.

> I'd rather see that in the application itself. For example, an error in
> playback will be different if we have a video playback application, or a
> webcam application (taking totem and vanity as examples).

Yep; which is why for the less common case (the webcam) the error
message could be different.

What I absolutely want to avoid is for each webcam application to
provide different error messages for the same error.  Especially in the
cases where it makes no sense to do so.

> > some things to consider here:
> > a) we can provide functions that map the very specific finegrained enums
> > to coarse enum domains (ie, all device errors could map to one class).
> 
> Things like device errors should be finegrained, is it a permission
> error, the device isn't there, the media is damaged, etc.

Possibly; I've had people tell me that "permission errors" are not
acceptable to be presented.  I would beg to differ, but I'm not going to
make this decision alone :)

> > b) we need to decide on whether or not we want plug-ins to be able to
> > provide custom translated messages.  If there is a direct one-to-one
> > mapping between enums and error strings, this is not really necessary. 
> > If the enum would be coarse-grained (ie, only apply to a domain, like
> > GST_ERROR_DEVICE), then the element needs to be able to give more
> > specific data.
> 
> I'd rather not see that. See gnome-vfs, we would just add more error
> elements as we find things that need error messages.

"that" = coarse-grained enums ? OK.  So you want direct
one-to-one-mapping and finegrained enums, just to make sure I understood
you ?

> > c) the API for elements could be something like
> >    gst_element_gerror (GstErrorType GST_ERROR_(type), gchar *message,
> > gchart *debug);
> >    since we want both strings to be printf-like if needed, this would
> > also mean that
> >    - gst_element_gerror takes ownership of the strings and frees them
> >    - a typical call would look like
> >      gst_element_gerror (GST_ERROR_DEVICE,
> >                          g_strdup_printf (_("%s not accessible",
> >                                           device),
> >                          g_strdup_printf ("no write permissions on %s",
> >                                           device));
> 
> Either that code will leak, or it will crash when I pass non-malloc'ed
> strings ;)

The point is that _gerror will unref the passed strings, it will likely
also be wrapped in a macro that does the strdup for you, and you should
never pass non-malloc'd strings :) The reason we want to do this is
because this is the only way we could come up with a way of having two
printf-like strings.
The actual call will probably end up being GST_ELEMENT_GERROR
(GST_ERROR_DEVICE, ("%s not accessible", device), ("no write permissions
on %s", device));


> I really don't like the idea of debug info being presented to the user.
Well, a) it's hidden (a button press away) and b) I think the use cases
validate the need.  What do you think ?

I'm planning on doing this next week, so appreciate feedback still.

Thomas

Dave/Dina : future TV today ! - http://davedina.apestaart.org/
<-*- thomas (dot) apestaart (dot) org -*->
I can't leave you alone
because you're so disarming
and I'm caught in the midst of you
<-*- thomas (at) apestaart (dot) org -*->
URGent, best radio on the net - 24/7 ! - http://urgent.rug.ac.be/