qtdemux: Various SEGMENT event related fixes, including regression fixes
Submitted by Sebastian Dröge @slomo
Link to original bug (#767071)
Description
See commit messages
See also
Activity
@slomosubmitted a patch:The point of d8fb7a9c 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.Patch 328803, "qtdemux: Don't override TIME segments from upstream that we just saw":
qtdemux-Dont-override-TIME-segments-from-upstream-.patch@slomosubmitted a patch:Patch 328804, "qtdemux: In PULL mode, nothing is ever going to send us a SEGMENT event":
qtdemux-In-PULL-mode-nothing-is-ever-going-to-send.patch@slomosubmitted a patch:But in that case also remove the pending newsegment event, otherwise we would
later send a possibly outdated event.Patch 328805, "qtdemux: Only activate segments and send SEGMENT events if we have streams":
qtdemux-Only-activate-segments-and-send-SEGMENT-ev.patch@slomosubmitted a patch: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.Patch 328806, "qtdemux: Use the demuxer segment instead of a new one for MSS streams":
qtdemux-Use-the-demuxer-segment-instead-of-a-new-o.patch@slomosubmitted a patch: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 00f23053, which
caused seeking in DASH streams to send no SEGMENT events.Patch 328807, "qtdemux: Make sure to always forward SEGMENT events even if there is no stream segment in the right range":
qtdemux-Make-sure-to-always-forward-SEGMENT-events.patch
@thiagossantosreviewed patch 328807: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.
@slomosaid:(In reply to Thiago Sousa Santos from comment 10)
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.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.
@thiagossantossaid:(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]:
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.
@slomosaid: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?
@slomocommitted some patches:Attachment 328803 pushed as f3e68164 - qtdemux: Don't override TIME segments from upstream that we just saw
Attachment 328804 pushed as f8eb909d - qtdemux: In PULL mode, nothing is ever going to send us a SEGMENT event
Attachment 328805 pushed as 84e698c5 - qtdemux: Only activate segments and send SEGMENT events if we have streams
Attachment 328806 pushed as 4498e57c - qtdemux: Use the demuxer segment instead of a new one for MSS streams@slomosaid: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 ;)
@slomosaid: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.
@slomosubmitted a patch: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.Patch 328905, "qtdemux: Forward segments directly if we are operating in PUSH mode on fragmented streams":
qtdemux-Forward-segments-directly-if-we-are-operat.patch@slomosaid: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.
@slomosaid:Attachment 328803 [details] pushed as f3e68164 - qtdemux: Don't override TIME segments from upstream that we just saw
Attachment 328806 [details] pushed as 4498e57c - qtdemux: Use the demuxer segment instead of a new one for MSS streamsThese two also went into 1.8 now as they relate to a blocker bug: bug 766868
