GNOME Bugzilla – Bug 758257
adaptivedemux: don't expose pads until caps are known
Last modified: 2017-02-07 13:36:46 UTC
In implementing decodebin3, it's causing me pain that when it switches bitrates, adaptivedemux immediately adds new pads and removes old ones, but the caps on the new pads aren't known until some time later when HTTP data arrives. That increases the window in which the demuxer is not connected to anything, when in fact if we know the caps, we could immediately splice the output onto an existing demuxer, parser or decoder. My plan is: 1) Block all new streams. Keep a counter of number of pending streams. Store block-id on the Stream structure 2) When all new streams pre-roll (blocked => pending--, when pending == 0), unblock and expose them 3) Send EOS and remove old pads after new pads are exposed. 4) When handling a seek, unblock any blocked streams so the seek can be processed. Anyone see any problems?
Created attachment 316388 [details] [review] adaptivedemux: Preroll streams before exposing them To ensure that pads have caps when they are exposed, do the exposing when all pending streams have prerolled an output buffer, and only then EOS and remove any old pads. Improves the switching sequence by making caps available as soon as a pad appears.
Makes sense to me, let's go ahead with this and give it some wider testing? But with dash, can't we know the caps immediately from the Manifest anyway, so won't have to wait until actual data is available?
For DASH at least, for HLS we don't and typefind. So maybe needs two code paths? Ideally exposing pads immediately for DASH would be nice.
This seems to break seeking in multi-period DASH streams btw
Created attachment 316666 [details] [review] adaptivedemux: Preroll streams before exposing them To ensure that pads have caps when they are exposed, do the exposing when all pending streams have prerolled an output buffer, and only then EOS and remove any old pads. Improves the switching sequence by making caps available as soon as a pad appears.
This fixes it for me, but I think the condition variable and the expose counter handling is a bit fragile. Probably needs to be reset in more cases, like in general when streams are disappearing. But now I first have to look at a crash in dashdemux, exposed by this.
Also caused by this patch, when seeking between periods. The active_streams in the client are freed, but then later in the seeking code old active_stream is used.
Created attachment 343022 [details] [review] urisourcebin: Block demuxer's srcpad until get caps event Unlike dashdemux, HLS demux needs delay to find type. So, exposed pad might have NULL caps. For decision on slot reuse, urisourcebin shouldn't push eos until getting caps event from pending pad.
Hello slomo I have trouble with same issue. I think that urisourcebin seems right element for handling this.
No, this should be done correctly in adaptivedemux so that it only exposes pads once it knows the caps. That's how other code expects demuxers to behave already.
Created attachment 343078 [details] [review] [REBASE] adaptivedemux: Preroll streams before exposing them
Created attachment 343079 [details] [review] adaptivedemux: Block prerolling pads until exposed In download thread, event/buffer can be pushed before the corresponding pad is exposed. To ensure sequence, stream's srcpads will be blocked until exposing newpads and removing oldpads are finished.
Created attachment 343080 [details] [review] adaptivedemux: Activates only necessary thread's task Depending on current demuxer status, activates only needed thread. For example, when demuxer is preroll state, only new stream's downloading threads should be activated.
(In reply to Sebastian Dröge (slomo) from comment #10) > No, this should be done correctly in adaptivedemux so that it only exposes > pads once it knows the caps. That's how other code expects demuxers to > behave already. On top of "adaptive demux: Preroll streams before exposing them" patch, my two more patches can fix this issue by adaptivedemux itself
Review of attachment 343079 [details] [review]: I don't understand this patch. My previous patch split the prepare_streams and expose_streams steps into 2 functions, and only calls expose when a buffer is seen. The problem with blocking inside expose_streams() as well is that it will block an application seek() thread (usually the main thread) for an unknown length of time until data has been downloaded from the server, or a timeout if data doesn't arrive for a long time. The parts of the patch about storing events as sticky events seem OK as an optimisation, but shouldn't have fixed any actual bug afaict. I suspect the only part we actually need is the part that moves clearing the do_block flag after removing old pads?
Review of attachment 343080 [details] [review]: Generally seems OK. The other patches need to land first. ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +278,3 @@ +static void gst_adaptive_demux_start_tasks (GstAdaptiveDemux * demux, + gboolean is_preroll); Instead of 'is_preroll', call the variable 'start_preroll_streams' please - I think it's clearer.
(In reply to Jan Schmidt from comment #17) > Review of attachment 343079 [details] [review] [review]: > I suspect the only part we actually need is the part that moves clearing the > do_block flag after removing old pads? Not only clearing "do_block" parts, calling _expose_streams() in main thread seems to be needed. Frankly speaking, I'm not sure why your original patch does not work on master version. Maybe, I suspect that calling _expose_streams() in downloading thread might causes some issues and always calling it in downloading thread seems not to be unsafe. I'm keep debugging now, though. Main concept of my first patch is, calling _start_task() and _expose_streams() on main thread, and then, waiting until "preroll complete" (notifyed by download thread) What is your opinion? Any way, I'll split my patch into several parts for easy review.
If there's a problem calling expose_streams() in the downloading thread, we should work out what it is. Can you explain how / when it goes wrong for you? The problem with your patch is that it a) calls expose_streams() from the main thread and then b) that blocks until all the pads complete preroll anyway (preroll_pending reaches 0). There's no way to know how long that might block - with a slow internet connection it could be a very long time. The call to expose_streams has to happen from the streaming thread IMO - or we could use an auxilliary thread if there's really no way to make that reliable.
Created attachment 343232 [details] [review] adaptivedemux: Prepare GstSegment on only streams which are being prepared GstSegment is required for currently preparing stream, not for already existing streams.
Created attachment 343233 [details] [review] adaptivedemux: Activates only necessary thread's task Depending on current demuxer status, activates only needed thread. For example, when demuxer is preroll state, only new stream's downloading threads should be activated.
Created attachment 343234 [details] [review] adaptivedemux: Don't release manifest_lock before calling _expose_streams() Inside of _expose_streams(), manifest_lock will be released.
Created attachment 343235 [details] [review] adaptivedemux: Remove duplicated calling of _expose_streams() Because it will be called by _prepare_streams() in download thread
Created attachment 343236 [details] [review] adaptivedemux: Release block after removing oldstreams In order for parentbin (e.g., uriosourcebin) to know it's stream change case, nothing should be pushed to downstream until pushing eos to oldstreams' pads or removing the pads
Created attachment 343237 [details] [review] adaptivedemux: Ensure setting caps before exposing pad If not, exposed pad will have ANY caps
(In reply to Jan Schmidt from comment #19) > If there's a problem calling expose_streams() in the downloading thread, we > should work out what it is. Can you explain how / when it goes wrong for you? It was simple double mutex release issue :) Please refer to attachment 343234 [details] [review] > There's no way to know how long that might block - with a slow internet > connection it could be a very long time. The call to expose_streams has to > happen from the streaming thread IMO - or we could use an auxilliary thread > if there's really no way to make that reliable. OK, Thanks for your detailed comment :) I maintained your structure, and just fixed some other minor issues. Please review my patches.
Thanks, these all look good. If it's OK with you, I'd prefer to squash them all into my original commit and just add attribution when pushing them to git. Thank you for splitting it up though, much easier to review.
(In reply to Jan Schmidt from comment #27) > Thanks, these all look good. If it's OK with you, I'd prefer to squash them > all into my original commit and just add attribution when pushing them to > git. > > Thank you for splitting it up though, much easier to review. Sure, you can squash them into yours.
Created attachment 343242 [details] [review] adaptivedemux: Start download task depending on target streams Choose suitable target streams
There is one more fix
Hello Jan Schmidt If there is no more known issue, can we merge this on master branch and open to other developers? I think this approach must be accepted for playbin3 pipeline :)
Squashed and pushed: commit b2113f69c622c37c32d4336025f80a5c1d190897 Author: Jan Schmidt <jan@centricular.com> Date: Sat Jan 7 12:12:05 2017 +0900 adaptivedemux: Preroll streams before exposing them To ensure that pads have caps when they are exposed, do the exposing when all pending streams have prerolled an output buffer, and only then EOS and remove any old pads. Improves the switching sequence by making caps available as soon as a pad appears. With fixes from Seungha Yang <sh.yang@lge.com> https://bugzilla.gnome.org/show_bug.cgi?id=758257 I think there's more things to fix - but this code seems OK for the dev branch. Thanks for fixing up my code, and for your persistence!