GNOME Bugzilla – Bug 756806
message: Add extra information fields to error/warning/info messages
Last modified: 2017-07-14 15:27:29 UTC
See https://github.com/sdroege/gst-player/issues/93 and https://git.gnome.org/browse/totem/tree/src/backend/bacon-video-widget.c#n3733 totem has to fall back to parsing strings from the error messages to do anything useful. We should make that better somehow. A first step would probably be to add some extra information field to these messages.
Created attachment 322843 [details] [review] new propsed API Putting that here for comments about the API before continuing.
Review of attachment 322843 [details] [review]: ::: gst/gstelement.c @@ +1904,3 @@ break; case GST_MESSAGE_INFO: + message = gst_message_new_info (GST_OBJECT_CAST (element), gerror, sent_debug); // TODO Should get the same message API for warning/info messages ::: gst/gstelement.h @@ +409,3 @@ * application will be requested to stop further media processing. */ +#define GST_ELEMENT_ERROR_DATA(el, domain, code, text, debug, args...) \ ERROR_WITH_DETAILS() maybe? @@ +417,3 @@ if (__dbg) \ GST_WARNING_OBJECT (el, "error: %s", __dbg); \ + gst_element_message_full_data (GST_ELEMENT(el), GST_MESSAGE_ERROR, \ And message_full_with_details()? ::: gst/gstmessage.c @@ +441,3 @@ +/** + * gst_message_error_set_details: + * @message: (transfer none): The message object No transfer annotations needed for the instance @@ +461,3 @@ + * gst_message_parse_error_details: + * @message: (transfer none): The message object + * Returns: (transfer none): The details structure Isn't this function transfer full as you implemented it? And the "Returns: " line should be below the docs @@ +468,3 @@ + */ +GstStructure * +gst_message_parse_error_details (GstMessage * message) Elsewhere we use an out parameter for the return values. Seems a bit inconsistent
Created attachment 322848 [details] [review] new propsed API It's best to make _parse_details to be transfer none I think, or you can parse it multiple times as it's not refcounted AFAICT. I also added some code to free it in _free, then.
Created attachment 322964 [details] [review] new propsed API Info and warning versions added.
Review of attachment 322964 [details] [review]: * Make sure symbols are added to win32/def file (make update-exports) ::: gst/gstelement.h @@ +860,3 @@ + const gchar * function, gint line, + const char *name, ...); + This should have a G_GNUC_NULL_TERMINATED; ::: gst/gstmessage.c @@ +208,3 @@ + case GST_MESSAGE_WARNING: + case GST_MESSAGE_INFO: + if (gst_structure_has_field_typed (GST_MESSAGE_STRUCTURE (message), Why do we need to special-case the freeing of the "details" structure ? Doesn't it automatically get freed when the messages structure is freed ? @@ +415,3 @@ * Returns: (transfer full): the new error message. * + * Since: 1.8 1.10. It's too late for new feature addition in 1.8 @@ +480,3 @@ + * gst_message_parse_error_details: + * @message: (transfer none): The message object + * @structure: (transfer none): A pointer to the returned details structure structure needs (out callee-allocates) annotation. It's a return value, and it will return a copy. Might want to specify user should free it after usage. Same applies to other methods ::: tests/check/gst/gstmessage.c @@ +47,3 @@ error = g_error_new (domain, 10, "test error"); fail_if (error == NULL); + d = gst_structure_new ("title", "test-field", G_TYPE_STRING, Can you make this an "additional" test (at the end of the function for example), so that we check both usages ? i.e. the error message with and without details.
Review of attachment 322964 [details] [review]: ::: gst/gstelement.h @@ +437,3 @@ + * application will be requested to stop further media processing. + */ +#define GST_ELEMENT_ERROR(el, domain, code, text, debug, ...) \ There shouldn't be the stray ', ...' at the end of this, it should be: GST_ELEMENT_ERROR(el, domain, code, text, debug)
When using this patch, I get compilation failures in -good/ext/oss4: oss4-source.c: In function ‘gst_oss4_source_open’: oss4-source.c:367:179: error: ‘GST_RESOURCE_ERROR_0x00000001’ undeclared (first use in this function) oss4-source.c:367:179: note: each undeclared identifier is reported only once for each function it appears in Makefile:720: recipe for target 'libgstoss4audio_la-oss4-source.lo' failed make: *** [libgstoss4audio_la-oss4-source.lo] Error 1 make: *** Waiting for unfinished jobs.... oss4-sink.c: In function ‘gst_oss4_sink_open’: oss4-sink.c:502:179: error: ‘GST_RESOURCE_ERROR_0x00000002’ undeclared (first use in this function) oss4-sink.c:502:179: note: each undeclared identifier is reported only once for each function it appears in Makefile:713: recipe for target 'libgstoss4audio_la-oss4-sink.lo' failed make: *** [libgstoss4audio_la-oss4-sink.lo] Error 1
That GST_RESOURCE_ERROR_0x00000002 issue is because READ_WRITE is defined in oss4 header files. In order to avoid that the GST_ELEMENT_* macros should not call the other macro, but instead be a copy (with a NULL argument).
GstStructure isn't refcounted AFAICT, it's based on GType, so it can't be destroyed automatically when the containing structure is destroyed, right ? Or is there some magic happening here that I'm not aware of ?
Created attachment 323399 [details] [review] new propsed API All done. It looks like the stuctures are always copied deep in glib, so they all need freeing. The test is Valgrind clean.
Created attachment 323403 [details] [review] new propsed API With missing 1.8/1.10 bumps.
This looks good to me in principle, we should try to get it in. - gst_message_new_info_with_details(): the 'returns' annotation has a copy'n'paste-o ("a new warning message") - the transfer annotations look a bit wrong all over the place - e.g. gst_message_new_error_with_details() says 'transfer full' for details and then calls _set_details() which is not transfer full. - gst_message_parse_error_details() has "@structure: (out callee-allocates):" - _parse_details() says "@structure: (transfer none):" in some places but is actually transfer full because gst_structure_get() returns a copy (but also see below). > #define GST_ELEMENT_ERROR_WITH_DETAILS(el, domain, code, text, debug, args...) Variadic macros are a bit tricky, there are multiple flavours and we need a backup for when not available, see gstinfo.h - I wonder if _set_details() should be _take_details() with transfer full so that we don't copy the structure again - I wonder if the detail fields should be set as a separate structure or actually directly on the existing GstMessage structure? - I wonder if _parse_details() shouldn't return a const GstStructure * that is valid as long as the message is valid, thus avoiding a structure copy when parsing it, and a free.
(In reply to Tim-Philipp Müller from comment #12) [misc annotation bugs] Fixed (though not posting because of the following) > > #define GST_ELEMENT_ERROR_WITH_DETAILS(el, domain, code, text, debug, args...) > > Variadic macros are a bit tricky, there are multiple flavours and we need a > backup for when not available, see gstinfo.h That's going to be tricky with the ## usage. One'd need a macro to paste those, unless making new identifiers that'd trample all over the global namespace. > - I wonder if the detail fields should be set as a separate structure or > actually directly on the existing GstMessage structure? There's a larger risk of namespace collision if using the same structure. No real opinion on the copy-or-not choice.
Vincent, what's the status here? Would be good to get that in for 1.10. (In reply to Tim-Philipp Müller from comment #12) > > #define GST_ELEMENT_ERROR_WITH_DETAILS(el, domain, code, text, debug, args...) > > Variadic macros are a bit tricky, there are multiple flavours and we need a > backup for when not available, see gstinfo.h Maybe instead of a variadic macro we can do it like GST_ELEMENT_ERROR()? :) GST_ELEMENT_ERROR_WITH_DETAILS(el, domain, code, text, debug, ("foo", G_TYPE_INT, 123, "bar", G_TYPE_STRING, "123", NULL)) Where GST_ELEMENT_ERROR_DETAILS() expands to a call to gst_structure_new()? Basically like the "debug" and "text" parts are currently passed to g_strdup_printf(). > - I wonder if _set_details() should be _take_details() with transfer full so > that we don't copy the structure again Yes because above :) > - I wonder if the detail fields should be set as a separate structure or > actually directly on the existing GstMessage structure? Separate structure because otherwise we could get annoying name clashes. > - I wonder if _parse_details() shouldn't return a const GstStructure * that > is valid as long as the message is valid, thus avoiding a structure copy > when parsing it, and a free. Yes!
(In reply to Sebastian Dröge (slomo) from comment #14) > Vincent, what's the status here? Would be good to get that in for 1.10. I'll have another go at it then. The main thing was the request for macro based thing IIRC, I'll see if it can be made to work.
One clarification, though: End of comment 2: > @@ +468,3 @@ > + */ > +GstStructure * > +gst_message_parse_error_details (GstMessage * message) > > Elsewhere we use an out parameter for the return values. Seems a bit inconsistent Then, end of comment 14: > > - I wonder if _parse_details() shouldn't return a const GstStructure * that > > is valid as long as the message is valid, thus avoiding a structure copy > > when parsing it, and a free. > > Yes! So I go back to the original, right (plus make it const) ?
I think making it a const out parameter would be more consistent with API elsewhere :)
Created attachment 331689 [details] [review] new propsed API Requested changes except the macro ones.
Created attachment 331692 [details] [review] new propsed API Missed update for a couple functions.
Created attachment 331694 [details] [review] new propsed API That idea of using just one compound argument makes it all very easy, and I've just realized it means we don't need the three variants tpm had mentioned \o/
Something seems wrong as I've not changed the gst_element_message_full_with_details to match, and the test still passes. I will debug and post an update.
Created attachment 331729 [details] [review] new propsed API
Created attachment 331733 [details] [review] new propsed API Make the macro actually usable.
ack/nak ?
Review of attachment 331733 [details] [review]: ::: gst/gstelement.c @@ +3482,3 @@ + +GstStructure * +gst_structure_new_details (const char *name, ...) This should probably be named a bit different. It's not a structure with details or something, but a very specific kind of structure. gst_message_details_new() or something like that maybe. ::: gst/gstelement.h @@ +401,3 @@ +GstStructure *gst_structure_new_details(const char *name, ...); +#define MAKE_DETAILS(args) gst_structure_new_details args Namespace pollution @@ +418,3 @@ + * Utility function that elements can use in case they encountered a fatal + * data processing error. The pipeline will post an error message and the + * application will be requested to stop further media processing. Since: 1.10 @@ +459,3 @@ + GST_MESSAGE_ERROR, GST_ ## domain ## _ERROR, \ + GST_ ## domain ## _ERROR_ ## code, __txt, __dbg, __FILE__, \ + GST_FUNCTION, __LINE__); \ Instead of all the duplication, can't this just map to GST_ELEMENT_ERROR_WITH_DETAILS(..., (NULL))? ::: gst/gstmessage.c @@ +451,3 @@ + * Attaches optional additional details to an error message. + * + * Since: 1.10 Where would this function be used, do you have an example usage in mind? @@ +699,3 @@ + + *structure = NULL; + v = gst_structure_get_value (GST_MESSAGE_STRUCTURE (message), "details"); You might want to add a quark for the "details" string
(In reply to Sebastian Dröge (slomo) from comment #25) > Review of attachment 331733 [details] [review] [review]: > ::: gst/gstmessage.c > @@ +451,3 @@ > + * Attaches optional additional details to an error message. > + * > + * Since: 1.10 > > Where would this function be used, do you have an example usage in mind? Apart from it being used in the main function, not really. I suppose there could be a function creating an error, then some code later attaches details if it finds anything of interest, but I have no actual real case. Are you asking for it to be made internal ?
Let's make it internal for the time being then until someone needs it :)
Created attachment 331985 [details] [review] new propsed API
Review of attachment 331985 [details] [review]: ::: gst/gstelement.c @@ +3482,3 @@ + +GstStructure * +gst_message_details_new (const char *name, ...) If you put it into gstelement.c, make it gst_element_message_details_new() :) @@ +3486,3 @@ + GstStructure *structure; + va_list varargs; + if (name == NULL) return NULL; maybe? @@ +3488,3 @@ + + va_start (varargs, name); + structure = gst_structure_new_valist ("details", name, varargs); No quark for the string? ::: gst/gstmessage.c @@ +628,3 @@ + g_value_init (&v, GST_TYPE_STRUCTURE); + g_value_take_boxed (&v, details); + gst_structure_id_take_value (GST_MESSAGE_STRUCTURE (message), details_quark, Just inline that into the constructors for now
Review of attachment 331985 [details] [review]: ::: gst/gstelement.h @@ +401,3 @@ +GstStructure *gst_message_details_new(const char *name, ...); +#define GST_MAKE_DETAILS(args) gst_message_details_new args GST_ELEMENT_MESSAGE_MAKE_DETAILS() maybe?
Created attachment 331991 [details] [review] new propsed API
Review of attachment 331991 [details] [review]: Good to go IMHO, just see below and feel free to fix or not before merging ::: gst/gstelement.c @@ +3491,3 @@ + + va_start (varargs, name); + structure = gst_structure_new_valist ("details", name, varargs); Still no quark but ok :) ::: gst/gstmessage.c @@ +512,3 @@ message = gst_message_new_custom (GST_MESSAGE_WARNING, src, structure); + if (details) { + GValue v = { 0 }; G_VALUE_INIT btw
Ah, forgot to post the comment: there is no structure API that takes a GQuark for this. There's one that takes the varargs, but none that takes a va_list. Though one could be added for the occasion, I suppose.
Created attachment 331992 [details] [review] new propsed API
I changed the G_VALUE_INIT one, and left the quark one as is. commit 1105caa805ab620f7168652fa96b7a255581a354 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Mar 2 11:22:23 2016 +0000 message: new API for additional custom data to error messages https://bugzilla.gnome.org/show_bug.cgi?id=756806
Please also add things to the sections file in gstreamer/docs/gst, thanks :)
Done (I think)
GST_ELEMENT_ERROR_WITH_DETAILS and the other macros are missing still
done
The failure from comment #7 was not resolved. This should fix it, don't know if there's an easier way. commit d052ae63d8632b78e933a8447e5f2c8aa25a6c17 Author: Tim-Philipp Müller <tim@centricular.com> Date: Sun Jul 24 01:35:41 2016 +0100 element: fix GST_ELEMENT_ERROR() error code expansion In some corner cases, the error 'code' part passed to GST_ELEMENT_ERROR() is a valid define as well, in which case it won't survive two levels of macro expansion, but only one. Fixes: oss4-sink.c: In function ‘gst_oss4_sink_open’: error: ‘GST_RESOURCE_ERROR_0x00000002’ undeclared (first use in this function) GST_ ## domain ## _ERROR_ ## code, __txt, __dbg, __FILE__, which is from GST_ELEMENT_ERROR(el,RESOURCE,OPEN_WRITE,..) and OPEN_WRITE happens to be defined to 2 here. https://bugzilla.gnome.org/show_bug.cgi?id=756806 https://bugzilla.gnome.org/show_bug.cgi?id=769117
The exported function gst_element_message_details_new isn't documented and not declared in win32/common/libgstreamer.def gstelement.h:402: Warning: Gst: Constructor return type mismatch symbol='gst_element_message_details_new' constructed='Gst.Element' return='Gst.Structure'
(In reply to Josep Torra Valles from comment #41) > The exported function gst_element_message_details_new isn't documented and > not declared in win32/common/libgstreamer.def It seems to be there here > gstelement.h:402: Warning: Gst: Constructor return type mismatch > symbol='gst_element_message_details_new' constructed='Gst.Element' > return='Gst.Structure' Not sure how to fix that, everything that contains "new" in the name is considered a constructor. And there is only an annotation to mark things as method (this is none because the first argument is not an instance) or constructor.
Whee. I can just rename it to _create or something ?
Maybe :)
*** Bug 744093 has been marked as a duplicate of this bug. ***