Created attachment 342109 [details] [review]
dash: Split gst_isoff_sidx_parser_add_buffer() function

To parsing sidx box itself with isobmff parser,
split gst_isoff_sidx_parser_add_buffer() into two parts.

Created attachment 342110 [details] [review]
dashdemux: Always parsing sidx for On-Demand profile

Some on-demand profile MPDs have no indexRange attribute. Spec. is saying that is optional.

http://dash.akamaized.net/dash264/TestCasesMCA/dolby/4/1/ChID_voices_71_384_448_768_ddp.mpd

Review of attachment 342110 [details] [review]:

Looks generally good, thanks

::: ext/dash/gstdashdemux.c
@@ +706,3 @@
+      stream->is_isobmff =
+          gst_structure_has_name (s, "video/quicktime") ||
+          gst_structure_has_name (s, "audio/x-m4a");

audio/x-m4a is generally isobmff, so only considering this for ondemand profile seems weird. While it's then unused for audio currently, this might change later and cause problems here

@@ +1166,2 @@
     stream->fragment.uri = fragment.uri;
+    /* If mpd does not specify indexRagne (i.e., null index_uri),

Typo

@@ +2439,3 @@
 
+  if (dashdemux->allow_trickmode_key_units ||
+      gst_mpd_client_has_isoff_ondemand_profile (dashdemux->client)) {

&& !SIDX (dashstream)->entries maybe?

Created attachment 342244 [details] [review]
dashdemux: Always parsing sidx for On-Demand profile

(In reply to Sebastian Dröge (slomo) from comment #4)
> Review of attachment 342110 [details] [review] [review]:
> 
> Looks generally good, thanks
> 
> ::: ext/dash/gstdashdemux.c
> @@ +706,3 @@
> +      stream->is_isobmff =
> +          gst_structure_has_name (s, "video/quicktime") ||
> +          gst_structure_has_name (s, "audio/x-m4a");
> 
> audio/x-m4a is generally isobmff, so only considering this for ondemand
> profile seems weird. While it's then unused for audio currently, this might
> change later and cause problems here

I modified the condition there.

> @@ +2439,3 @@
>  
> +  if (dashdemux->allow_trickmode_key_units ||
> +      gst_mpd_client_has_isoff_ondemand_profile (dashdemux->client)) {
> 
> && !SIDX (dashstream)->entries maybe?

Checking GST_ISOFF_SIDX_PARSER_FINISHED seems to be more straightforward and readable :)

Attachment 342109 [details] pushed as dfa300c - dash: Split gst_isoff_sidx_parser_add_buffer() function
Attachment 342244 [details] pushed as 124e386 - dashdemux: Always parsing sidx for On-Demand profile

(In reply to Seungha Yang from comment #3)
> Some on-demand profile MPDs have no indexRange attribute. Spec. is saying
> that is optional.

While the spec says that it's optional, it says that if the value is not given then it's "unknown" (for SegmentBase) or there is no index table (for SegmentList).

I don't see how that means that we should parse any sidx that we might find while parsing the segment and assume that it is valid for the whole media segment.


In my case I have a stream here that contains a SegmentList without index, and the SegmentList works as an index (it has the offsets of each fragment). However at the beginning of each fragment, there is a sidx that only describes the following moof. We're now parsing each of them one after another and have incomplete/unexpected information in the sidx index, which then causes infinite loops during parsing.

I'd revert these commits again unless you disagree, if anything we will have to do something more clever here than just blindly parsing any sidx that we might see somewhere.

We would need to accumulate the SIDX at least, and then make sure to set the current index to the correct value. This can probably be done based on the earliest_presentation_time.

The code right now replaces each SIDX with the new one, and just increments the current index (and then it usually points after the end of the SIDX array...)

(In reply to Sebastian Dröge (slomo) from comment #8)
> (In reply to Seungha Yang from comment #3)

If your concern is the combination of sidx + SegmentList, I have a solution for it, actually (although I've never reported it on bugzilla), there are that kinds of streams in my private test contents. Can you provide your content? If possible, I'd like to report new patch for it.

Anyway, My idea is first find parent segment by SegmentList, and then do refinement using SIDX.

Oops, my private contents are sidx + SegmentTemplate, sorry. But, it does not much differ to SegmentList anyway.

I don't think I can share my stream unfortunately, but if you can provide a patch that works for your content with SegmentTemplate then I can test it on mine and fix up anything that is missing.


(In reply to Seungha Yang from comment #10)
> Anyway, My idea is first find parent segment by SegmentList, and then do
> refinement using SIDX.

You mean first of all you go via the SegmentList/Template, and then inside a segment specified by that you would then search by SIDX? That should work, yes.

However you need to ensure that you handle the SIDX themselves correctly. In my case there is exactly one SIDX in front of each segment that contains exactly one entry (for the following segment). So you would have to accumulate all SIDX from past segments here (or always parse again when switching segments?).

Also I think there's nothing that forbids for example something like my case, but the SIDX actually contains information about the previous, next and following segment. Or about the next and all previous segments. This has to be all considered when parsing the SIDX to get a coherent view.

(In reply to Sebastian Dröge (slomo) from comment #12)
> I don't think I can share my stream unfortunately, but if you can provide a
> patch that works for your content with SegmentTemplate then I can test it on
> mine and fix up anything that is missing.
Then, can you provide only MPD to me? (or only some part of it to figure out hierarchy of xml doc.) 
 
> Also I think there's nothing that forbids for example something like my
> case, but the SIDX actually contains information about the previous, next
> and following segment. Or about the next and all previous segments. This has
> to be all considered when parsing the SIDX to get a coherent view.

I'm a little bit confused by the terminology "Segment". Although I need to study the spec more, It does not seem to make sense that a SIDX has any information about outside of the "Segment" which SIDX belongs to. But it should include only the "Subsegment" information which SIDX belongs to, IMHO. In this context, your stream makes sense because each SIDX contains "Subsegment" index of which the SIDX belongs to.

In summary, it's still questionable that whether accumulating or searching prev/next "Segment" is necessary when parsing SIDX...

(In reply to Seungha Yang from comment #13)
> (In reply to Sebastian Dröge (slomo) from comment #12)
> > I don't think I can share my stream unfortunately, but if you can provide a
> > patch that works for your content with SegmentTemplate then I can test it on
> > mine and fix up anything that is missing.
> Then, can you provide only MPD to me? (or only some part of it to figure out
> hierarchy of xml doc.) 

It's basically

<MPD>
<Period>
<AdaptationSet>
<Representation>
<BaseURL>
<SegmentList>
<Initialization />
<SegmentURL mediaRange="..." />
<SegmentURL mediaRange="..." />
...

and the same repeated for other AdaptationSets


> > Also I think there's nothing that forbids for example something like my
> > case, but the SIDX actually contains information about the previous, next
> > and following segment. Or about the next and all previous segments. This has
> > to be all considered when parsing the SIDX to get a coherent view.
> 
> I'm a little bit confused by the terminology "Segment". Although I need to
> study the spec more, It does not seem to make sense that a SIDX has any
> information about outside of the "Segment" which SIDX belongs to. But it
> should include only the "Subsegment" information which SIDX belongs to,
> IMHO. In this context, your stream makes sense because each SIDX contains
> "Subsegment" index of which the SIDX belongs to.
> 
> In summary, it's still questionable that whether accumulating or searching
> prev/next "Segment" is necessary when parsing SIDX...

It might not be *necessary* but it should be handled in a useful way without breaking :)

If I remember the MP4 spec correctly, the content of the SIDX seems to be allowed to be basically anything. And at least the case of having entries in there further away then just the next fragment exists (e.g. in your stream), and the case where it only contains the next fragment and there's a new one before each fragment (e.g. my stream).

Both seem valid cases of fragmented MP4 streams. Mine seems to require some kind of accumulation of the SIDX (so you can get a "map" of the fragments outside the current fragment), although the same information is also available in the MPD at the DASH level (each SegmentURL is a byte-range into a single URL).
It could be equally possible that e.g. at the DASH level you have one "segment", but this points to a fragmented MP4 with two fragments (moof)... and either a SIDX with both at the beginning, or a SIDX with each one entry in front of each fragment.

I don't think it is necessary to support all theoretically possible combinations at this point, but we should at least handle them in a way that does not involve crashing or further breakage than needed. 


What do you think? I think as a first step it would be useful to just handle it like following: whenever a SIDX is found, you only parse that one and forget all others you saw before. The SIDX index is then updated according to the current offset so that we're not at the beginning of the SIDX but at the SIDX entry that represents what will now follow. If there is no entry that we can correlated to the fragment that follows, we just ignore the whole SIDX (which then possibly breaks seeking but not playback otherwise).

(In reply to Sebastian Dröge (slomo) from comment #14)
Thanks for your detailed suggestion :)
Anyway, I guess your problem might be a bug of Attachment 342244 [details], I just found it. I'll update it soon.

About your suggestion, Can you confirm following is correct understanding?
Each SegmentURL shares the same url but only mediaRange is different.
> <MPD>
> <Period>
> <AdaptationSet>
> <Representation>
> <BaseURL>
> <SegmentList>
> <Initialization />
--> Let's call this "Segment1"
> <SegmentURL mediaRange="..." />  --> SIDX1 in front of moof, and only one entry for Segment1
--> Let's call this "Segment2"
> <SegmentURL mediaRange="..." />  --> SIDX2 in front of moof, and only one entry for Segment2
> ...

If my understanding is correct, I'm still confused why accumulation of SIDX might be necessary.... because SIDX1 belongs only to "Segment1"
Maybe, if your concern is the *possible* case that information of "Segment2" is informed not only by "SIDX2" but also by "SIDX1". 
If so, what we should do in this case is, ensuring/checking whether each SIDX.entries is inside of corresponding "Segment" or not. In other words, if some SIDX entries are outside of the corresponding "Segment", then just ignore it, although I'd like to think this case as *wrongly* encoded stream issue though :)

> What do you think? I think as a first step it would be useful to just handle
> it like following: whenever a SIDX is found, you only parse that one and
> forget all others you saw before. The SIDX index is then updated according
> to the current offset so that we're not at the beginning of the SIDX but at
> the SIDX entry that represents what will now follow. If there is no entry
> that we can correlated to the fragment that follows, we just ignore the
> whole SIDX (which then possibly breaks seeking but not playback otherwise).

"whole SIDX" is meaning the whole SIDX in the MPD? Isn't it enough that ignoring SIDX for the corresponding "Segment"?

(In reply to Seungha Yang from comment #15)
> (In reply to Sebastian Dröge (slomo) from comment #14)
> Thanks for your detailed suggestion :)
> Anyway, I guess your problem might be a bug of Attachment 342244 [details],
> I just found it. I'll update it soon.
> 
> About your suggestion, Can you confirm following is correct understanding?
> Each SegmentURL shares the same url but only mediaRange is different.
> > <MPD>
> > <Period>
> > <AdaptationSet>
> > <Representation>
> > <BaseURL>
> > <SegmentList>
> > <Initialization />
> --> Let's call this "Segment1"
> > <SegmentURL mediaRange="..." />  --> SIDX1 in front of moof, and only one entry for Segment1
> --> Let's call this "Segment2"
> > <SegmentURL mediaRange="..." />  --> SIDX2 in front of moof, and only one entry for Segment2
> > ...
> 
> If my understanding is correct

Your understanding is correct, yes

> I'm still confused why accumulation of SIDX
> might be necessary.... because SIDX1 belongs only to "Segment1"

For my stream yes, but there seems to be nothing that forbids something else. We need to at least check for these cases and handle them gracefully instead of e.g. crashing (currently things crash / use invalid memory on my stream already, which is not acceptable of course :) )

> Maybe, if your concern is the *possible* case that information of "Segment2"
> is informed not only by "SIDX2" but also by "SIDX1". 
> If so, what we should do in this case is, ensuring/checking whether each
> SIDX.entries is inside of corresponding "Segment" or not. In other words, if
> some SIDX entries are outside of the corresponding "Segment", then just
> ignore it, although I'd like to think this case as *wrongly* encoded stream
> issue though :)

I don't think it's invalid. But yes, I agree otherwise

> > What do you think? I think as a first step it would be useful to just handle
> > it like following: whenever a SIDX is found, you only parse that one and
> > forget all others you saw before. The SIDX index is then updated according
> > to the current offset so that we're not at the beginning of the SIDX but at
> > the SIDX entry that represents what will now follow. If there is no entry
> > that we can correlated to the fragment that follows, we just ignore the
> > whole SIDX (which then possibly breaks seeking but not playback otherwise).
> 
> "whole SIDX" is meaning the whole SIDX in the MPD? Isn't it enough that
> ignoring SIDX for the corresponding "Segment"?

The whole SIDX of that segment, so yes what you said :)

Thanks. everything becomes clear now.
I'll update a patch to fix SIDX bug first, then will report another patch for sanity checking as much as possible :)

The way how seeking is done with the SIDX, it also seems to assume to have information about *all* fragments in the SIDX. Not just the current one. So if we only ever have the current one in there (as for my stream), SIDX based seeking would not know what to do.

(In reply to Sebastian Dröge (slomo) from comment #18)
> The way how seeking is done with the SIDX, it also seems to assume to have
> information about *all* fragments in the SIDX. Not just the current one. So
> if we only ever have the current one in there (as for my stream), SIDX based
> seeking would not know what to do.
==> From my view point for SIDX, it is used to know how "a Segment" is divided into "Subsegments"
So, we can just ignore the SIDX in your case, since the SIDX only contains only *one* subsegment information.


ISOBMFF is saying that
"Each Segment Index box documents how a (sub)segment is divided into one or more  subsegments (which may themselves be further subdivided using Segment Index boxes).	
In that context, my understanding about SIDX is, the coverage of SIDX is varying depending on how the "Segment" is defined.
For example, in case of usual On-Demand profile with SegmentBase, I'd like to say there is only one "Segment" in each media stream. And, the *one* "Segment" is divided into "Subsegments" by SIDX (which has full list of "Subsegments"). And, SegmentList/SegmentTemplate cases, each list of url-defined, byte-ranged or template fragment is a "Segment". And, each "fragment" might be subdivided into "Subsegment" if there is SIDX. So, we can seek to more precise position using the SIDX.
So, the my conclusion about SIDX based seeking is,
we should 2 step seeking if SIDX is there. The first is MPD based (which defines top level "Segment" division) and the last is refinement a "Segment" using SIDX (if there). So, we can just concentrate on the current "Segment", and if any SIDX entries which pointing outside of the "Segment" can be ignored.

(In reply to Seungha Yang from comment #19)
> (In reply to Sebastian Dröge (slomo) from comment #18)
> > The way how seeking is done with the SIDX, it also seems to assume to have
> > information about *all* fragments in the SIDX. Not just the current one. So
> > if we only ever have the current one in there (as for my stream), SIDX based
> > seeking would not know what to do.
> ==> From my view point for SIDX, it is used to know how "a Segment" is
> divided into "Subsegments"
> So, we can just ignore the SIDX in your case, since the SIDX only contains
> only *one* subsegment information.

Yes, correct

> ISOBMFF is saying that
> [...]
> So, the my conclusion about SIDX based seeking is,
> we should 2 step seeking if SIDX is there. The first is MPD based (which
> defines top level "Segment" division) and the last is refinement a "Segment"
> using SIDX (if there). So, we can just concentrate on the current "Segment",
> and if any SIDX entries which pointing outside of the "Segment" can be
> ignored.

Correct, that is also my understanding but not how the code does it right now. Which is what I wanted to point out above. The code assumes that if isobmff-ondemand (i.e. sidx), then we only have to look into the sidx for seeking.

See gst_dash_demux_stream_seek() which always returns if sidx after seeking based on the sidx (even if the target position is not in the sidx and we need to go to the code below that block where segments are switched).

So that has to be changed too here

Hi Seungha, did you already make any progress on this or need to discuss anything about the implementation? Let me know :)

Created attachment 347815 [details] [review]
dashdemux: Advance subfragment only if any

SIDX based playback is not restricted to SegmentBase, but it possible
with SegmentList/SegmentTemplate. In the latter case, each fragment
has its own SIDX box and might be subdivided into subfragment.
So, demux should not assume that the end of subfragment is the end
of stream. Moreover, should try advance subfragment only if there
are remaining subfragments.

(In reply to Sebastian Dröge (slomo) from comment #21)
> Hi Seungha, did you already make any progress on this or need to discuss
> anything about the implementation? Let me know :)

Sorry for late implementation... I hope attachment 347815 [details] [review] can fix normal playback of yours. But there are still some bugs on seeking and end of fragment handling. I'm keeping working on it.

Review of attachment 347815 [details] [review]:

::: ext/dash/gstdashdemux.c
@@ +2749,3 @@
                 && (GST_ADAPTIVE_DEMUX (stream->demux)->
                     segment.flags & GST_SEGMENT_FLAG_TRICKMODE_KEY_UNITS))
+            && advance && gst_dash_demux_stream_has_next_subfragment (stream)) {

Unfortunately this does not solve anything. It doesn't advance here, then loops and tries to get 0 bytes (sidx_end_offset - dash_stream->sidx_current_offset) from the adapter forever

commit a9c34fb9e3b6cfd494decd107b744d39cd1dc791
Author: Seungha Yang <sh.yang@lge.com>
Date:   Tue Mar 7 21:56:03 2017 +0900

    dashdemux: Advance subfragment only if any exist
    
    SIDX based playback is not restricted to SegmentBase, but it possible
    with SegmentList/SegmentTemplate. In the latter case, each fragment
    has its own SIDX box and might be subdivided into subfragment.
    So, demux should not assume that the end of subfragment is the end
    of stream. Moreover, should try advance subfragment only if there
    are remaining subfragments.
    
    With additional fixes by Sebastian Dröge <sebastian@centricular.com>
    
    https://bugzilla.gnome.org/show_bug.cgi?id=776200

(In reply to Sebastian Dröge (slomo) from comment #24)
> Review of attachment 347815 [details] [review] [review]:
> 
> Unfortunately this does not solve anything. It doesn't advance here, then
> loops and tries to get 0 bytes (sidx_end_offset -
> dash_stream->sidx_current_offset) from the adapter forever

OK, I can also reproduce it by using my test streams.

Created attachment 347973 [details] [review]
dashdemux: Exception handle in _sidx_seek()

If target seek position is outside of the range of sidx entries,
binary search returns NULL pointer.

Created attachment 347974 [details] [review]
dashdemux: Two depth seeking for On-Demand profile

Try to find fragment using MPD first, then do refinement to find
target subframgnet using SIDX if possible. Note that, if target fragment
was moved from the previously activated one, we should assume that
the last SIDX is invalid for new fragment.

Created attachment 347975 [details] [review]
dashdemux: Reset SIDX related variables per fragment

SIDX's base offset (i.e., byte offset of SIDX + sidx.first_offset)
mostly vary as per fragment. Also, target SIDX index must be zero for the
new fragment.

Created attachment 347976 [details] [review]
dashdemux: Do not advace subfragment after pending SIDX seek done

If a MPD is On-Demand profile and no index described, demux will terminate
download loop after parsing inband SIDX with flow return custom-success.
At this moment, SIDX index is excat target position, but finish_fragment()
might cause re-advancing subfragment depending on MPD structure.

Created attachment 347977 [details] [review]
dashdemux: Drain buffer at the end of subfragment

Some of streams such as below have tailing boxes at the end of subfragment.
http://dash.akamaized.net/dash264/TestCases/1a/netflix/exMPD_BIP_TC1.mpd

Thanks, these patches look all good and I'll push them together with some additional changes I have locally here once I'm finished with them (there are some invalid memory accesses in relation to the sidx index).

Attachment 347973 [details] pushed as ad015ce - dashdemux: Exception handle in _sidx_seek()
Attachment 347974 [details] pushed as a599067 - dashdemux: Two depth seeking for On-Demand profile
Attachment 347975 [details] pushed as b65e6db - dashdemux: Reset SIDX related variables per fragment
Attachment 347976 [details] pushed as 42a38b6 - dashdemux: Do not advace subfragment after pending SIDX seek done
Attachment 347977 [details] pushed as 644192a - dashdemux: Drain buffer at the end of subfragment