GNOME Bugzilla – Bug 752116
adapter: failure of gst_adapter_get_(buffer_)list
Last modified: 2015-08-16 13:39:52 UTC
I made new test unit for these new apis. But it's failed. Unless this is wrong test code, it should succeed.
Created attachment 307067 [details] [review] adapter: unit test for new get_(buffer_)list IMHO, in gst_adapter_get_buffer_list, in while loop, cur = adapter->buflist->data; But cur always points out to the first buffer. Because gst_adapter_get_buffer doesn't flush anything and not update any index(something like adapter->skip) Does this need to fix?
Yes, the test is correct and the adapter code is wrong. Want to provide a patch?
Instead of get_buffer(), these functions should iterate over the buffer list themselves I guess. The problem is that the current code assumes that get_buffer() removes the buffer, but that's obviously not correct.
Basically it should replicate must of the code in gst_adapter_get_buffer() and iterate longer over the to accumulate all buffers needed.
Created attachment 307076 [details] [review] adapter: fix to get valid (buffer_)list get_list/get_buffer_list should be done with buffers in adapter remaining while take_list/take_buffer_list flushes each buffer one by one.
Review of attachment 307076 [details] [review]: ::: libs/gst/base/gstadapter.c @@ +1101,3 @@ + g = adapter->buflist; + skip = adapter->skip; IIRC skip should be set to 0 after the first buffer is done. It is the number of bytes to skip from the first buffer IIRC.
Created attachment 307084 [details] [review] adapter: fix to get valid (buffer_)list Yes. I should've considered the case that skip is non-0. :)
commit af62e2874fb2d84b91a6f5510a4ca5a8a0e72139 Author: Hyunjun Ko <zzoon.ko@samsung.com> Date: Thu Jul 9 00:21:42 2015 +0900 adapter: fix to get valid (buffer_)list get_list/get_buffer_list should be done with buffers in adapter remaining while take_list/take_buffer_list flushes each buffer one by one. https://bugzilla.gnome.org/show_bug.cgi?id=752116 commit 9338f93494c96f810c3abe1b478b59066397275a Author: Hyunjun Ko <zzoon.ko@samsung.com> Date: Wed Jul 8 20:06:27 2015 +0900 adapter: unit test for new get_(buffer_)list
Review of attachment 307084 [details] [review]: ::: libs/gst/base/gstadapter.c @@ +1108,3 @@ hsize = MIN (nbytes, cur_size - skip); + if (skip == 0 && cur_size == hsize) { I think you wan to gst_buffer_ref the buffer ic cur_size <= to hsize, otherwise it will create a new buffer to every buffer except the last one. @@ +1232,3 @@ hsize = MIN (nbytes, cur_size - skip); + if (skip == 0 && cur_size == hsize) { same here
(In reply to Olivier Crête from comment #9) > Review of attachment 307084 [details] [review] [review]: > > I think you wan to gst_buffer_ref the buffer ic cur_size <= to hsize, > otherwise it will create a new buffer to every buffer except the last one. I want to get ref-buffer only if current gst buffer size equuals hsize. Otherwise, it will call buffer_copy_region, yes, which is creating new gst buffer. I don't understand the case that it creates new buffers to all except the last one. If you help me understanding, I'll look into this carefully. :)
What Olivier means is that for all buffers except for the last and first you can just return a ref and don't need to copy. We will always take the full buffer for all those. Only the first and last might be partial and might require a gst_buffer_copy_region()
Created attachment 307192 [details] [review] adapter: change log message properly Oh. it's about wrong log message. I think it should be changed. Thanks for help, Oliver and Sebastian!
Actually it's not about the log message, just that Olivier and I were confused :) cur_size is the size of the current buffer, hsize the size of what we have to return from the current buffer. So hsize==cur_size whenever we take the complete buffer, also for middle buffers.