Yeah, I just ran into this here :) Will revert it together with another fix.

Actually see my comments in the unit test coverage bug. Why do you think your change was wrong? I reverted it now, but actually I'm not sure anymore with the reasoning I had. Your change seems correct to me now.

Marking as blocker for now, we should revert the revert or fix something else :) https://bugzilla.gnome.org/show_bug.cgi?id=752027#c9 for reference.

the new problem is in commit 07d27d906af32c82b0d7460ef63aabf073906df0
mpdparser: Fix off-by-one in has-next-segment calculation

Why did you made that change?

segment_index indicates the next segment. So the function gst_mpd_client_has_next_segment needs tor return true if segment_index is in valid array range.

The fact that you added "&& stream->segment_index + 1 >= segments_count" is wrong.
The definition of segment_index is:
struct _GstActiveStream
{
...
gint segment_index;                         /* index of next sequence chunk */
...
}

So it should not be offset by 1. The code was correct.

It was strange that the developer choose to keep the index of the next segment, instead of the natural way of keeping the index of the current segment. It confused me when trying to understand what this index really points to and when it is valid and when not.

Please revert commit 07d27d906af32c82b0d7460ef63aabf073906df0

(In reply to Florin Apostol from comment #4)
> the new problem is in commit 07d27d906af32c82b0d7460ef63aabf073906df0
> mpdparser: Fix off-by-one in has-next-segment calculation
> 
> Why did you made that change?

See the related adaptivedemux change. Consider the following case:
- we're currently downloading segment 304 of 305
- downloading fails
- has_next_segment() still returns TRUE because 304 < 305

but: 304 is the last segment of the stream. It has no next segment anymore.

> It was strange that the developer choose to keep the index of the next
> segment, instead of the natural way of keeping the index of the current
> segment. It confused me when trying to understand what this index really
> points to and when it is valid and when not.
> 
> Please revert commit 07d27d906af32c82b0d7460ef63aabf073906df0

I think your understanding of it is not correct, it's at least not what happens in adaptivedemux. The segment index is the index of the current segment we're downloading until that download is finished. Only once the download is finished it will be incremented with advance_segment.

(In reply to Florin Apostol from comment #4)
> The definition of segment_index is:
> struct _GstActiveStream
> {
> ...
> gint segment_index;                         /* index of next sequence chunk
> */
> ...
> }

I would argue that this comment is either outdated, or things are inconsistent between adaptivedemux and dashdemux/mpdparser. It all seems correct in the code with the interpretation of segment_index being the *current* index though.


Especially it is initialized at 0 and not at 1, it must be initialized to 1 if the above comment was correct.

Or maybe the comment means: index of the next sequence chunk that will be downloaded.

Florin, any thoughts? I think I'd like to introduce the patch from bug #751850 again ASAP

I agree that segment_index is the index of the next sequence chunk to be downloaded. So, the comment from its definition is correct.

All the function that uses it seems to have the same understanding of its meaning:
gst_mpd_client_get_next_fragment, gst_mpd_client_get_next_fragment_duration, gst_mpd_client_get_next_fragment_timestamp all use segment_index value to retrieve the next fragment. They don't do any incrementing.

I'm not sure what is the difference between a segment and a fragment. I believe they are synonyms and they refer to the same thing. Can you confirm this?

I still believe the code was originally correct and you do not need to add 1 in gst_mpd_client_has_next_segment. If you still think that the 1 offset needs to be there, then we probably need to update all other functions that validate the index.

I expect the usage of these functions to be something like:
while (gst_mpd_client_has_next_segment())
{
  gst_mpd_client_get_next_fragment (fragment);
  download fragment
  gst_mpd_client_advance_segment()
  if download successful
    use downloaded data
}

This works correct with initializing segment_index to 0 and does not need to be initialized to 1.

The fact that download failed should not impact the index advance. We just can't use that data, but the index should be updated. What if the download failed for packet 100/305. Will you still consider that the stream has no next segment?

I'll spent some time today trying to get a better understanding of segments, fragments and how they are used in dashdemux

(In reply to Florin Apostol from comment #9)
> I agree that segment_index is the index of the next sequence chunk to be
> downloaded. So, the comment from its definition is correct.
> 
> All the function that uses it seems to have the same understanding of its
> meaning:
> gst_mpd_client_get_next_fragment, gst_mpd_client_get_next_fragment_duration,
> gst_mpd_client_get_next_fragment_timestamp all use segment_index value to
> retrieve the next fragment. They don't do any incrementing.

Correct, and they use the current value of the segment_index.

> I'm not sure what is the difference between a segment and a fragment. I
> believe they are synonyms and they refer to the same thing. Can you confirm
> this?

Correct

> I still believe the code was originally correct and you do not need to add 1
> in gst_mpd_client_has_next_segment. If you still think that the 1 offset
> needs to be there, then we probably need to update all other functions that
> validate the index.

Yeah, need to check the others too then :)

> I expect the usage of these functions to be something like:
> while (gst_mpd_client_has_next_segment())
> {
>   gst_mpd_client_get_next_fragment (fragment);
>   download fragment
>   gst_mpd_client_advance_segment()
>   if download successful
>     use downloaded data
> }
> 
> This works correct with initializing segment_index to 0 and does not need to
> be initialized to 1.

It doesn't. Consider the case with segment_index=0, segments_count=1:
First loop: has_next_segment==TRUE (0 < 1), advance (0 -> 1)
Second loop: has_next_segment==FALSE (1 < 1)

Now for the first loop, there is no segment after the first one. So it should not return TRUE there.


The code in adaptivedemux is more like

do {
  get_segment();
  if (success)
    advance();
  else if (failed_too_often)
    error();
} while (has_next_segment());

> The fact that download failed should not impact the index advance. We just
> can't use that data, but the index should be updated. What if the download
> failed for packet 100/305. Will you still consider that the stream has no
> next segment?

What do you mean? The problem is that has_next_segment() returns TRUE for the last segment without my change. It only returns FALSE once the current segment is after the last one then.

> 
> > I expect the usage of these functions to be something like:
> > while (gst_mpd_client_has_next_segment())
> > {
> >   gst_mpd_client_get_next_fragment (fragment);
> >   download fragment
> >   gst_mpd_client_advance_segment()
> >   if download successful
> >     use downloaded data
> > }
> > 
> > This works correct with initializing segment_index to 0 and does not need to
> > be initialized to 1.
> 
> It doesn't. Consider the case with segment_index=0, segments_count=1:
> First loop: has_next_segment==TRUE (0 < 1), advance (0 -> 1)
> Second loop: has_next_segment==FALSE (1 < 1)
> 
> Now for the first loop, there is no segment after the first one. So it
> should not return TRUE there.

It should return true. Because the first segment is the "next segment" (segment_index=0). Nothing has been downloaded yet, so there is a next segment. The function returns true, the get_next_fragment will obtain the uri, the download will download it. Your get_segment() should contain:
   gst_mpd_client_get_next_fragment (fragment);
   download fragment
After download you call advance. Only after this, a call to get_next_segment will return false (because the stream had 1 next segment, we downloaded it, and now there is no next segment).

> 
> 
> The code in adaptivedemux is more like
> 
> do {
>   get_segment();
>   if (success)
>     advance();
>   else if (failed_too_often)
>     error();
> } while (has_next_segment());
> 
> > The fact that download failed should not impact the index advance. We just
> > can't use that data, but the index should be updated. What if the download
> > failed for packet 100/305. Will you still consider that the stream has no
> > next segment?
> 
> What do you mean? The problem is that has_next_segment() returns TRUE for
> the last segment without my change. It only returns FALSE once the current
> segment is after the last one then.

It returns true because you did not advance it yet. segment_index points to the same segment you attempted to download. That is the "next segment". Until you confirm the download with an advance_segment call, every call to get_next_segment will return true.

Your problem is that in the download, in case of errors, you want to see if there is a segment beyond the one you are downloading. That should be a "next next segment". And with current API, the only way to see that is to call advance and then call has_next_segment. But you cannot call advance in there because it will be called again after the download (by gst_adaptive_demux_stream_advance_fragment_unlocked).

With the current API, I don't think you can make the EOS conversion in gst_adaptive_demux_stream_download_fragment.

I don't think I agree here. Do you see an actual problem, if so which?

All uses of segment_index seem to be for the "current" segment, that is the one that is currently going to be downloaded. Basically the "current" position. And that's also how this information is used inside adaptivedemux.


The only thing that looks wrong to me now is gst_mpd_client_advance_segment(), which should be like in your patch that I reverted.

(In reply to Florin Apostol from comment #11)
> It should return true. Because the first segment is the "next segment"
> (segment_index=0). Nothing has been downloaded yet, so there is a next
> segment.

It should return FALSE because there is no next segment after the one we're currently at. So we download the current one and then EOS because there's no next segment.

(In reply to Sebastian Dröge (slomo) from comment #12)
> I don't think I agree here. Do you see an actual problem, if so which?
> 
> All uses of segment_index seem to be for the "current" segment, that is the
> one that is currently going to be downloaded. Basically the "current"
> position. And that's also how this information is used inside adaptivedemux.
> 
That's correct. The problem is that dashdemux calls this current segment "next segment". So functions are named get_next_fragment, gst_mpd_client_has_next_segment, etc. They all use the current value of segment_index, so they are referring to your current segment, but the function names contain a misleading "next" word in them. Thy should be called has_current_segment, get_current_fragment, etc.

If you accept this, then you realize there is no function to see if there is a segment beyond the current one. The has_next_segment is an unfortunate naming for get_current_segment and there is no proper has_next_segment function

Yes, everything that refers to "next" segment actually means the current one, except for has_next_segment(). The naming is a bit broken indeed.

has_next_segment() is used to check if there is a segment after the current one, and that's what it actually does after my change. All the other "next" functions are getting timestamps, durations, URIs, etc for the current segment. And advance is switching the current segment to the next one.

I agree with this. In light of these, I propose to rename the broken functions to contain the "current" name.

The has_next_segment should indeed be offset by 1.

And my patch should be reapplied. 
I will also update the unit tests to accommodate to the new logic.

I guess you already figured it all out, but just some clarification:

Segment == fragment here, I guess the mismatches are because the different specs (HLS/DASH/MSS) calls them differently and the naming got mixed when doing the API.

the segment_index is the 'next' fragment to download, meaning it is initialized at 0 because get_next_fragment will return the next fragment to be downloaded. It is just one logical way of thinking, it could also be named the current fragment if we changed all the function names. I'm ok if you wanted to change to make it more clear.

When at the last fragment, advance_fragment() should increment the index (or decrement if reverse playback) and, as it is out of the fragments range, return EOS in this case.

So now get_next_fragment returns the fragment from the current index (the next fragment to download) but has_next_fragment looks at segment_index+1! Indeed, the names are misleading.

Some function names must be changed.

Alright, let's revert the revert then ;)

commit a755fbb44021c84535d7f1ecd1ab85fdbb6b9afa
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Jul 8 16:31:48 2015 +0300

    Revert "Revert "dashdemux: fixed gst_mpd_client_advance_segment to return GST_FLOW_EOS""
    
    This reverts commit 4875ddf5855a26f349df7b385b68eb692e314bfa.
    
    This was based on a misunderstanding of the code.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=752085


And renaming as a next step here.

Florin, are you updating the tests? Let us know if you have a patch.

tests updated. Se the patch in https://bugzilla.gnome.org/show_bug.cgi?id=752027

Created attachment 307114 [details] [review]
proposed patch

attached a patch that renames "next" to "current" and "fragment" to "segment" in dashdemux

Comment on attachment 307114 [details] [review]
proposed patch

This does not apply anymore. We should either just merge it directly when updated, or close this bug. It's going to not be mergeable again otherwise due to changes

-- 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/270.