GNOME Bugzilla – Bug 752714
dashdemux: index information returned by gst_mpd_client_get_next_fragment is not used
Last modified: 2018-11-03 13:37:56 UTC
The gst_mpd_client_get_next_fragment function will set the index information for each media segment. But this information is never used by the caller function gst_dash_demux_stream_update_fragment_info. The stream->fragment data is set with all other information except the index related ones. But even if the gst_dash_demux_stream_update_fragment_info function will properly update the index information on stream->fragment, the gstadaptivedemux is written such that the index is downloaded only when the header is retrieved (in the gst_adaptive_demux_stream_download_header_fragment function). I believe the index downloading code should be moved from gst_adaptive_demux_stream_download_header_fragment to gst_adaptive_demux_stream_download_fragment so that it is attempted for each media fragment. According to the standard, each media segment can have its own segment index, so gstadaptivedemux should try to get all of them, not just the first one. I don't have any real life examples that use segment index. If some could be provided, I could test this and provide a patch.
Created attachment 308392 [details] [review] patch 1/2
Created attachment 308394 [details] [review] patch 2/2 This patch improves index handling. It solves several bugs involving handling of segment index: https://bugzilla.gnome.org/show_bug.cgi?id=752714 https://bugzilla.gnome.org/show_bug.cgi?id=752718 https://bugzilla.gnome.org/show_bug.cgi?id=752721 https://bugzilla.gnome.org/show_bug.cgi?id=752727 https://bugzilla.gnome.org/show_bug.cgi?id=752770
Review of attachment 308394 [details] [review]: ::: ext/dash/gstmpdparser.c @@ +2540,3 @@ gst_mpdparser_build_URL_from_template (const gchar * url_template, + const gchar * id, guint number, guint bandwidth, guint64 time, + gboolean * used_number_or_time) What do you think of making this an input parameter instead? The user would set if it wants any index, global index or segment index. This way you enforce the caller to select what kind of index it wants rather than returning anything and expeting a post-call check. The reason for enforcing is that with time people tend to forget these small details of the code and this post-check might be forgotten when the code evolves. What do you think?
Created attachment 308528 [details] [review] updated patch 2/2 updated patch 2/2 based on received comments: gst_mpdparser_build_URL_from_template now has an input parameter
Review of attachment 308392 [details] [review]: Looks good, will push with the other once it is ready.
Review of attachment 308528 [details] [review]: ::: ext/dash/gstmpdparser.c @@ +3869,3 @@ + ignore_index = TRUE; + } + } This seems unaligned with what adaptivedemux expects. If you return the index range only when it is inside the media range then there is not really a reason to expose it as downloading the media range will already include the index. Downloading the index and then the media will make the index be downloaded twice. Is it allowed on spec to have the index outside of the media range?
In case @index attribute is missing and @indexRange is present, the spec mandates that the index is inside the media segment. I believe isoff is the only one that supports this. There is a reason for downloading the index first. In case of switching representations, the index is downloaded first in order to reposition the stream. The media is downloaded starting from the current position. So, it avoids re-downloading the elapsed media segment. And the index is not downloaded twice. The gst_dash_demux_stream_update_fragment_info function, in case of isombff, will consider the media to start after the index ends, so it does not download the index again.
What's the status here?
I have nothing to add at this moment. But it should be reviewed carefully, possible also by other people, because it introduces significant changes in the way indexes are handled and maybe I missed some scenarios.
Review of attachment 308528 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +934,3 @@ + } else { + /* header did not configure index, take values from fragment */ + stream->fragment.index_uri = fragment.index_uri; I think this might need a strdup. gst_dash_demux_stream_update_headers_info allocates for stream->fragment.index_uri, and gst_adaptive_demux_stream_fragment_clear calls g_free. Not super sure though, as there is some similar code below that does not allocate either...
Review of attachment 308528 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +934,3 @@ + } else { + /* header did not configure index, take values from fragment */ + stream->fragment.index_uri = fragment.index_uri; no, it doesn't. fragment variable is being used to get the data from gst_mpd_client_get_next_fragment function. When the fragment variable is destroyed, it does not free the contained strings. The caller of gst_mpd_client_get_next_fragment function (in this case gst_dash_demux_stream_update_fragment_info) will take ownership of the data from fragment variable.
What's the status here now?
I still have nothing to add at this moment. But it should be reviewed carefully, possible also by other people, because it introduces significant changes in the way indexes are handled and maybe I missed some scenarios.
commit 19d604e92f621401c00bef46381866dde0d5b08b Author: Florin Apostol <florin.apostol@oregan.net> Date: Mon Nov 2 11:17:29 2015 +0000 dashdemux: remove unreachable code The stream->cur_seg_template is set to the lowest available segment template (representation or adaptation or period, in this order). Because the template elements are inherited, the lowest template will have all the elements the parents had, so there is no need to check the parent for an element that is not found in the child (eg initialisation or index). https://bugzilla.gnome.org/show_bug.cgi?id=752714
Any real world test urls here? I wonder what an index for a segment mans in the end. Is it a sidx for a segment? Or is it a different kind of index?
My understanding is that segment index is a piece of information that clients can use to seek into the stream. And yes, sidx is one of them. But I believe there can be others. 6.2.4 Index Segment Index Segments contain information that is related to Media Segments and primarily contain indexing information for Media Segments. An Index Segment may provide information for one or more Media Segments. The Index Segment may be media format specific and more details shall be defined for each media format that permits Index Segments.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/276.