GNOME Bugzilla – Bug 757602
adaptivedemux: improved error message if availabilityStartTime is missing for a live stream
Last modified: 2016-01-18 18:27:32 UTC
For a live mpd, if availabilityStartTime is missing, adaptive demux asserts with: Unexpected critical/warning: gst_date_time_to_g_date_time: assertion 'datetime != NULL' failed. This patch improves the error message to: Unexpected critical/warning: gst_mpd_client_seek_to_time: assertion 'client->mpd_node->availabilityStartTime != NULL' failed
Created attachment 314848 [details] [review] proposed patch
Seems reasonable. I'd just like to point out that these are not "error messages" really, but indicate a programming error in the calling code somewhere, and in other circumstances (if compiled with the certain CFLAGS) it might just crash instead. Calling code must not rely on a g_return_if*() being there, as it might be compiled out. So I guess the question is if anything else needs fixing up here, or if that's been done already. If availabilityStartTime being NULL is a legitimate condition, then it needs to be checked outside of a g_return_if_fail*().
The standard mandates that availabilityStartTime must be present for @type='dynamic'. I know that they indicate programming errors. The patch just improves the message the developer gets.
Ok, so basically good to go as is?
The availabilityStartTime is read from external input, so it is not a programming error to have it missing. On this function, it might make sense to have this assertion is somewhere before in the flow it would check if the value is NULL or not and abort earlier. If all assertions were disabled it would end up still failing ungracefully without any notice to the user. If availability start time is missing it should warn the user, not the developer. The problem here is with the input.
(In reply to Thiago Sousa Santos from comment #5) > On this function, it might make sense to have this assertion is somewhere s/is/if
Right, hence my question if that's done already - if it's a required parameter in this case then other code should post an error and bail out before this is called.
(In reply to Tim-Philipp Müller from comment #7) > Right, hence my question if that's done already - if it's a required > parameter in this case then other code should post an error and bail out > before this is called. I don't think it is done atm. So we need an additional patch to prevent reaching this new assertion added by this patch.
Created attachment 318508 [details] [review] patch to check that availabilityStartTime is present This patch checks that availabilityStartTime is present for live streams and prints an error message and returns error in case it is not. The following error messages are logged: <dashdemux>MPD does not have availabilityStartTime <dashdemux>error: Invalid manifest.
commit 88f509a710f2b4acc8c32650b5f0bd14d6def699 Author: Florin Apostol <florin.apostol@oregan.net> Date: Fri Jan 8 11:04:13 2016 +0000 dashdemux: added check that availabilityStartTime is present for live streams https://bugzilla.gnome.org/show_bug.cgi?id=757602 commit 2918dff2e0b8bf4566d136e7abb581a8f495d73d Author: Florin Apostol <florin.apostol@oregan.net> Date: Wed Nov 4 18:15:24 2015 +0000 adaptivedemux: improved error message if availabilityStartTime is missing for a live stream For a live mpd, if availabilityStartTime is missing, adaptive demux asserts with: Unexpected critical/warning: gst_date_time_to_g_date_time: assertion 'datetime != NULL' failed. This patch improves the error message to: Unexpected critical/warning: gst_mpd_client_seek_to_time: assertion 'client->mpd_node->availabilityStartTime != NULL' failed https://bugzilla.gnome.org/show_bug.cgi?id=757602