After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 751241 - vtdec: handle non-consecutive GstBuffer input without copying
vtdec: handle non-consecutive GstBuffer input without copying
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Mac OS
: Normal enhancement
: 1.5.90
Assigned To: Ilya Konstantinov
GStreamer Maintainers
Depends on: 751641
Blocks:
 
 
Reported: 2015-06-19 23:02 UTC by Ilya Konstantinov
Modified: 2015-08-16 13:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
vtdec: handle non-consecutive GstBuffer input without copying (5.03 KB, patch)
2015-06-29 10:27 UTC, Ilya Konstantinov
none Details | Review
vtdec: handle non-consecutive GstBuffer input without copying (5.16 KB, patch)
2015-07-13 14:57 UTC, Ilya Konstantinov
committed Details | Review

Description Ilya Konstantinov 2015-06-19 23:02:56 UTC
CMBlockBuffer offers a model similar to GstBuffer, whereas it can consist of multiple non-consecutive memory blocks.

Instead of using gst_buffer_map, we should:
- iterate over the GstBuffer's memories
- map the GstMemory objects individually
- create a CMBlockBufferCustomBlockSource {
   .AllocateBlock = nil,
   .FreeBlock = { wrapper function that does gst_memory_unmap + unref },
   .refCon = gst_mem
  } for each memory
- call CMBlockBufferAppendMemoryBlock, passing the mapped pointer and the custom block source

This complements bug 751071. Possibly, this function should be moved out into the applemedia library.
Comment 1 Ilya Konstantinov 2015-06-29 10:27:15 UTC
Created attachment 306272 [details] [review]
vtdec: handle non-consecutive GstBuffer input without copying

This patch implements non-contiguous CMBlockBuffers.

In the process, I've realised we were using CMBlockBufferCreateWithMemoryBlock improperly. It does NOT copy its input buffer, so gst_buffer_unmap-ing immediately is incorrect. In the new implementation, its the CMBlockBuffer's responsibility to unmap.

P.S. Once bug 747707 lands, I think cm_block_buffer_from_gst_buffer should move into the library.
Comment 2 Nicolas Dufresne (ndufresne) 2015-07-13 14:14:06 UTC
Review of attachment 306272 [details] [review]:

Looks good to me except for one comment. Can this be merged right away or does it depend on something else ?

::: sys/applemedia/vtdec.c
@@ +585,3 @@
+    GstMapInfo *info;
+
+    mem = gst_memory_ref (gst_buffer_peek_memory (buf, i));

Use gst_buffer_get_memory(buf, i);
Comment 3 Ilya Konstantinov 2015-07-13 14:15:29 UTC
Depends on nothing else, please merge.
Comment 4 Ilya Konstantinov 2015-07-13 14:57:49 UTC
Created attachment 307354 [details] [review]
vtdec: handle non-consecutive GstBuffer input without copying

CMBlockBuffer offers a model similar to GstBuffer, as it can
consist of multiple non-consecutive memory blocks.

Prior to this change, what we were doing was:

 1) Incorrect:

   CMBlockBufferCreateWithMemoryBlock does not copy the data,
   but we gst_buffer_unmap'd right away.

 2) Inefficient:

   If the GstBuffer consisted of non-contiguous memory blocks,
   gst_buffer_map resulted in malloc / memcpy.

With this change, we construct a CMBlockBuffer out of individual mapped
GstMemory objects. CMBlockBuffer is made to retain the GstMemory
objects (through the use of CMBlockBufferCustomBlockSource), so the
original GstBuffer can be unref'd.
Comment 5 Ilya Konstantinov 2015-07-13 14:59:58 UTC
Changes:

- gst_buffer_peek_memory -> gst_buffer_get_memory
- removed function free_map_info_slice
- added missing gst_memory_unref (mem)
- added comment
Comment 6 Nicolas Dufresne (ndufresne) 2015-07-13 15:11:58 UTC
Attachment 307354 [details] pushed as bfa054a - vtdec: handle non-consecutive GstBuffer input without copying