Created attachment 328803 [details] [review]
qtdemux: Don't override TIME segments from upstream that we just saw

The point of d8fb7a9c96b108814beeaa0e63f818d4648c7fe9 was to not have any
spurious segments stored for later if we do BYTES->TIME conversion, but
overriding any TIME segments from upstream does not make any sense.

See https://bugzilla.gnome.org/show_bug.cgi?id=763165

Created attachment 328804 [details] [review]
qtdemux: In PULL mode, nothing is ever going to send us a SEGMENT event

Created attachment 328805 [details] [review]
qtdemux: Only activate segments and send SEGMENT events if we have streams

But in that case also remove the pending newsegment event, otherwise we would
later send a possibly outdated event.

Created attachment 328806 [details] [review]
qtdemux: Use the demuxer segment instead of a new one for MSS streams

Upstream might have told us something about the to be expected segment, so
let's use that information instead of coming up with a [0,-1] segment.

Created attachment 328807 [details] [review]
qtdemux: Make sure to always forward SEGMENT events even if there is no stream segment in the right range

For DASH we will get SEGMENTS after all stream segments of the current moof.
We should forward them and then the following moof will contain data for this
segment.

This is a regression caused by 00f23053b1f27c6857a2153b39e1344d2d6e9eaf, which
caused seeking in DASH streams to send no SEGMENT events.

See https://bugzilla.gnome.org/show_bug.cgi?id=765669

Review of attachment 328807 [details] [review]:

Maybe this one here should always forward the segment for fragmented, and only not do that for normal MP4?

Review of attachment 328804 [details] [review]:

Looks good

Review of attachment 328803 [details] [review]:

Looks good, one minor corner case that might never happen is when receiving a non-byte and non-time segment. Then the old segment would be lost and the new one ignored. I don't thinks there is a valid use case for this, so just push.

Review of attachment 328806 [details] [review]:

Looks good, please push.

Review of attachment 328807 [details] [review]:

dash should default to creating a 'dummy segment' that has its stop time updated and increasing when new data is parsed. So what happens here is that the segment isn't activated as the stop-time is still too small if you seek forward to a fragment that isn't available yet.

Forwarding the segment directly for fragmented would work, but I think just doing it when dummy_segment == true would cover more cases. Also it worries me a bit that validate didn't catch any of these issues, will check what tests are missing there.

With the exception of this one, all the other patches look good, so feel free to push. Will report back when I investigate a bit on validate's side.

(In reply to Thiago Sousa Santos from comment #10)
> Review of attachment 328807 [details] [review] [review]:
> 
> dash should default to creating a 'dummy segment' that has its stop time
> updated and increasing when new data is parsed. So what happens here is that
> the segment isn't activated as the stop-time is still too small if you seek
> forward to a fragment that isn't available yet.
> 
> Forwarding the segment directly for fragmented would work, but I think just
> doing it when dummy_segment == true would cover more cases. Also it worries
> me a bit that validate didn't catch any of these issues, will check what
> tests are missing there.

What's dummy_segment and how is that supposed to work? The problem here is that adaptivedemux will have to push a SEGMENT event for the seek, and only after that it can push the new moof from which qtdemux would know that there is actually data available at the segment start.

(In reply to Sebastian Dröge (slomo) from comment #11)
> (In reply to Thiago Sousa Santos from comment #10)
> > Review of attachment 328807 [details] [review] [review] [review]:
> > 
> > dash should default to creating a 'dummy segment' that has its stop time
> > updated and increasing when new data is parsed. So what happens here is that
> > the segment isn't activated as the stop-time is still too small if you seek
> > forward to a fragment that isn't available yet.
> > 
> > Forwarding the segment directly for fragmented would work, but I think just
> > doing it when dummy_segment == true would cover more cases. Also it worries
> > me a bit that validate didn't catch any of these issues, will check what
> > tests are missing there.
> 
> What's dummy_segment and how is that supposed to work? The problem here is
> that adaptivedemux will have to push a SEGMENT event for the seek, and only
> after that it can push the new moof from which qtdemux would know that there
> is actually data available at the segment start.

As there is no edts, qtdemux fakes it by creating a qt-segment that goes from 0 to the current last timestamp. With dash it should grow a bit after every moof. The problem is that the user can seek to a position much more forward than what qtdemux has seen so far, so it will just assume it is EOS.

The fix by forwarding the segment event directly if it is fragment already improves the problem for dash but I think there is a more generic way to just forward it for streams that use this 'dummy segment'.

As the forward for fragmented already improves the situation we can push that to make things work again and then improve it again by checking the dummy_segment flag once I figure out how to improve validate to properly cover these use cases.

Not sure how that would work :) The code seems to create this dummy segment based on the known stream duration, but the problem here is that we don't know the stream duration (all we know is the duration of the current moof and the seek start position which is after that).

Do you propose to create a new dummy segment that would update the end time of the dummy segment to the seek start position?

However doing so would first forward a segment event that ends at the seek position, and then possibly later we would create the correct segment event.

Or would you, if dummy_segment, not forward anything and wait until the next moof before forwarding?

Attachment 328803 [details] pushed as f3e6816 - qtdemux: Don't override TIME segments from upstream that we just saw
Attachment 328804 [details] pushed as f8eb909 - qtdemux: In PULL mode, nothing is ever going to send us a SEGMENT event
Attachment 328805 [details] pushed as 84e698c - qtdemux: Only activate segments and send SEGMENT events if we have streams
Attachment 328806 [details] pushed as 4498e57 - qtdemux: Use the demuxer segment instead of a new one for MSS streams

That code also breaks seeks in DASH in other ways. If you do a seek that actually is inside the current range of the moof (that is, it doesn't go into my fallback code I added in the remaining patch here), we would forward a segment with the moof range downstream (that is, with a stop position). Unfortunately that stop position is the one of this single moof, not the one of the whole DASH stream ;)

All this was not a problem before, as we just forwarded the upstream TIME segments in push mode directly, instead of going through the segment activation code and other things.

I think it doesn't make sense to go through that if upstream is in TIME and we are handling fragmented input. Upstream is going to know better about segments than we do.

Created attachment 328905 [details] [review]
qtdemux: Forward segments directly if we are operating in PUSH mode on fragmented streams

We shouldn't go through segment activation as we will only have a limited
understanding of how the whole stream timeline looks like from the moof. We
only know about the current fragment, while upstream knows about the whole
stream.

This fixes seeking in DASH streams, both for seeks after the current moof and
for seeks into the current moof. The former would fail because the moof ends
and we can't activate any segment, the latter would cause a segment that stops
at the moof end, and no further fragments would be played because we end up
being EOS.

This solves it independent of the other pending patch. I think for the other pending patch it would make sense to at least check if any segment was activated at all per stream, and to error out otherwise (where my patch currently would just forward the segment). Currently this is just ignored and then bad things happen.

Attachment 328803 [details] pushed as f3e6816 - qtdemux: Don't override TIME segments from upstream that we just saw
Attachment 328806 [details] pushed as 4498e57 - qtdemux: Use the demuxer segment instead of a new one for MSS streams

These two also went into 1.8 now as they relate to a blocker bug: bug #766868

*** Bug 766868 has been marked as a duplicate of this bug. ***

Comment on attachment 328905 [details] [review]
qtdemux: Forward segments directly if we are operating in PUSH mode on fragmented streams

Let's get this in for now to fix seeking on adaptivedemux streams, can still look for a better solution later.

How should we proceed?

Not really a blocker anymore, the regressions are all gone there's just more weirdness left

-- 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-good/issues/276.