After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 757602 - adaptivedemux: improved error message if availabilityStartTime is missing for a live stream
adaptivedemux: improved error message if availabilityStartTime is missing for...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal minor
: 1.7.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-11-04 18:15 UTC by Florin Apostol
Modified: 2016-01-18 18:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patch (1.66 KB, patch)
2015-11-04 18:16 UTC, Florin Apostol
committed Details | Review
patch to check that availabilityStartTime is present (933 bytes, patch)
2016-01-08 15:35 UTC, Florin Apostol
committed Details | Review

Description Florin Apostol 2015-11-04 18:15:22 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
Comment 1 Florin Apostol 2015-11-04 18:16:18 UTC
Created attachment 314848 [details] [review]
proposed patch
Comment 2 Tim-Philipp Müller 2016-01-07 18:12:51 UTC
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*().
Comment 3 Florin Apostol 2016-01-07 18:20:42 UTC
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.
Comment 4 Tim-Philipp Müller 2016-01-07 18:30:01 UTC
Ok, so basically good to go as is?
Comment 5 Thiago Sousa Santos 2016-01-07 18:32:07 UTC
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.
Comment 6 Thiago Sousa Santos 2016-01-07 18:32:41 UTC
(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
Comment 7 Tim-Philipp Müller 2016-01-07 18:35:23 UTC
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.
Comment 8 Thiago Sousa Santos 2016-01-07 18:36:39 UTC
(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.
Comment 9 Florin Apostol 2016-01-08 15:35:44 UTC
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.
Comment 10 Thiago Sousa Santos 2016-01-18 18:27:10 UTC
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