GNOME Bugzilla – Bug 705079
hlsdemux: switch fragments to buffer lists to avoid copies
Last modified: 2014-05-03 08:01:23 UTC
Created attachment 250372 [details] [review] store GstFragment data in a GstBufferList Since the port to 0.11, buffers downloaded by uridownloader in a fragment are merged using gst_buffer_append. In theory this is the best way to do it, except that the maximum number of memory blocks in a single buffer is 16. After that all the memory blocks are merged to make room for the next append, which can trigger very big memcpy's while downloading. Instead switch back to buffer lists like it was done before 0.11. I've added a gst_fragment_extract function to copy data from the buffer list to a buffer. This allows merging the buffer list for the playlist or for the decrypt key. I've also rewritten the decryption process to work in place the buffer list. Before the single input buffer was decrypted in a new buffer, resulting in another big memory allocation.
Review of attachment 250372 [details] [review]: ::: ext/hls/gsthlsdemux.c @@ +1322,3 @@ + + /* Make sure we are aligned to the cipher block size */ + buf_size &= ~(buf_size & (block_size - 1)); Doesn't this cause us to decrypt parts of the buffer twice? @@ +1342,3 @@ + /* Handle pkcs7 unpadding here */ + gsize unpadded_size = map.size - map.data[map.size - 1]; + gst_buffer_resize (buffer, 0, unpadded_size); In theory the unpadding could remove one or more buffers from the list in the end. ::: gst-libs/gst/uridownloader/gstfragment.c @@ +47,2 @@ GstCaps *caps; + gsize size; Why not make the size a property too? @@ +245,3 @@ + buffer = gst_buffer_list_get (fragment->priv->buffer_list, 0); + if (buffer) { + caps = gst_type_find_helper_for_buffer (NULL, buffer, NULL); This could mean that you only typefind on a very small buffer, which might fail just because of that ::: gst-libs/gst/uridownloader/gstfragment.h @@ +61,3 @@ GType gst_fragment_get_type (void); +GstBufferList * gst_fragment_get_buffer_list (GstFragment *fragment); This also needs changes in dashdemux and mssdemux
(In reply to comment #1) > Review of attachment 250372 [details] [review]: > > ::: ext/hls/gsthlsdemux.c > @@ +1322,3 @@ > + > + /* Make sure we are aligned to the cipher block size */ > + buf_size &= ~(buf_size & (block_size - 1)); > > Doesn't this cause us to decrypt parts of the buffer twice? No, this clips the buffer size to be a multiple of the block size, the left bytes are copied in the temporary block, and when it is filled and decrypted the data is copied back to the buffers. This is a little convoluted but I don't see how to do it without copying or allocating too much data. > > @@ +1342,3 @@ > + /* Handle pkcs7 unpadding here */ > + gsize unpadded_size = map.size - map.data[map.size - 1]; > + gst_buffer_resize (buffer, 0, unpadded_size); > > In theory the unpadding could remove one or more buffers from the list in the > end. Right, I'll change this. > > ::: gst-libs/gst/uridownloader/gstfragment.c > @@ +47,2 @@ > GstCaps *caps; > + gsize size; > > Why not make the size a property too? > Ok. > @@ +245,3 @@ > + buffer = gst_buffer_list_get (fragment->priv->buffer_list, 0); > + if (buffer) { > + caps = gst_type_find_helper_for_buffer (NULL, buffer, NULL); > > This could mean that you only typefind on a very small buffer, which might fail just because of that Yes, I've looked for a function in typefind that tells me the minimal amount of data needed to do proper typefinding but I couldn't find one, like the max-extent in shared-mime-info. In the end the caps shouldn't be detected like this, the allowed data types are defined in the HLS spec so the caps should be set manually depending of the playlist info. > > ::: gst-libs/gst/uridownloader/gstfragment.h > @@ +61,3 @@ > GType gst_fragment_get_type (void); > > +GstBufferList * gst_fragment_get_buffer_list (GstFragment *fragment); > > This also needs changes in dashdemux and mssdemux Ok, I should have grepped for other users.
Created attachment 250627 [details] [review] uridownloader: use buffer lists to store fragments I've taken another approach on this: GstFragment now uses GstBufferList to store data, but I've kept the gst_fragment_get_buffer function for compatibility and simplicity. This function now merges all the buffers in the buffer list in one single buffer. This avoids copying all data while accumulating the memory blocks, the data is only copied once in the end. The improvement can be seen when running with GST_CAT_PERFORMANCE:5 logging, much less copies happen. Note that the typefinding issue is still there, although it is less likely to happen since the first buffer now contains 16 memory blocks. I also have patches that updates the demuxers to use buffer lists directly when possible, that I might send later.
Review of attachment 250627 [details] [review]: Looks generally good. Could you, on top of that, provide patches to at least one of the elements (hlsdemux probably then? :) ) that uses the new bufferlist functionality? ::: gst-libs/gst/uridownloader/gstfragment.c @@ -37,3 @@ PROP_DURATION, PROP_DISCONTINOUS, - PROP_BUFFER, Keep the buffer property if you keep get_buffer() too @@ +310,3 @@ + + if (n_blocks <= gst_buffer_get_max_memory ()) { + last_buffer = gst_buffer_append (last_buffer, buffer); I think you should adjust GST_BUFFER_OFFSET() and GST_BUFFER_OFFSET_END() here when appending memories
Arnaud?
Sorry I'm working on something else ATM, I'll get back to this ASAP :)
Arnaud? :) I could finish this too if necessary
Hi Sebastian, my hls branch on github has a final version of this patch: https://github.com/rawoul/gst-plugins-bad/commits/hls I see that you have pushed some patches on hlsdemux so there might be some conflicts. I won't be able to work on this for the next two weeks, but feel free to cherry-pick any patch and push them.
Thanks, I merged some of those changes, added a comment to one. Will look at the others once I have some time :)
This is not needed anymore. hlsdemux is now directly pushing buffers downstream as they are received from the source, and not by fragment.