Created attachment 348794 [details] [review]
adaptivedemux: Query updatability of manifest to subclass

If subclass cannot do, demux should push EOS to finish live streaming

Created attachment 348795 [details] [review]
dashdemux: Don't try mpd update if minimumUpdatePeriod is not present

Review of attachment 348794 [details] [review]:

Shouldn't this be also checked on the update_manifest call to skip updating it?

It should also be possible to interrupt the manifest update loop thread if no more updates are expected.

Created attachment 348930 [details] [review]
adaptivedemux: Stop updates loop if there is no more live playlist update

Although it's live playlist, updating playlist might be explicitly denied
by the given playlist

Created attachment 348931 [details] [review]
dashdemux: Don't try mpd update if minimumUpdatePeriod is not present

(In reply to Thiago Sousa Santos from comment #3)
> Review of attachment 348794 [details] [review] [review]:
> 
> Shouldn't this be also checked on the update_manifest call to skip updating
> it?
> 
> It should also be possible to interrupt the manifest update loop thread if
> no more updates are expected.

Thanks for your review.

I made it stop download loop.
And, instead of adding new virtual function, I changed it to use existing 
requires_periodical_playlist_update().

Review of attachment 348931 [details] [review]:

::: ext/dash/gstdashdemux.c
@@ +1819,3 @@
+      gst_buffer_unmap (buffer, &mapinfo);
+      return GST_FLOW_EOS;
+    }

Is this right? It could be that this is the last update that has new fragments but it also has removed the minimumUpdatePeriod. So we need this update and after this one we don't need to update no more.

Review of attachment 348930 [details] [review]:

I think it also would be a good idea to do the 'requires_periodical_playlist_update' check right from the updates loop. So that after an update we check if more updates are coming and, otherwise, we can stop that loop.

Created attachment 349608 [details] [review]
adaptivedemux: Stop updates loop if there is no more live playlist update

Created attachment 349609 [details] [review]
dashdemux: Don't try mpd update if minimumUpdatePeriod is not present

(In reply to Thiago Sousa Santos from comment #7)
> Review of attachment 348931 [details] [review] [review]:
> 
> ::: ext/dash/gstdashdemux.c
> @@ +1819,3 @@
> +      gst_buffer_unmap (buffer, &mapinfo);
> +      return GST_FLOW_EOS;
> +    }
> 
> Is this right? It could be that this is the last update that has new
> fragments but it also has removed the minimumUpdatePeriod. So we need this
> update and after this one we don't need to update no more.

You are correct:) I misunderstood at that point.

(In reply to Thiago Sousa Santos from comment #8)
> Review of attachment 348930 [details] [review] [review]:
> 
> I think it also would be a good idea to do the
> 'requires_periodical_playlist_update' check right from the updates loop. So
> that after an update we check if more updates are coming and, otherwise, we
> can stop that loop.

Please review updated patch :) As you suggested, stopping updates in the loop seems to looks better

Here's a trick question: What do you do if minimumUpdatePeriod is present ... but it's set to 0s ?

My understanding is that:
* it only implies that everything in the manifest is for everything available until "now"
* Whenever we go beyond the last segment present in the manifest we need to request an updated manifest

(In reply to Edward Hervey from comment #13)
> Here's a trick question: What do you do if minimumUpdatePeriod is present
> ... but it's set to 0s ?
This patch does not consider that condition. I'm planning to report now bug as I mentioned in bug #783028 if you ok. Abstract my idea for this zero minimumUpdatePeriod is,
- Disable periodic mpd update for zero minimumUpdatePeriod case.
- Update manifest only if the current manifest is expired
Note that, if inbandstream scheme (I'm working on bug#775803) is used, zero minimumUpdatePeriod is recommended. This is another usecase of zero minimumUpdatePeriod

> 
> My understanding is that:
> * it only implies that everything in the manifest is for everything
> available until "now"
That's my understanding. When I saw spec, it's still tricky for me, but DASHIF implementation guide is saying that
4.4.2.3.Some Special Cases
"If the MPD @minimumUpdatePeriod is set to 0, then the MPD documents all available  segments on the server."

> * Whenever we go beyond the last segment present in the manifest we need to
> request an updated manifest
Right. we should update at this moment or before.

Created attachment 353085 [details] [review]
dashdemux: Don't try periodical MPD update depending on minimumUpdatePeriod

Absent of minimumUpdatePeriod in dynamic type MPD means that
no more MPD update. See spec 5.3.1.2 Semantics of MPD element
Or, if minimumUpdatePeriod was zero, try update only if the last
MPD has been expired.

I hope attachment #353085 [details] can handle zero minimumUpdatePeriod. But not yet tested. I'm trying to find available test stream for that case.

Hello Edward Hervey and Thiago Sousa Santos

I tested using attached patch and following streams which were reported at
bug #783028 can be played.

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