GNOME Bugzilla – Bug 657790
[hlsdemux] Refactor element
Last modified: 2012-10-06 11:57:33 UTC
I have seen many changes in this element in the last week :) I had a refactoring branch pending to be submitted before the summer to fix the threading mess in this component but I have seen the Youness already sorted this out. I was wondering if this refactor still makes sense: * Factor out all the fetcher code in a new GObject that can be than be reused by other components. * Start using a GstFragment to manage fragments. This allows for instance an easy way to compute the download duration of each fragments that can be later be used for the bitrate-switch algoritm. You can find more details here: http://gitorious.org/ylatuya-gstreamer/gst-plugins-bad/commit/88790881ede8a226e4f47de4c40e5b7c1615c089
IMHO the refactoring makes sense in general and makes the code easier to read. Please go ahead and attach patches to this bug (but make it one commit per change, i.e. one commit for the GstFragment, one for the fetcher, etc)
This definitely looks interesting, but that page is huge, it's almost impossible to review. Could you rebase on top of the latest git and rework your code into smaller patches, I'd be glad to review the changes. Thanks for the suggestions!
That's the reason why I didn't submit it before the summer, I needed to split it up in several patches and didn't find tme until now :) I'll post again as soon as I have everything ready to be reviewed.
Created attachment 209930 [details] [review] refactor uri fetching Patch sourced from http://gitorious.org/ylatuya-gstreamer/gst-plugins-bad/trees/hlswip/gst/hls The only modification I have made from the source is to remove the dependency on GstFragment and use GstAdapter instead. I am not happy about how the canceling is managed, but I think this is the least intrusive way to patch the code in. Further optimisation & integration can be dealt with in new patches.
I wouldn't remove the use of the GstFragment. GstFragment was used instead of a simple GstAdapter to collect statistics about the downloaded fragment and reuse them in the heuristics for the bitrate adaptation (which is currently a very dumb one). An updated version of GstFragment can be found in #668094, where it was added to a base class for fragmented streaming. Also, GstFragment uses internally a GstBufferList, which should result in less copies than using an adapter(not 100% sure about that)
I have also been working on that here: http://cgit.collabora.com/git/user/tsaunier/gst-plugins-bad/log/?h=hlsdemux I just did a dumb rebase/commit split, and a few little changes. I still have a few known regressions, and I haven't taken the GstFragment from #668094 yet. Gil do you want to carry on that work or I should do it?
(In reply to comment #5) > I wouldn't remove the use of the GstFragment. GstFragment was used instead of a > simple GstAdapter to collect statistics about the downloaded fragment and reuse > them in the heuristics for the bitrate adaptation (which is currently a very > dumb one). > An updated version of GstFragment can be found in #668094, where it was added > to a base class for fragmented streaming. Also, GstFragment uses internally a > GstBufferList, which should result in less copies than using an adapter(not > 100% sure about that) I am aware of this that GstFragment is a better alternative. I was simply trying to isolate some of your code into a manageable patch. The GstFragment should be easy to add on top of this.
commit 70056a37c47d085c917f94574b76afb8c5f707dc Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Thu Apr 5 11:51:40 2012 -0400 hls: Some more debugging commit f9b0d59e84211fb822ca61634c57fe128e4dfeb0 Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Thu Mar 15 14:42:44 2012 -0400 hlsdemux: Replace the fetcher code with a GstURIDownloader object commit 2415f9080d3c772ad134b7283c07a8d58545198b Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Wed Mar 14 17:01:17 2012 -0400 hls: Minor cleanup in GstUriDownloader commit 71b9f57b63d472287c145245271c9ccc2ca6d483 Author: Andoni Morales Alastruey <ylatuya@gmail.com> Date: Wed Mar 14 17:06:22 2012 -0400 hlsdemux: Factor out all the fetcher code in a GstURIDownloader class This class is meant to be reusable by other components commit 5e85aaf11db9d2723e0ea01077ae34860a09da05 Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Thu Apr 5 10:22:29 2012 -0400 hls: Do not add reference to buffers passed in GstFragment We just steal the reference to the buffer, which means we can keep writing metadatas on the buffers. commit fdaa60c44fb9b27d75193b69c030c08f5b7f7740 Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Tue Mar 13 15:18:22 2012 -0400 hls: Minor GstFragment cleanup commit 501d42fa7809d1899cfd88459face69d5f37976d Author: Andoni Morales Alastruey <ylatuya@gmail.com> Date: Fri Jul 8 01:09:00 2011 +0200 hls: Add a GstFragment class that represents a fragment in a m3u playlist commit fef060590f698b92458c8fdc345f34c2105de458 Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Thu Mar 15 18:21:58 2012 -0400 hls: Make the updates thread a GstTask
This slew of unannounced patches has broken the module. Playback simply stalls after the initial 3 segments have been displayed. For example, it stalls on frame 897 with: gst-launch-0.10 playbin2 uri=http://devimages.apple.com/iphone/samples/bipbop/gear4/prog_index.m3u8
Any news on this?
The brokeness might actually be caused by an incompatibility with the latest libsoup release which I updated at the same time. I will investigate further.
Created attachment 212776 [details] [review] regression fix The problem was caused by the stream_task which would pause itself on empty queues, while no one started it again. The attached patch fixes this issue, restoring functionality of the module.
commit f4dbfe947d7c085b568b58b09205aaef851896d1 Author: Gil Pedersen <git@gpost.dk> Date: Wed Apr 25 13:31:36 2012 +0200 hlsdemux: start paused task on new data