GNOME Bugzilla – Bug 698562
rtpbuffer: broken language bindings for gst_rtp_buffer_get_payload
Last modified: 2013-07-10 07:22:02 UTC
When calling the function gst_rtp_buffer_get_payload() from Python, i.e. GstRtp.RTPBuffer.get_payload() the program segfaults. This is caused by the function returning a byte array, but it lacks a length parameter to store the returned array's length. This makes it impossible for the Python bindings to work. Attached is a patch that remedies this issue. Since the GstRtpBuffer API is already released we can not simply change the function. Instead I have added a new function gst_rtp_buffer_get_payload_data() with the proper parameters and annotation. Also added a 'Rename to:' annotation in order to replace the original gst_rtp_buffer_get_payload() function with the new gst_rtp_buffer_get_payload_data() when creating bindings. There is also an error in the annotations for the function gst_rtp_buffer_get_extension_data(). This function does have an output parameter for the length of the array, but it's expressed in number of 32-bit words. Again, this is a released API and can not simply be changed. Thus I've added a a new function that basically wraps the this one, but returns the length in terms of bytes.
Created attachment 242130 [details] [review] New function to use instead of gst_rtp_buffer_get_payload in language bindings.
Ok, some nitpicks, and some things I'm not sure about yet: - please add 'Since: 1.2' markers for new API - the old functions that we're replacing via Rename-to should probably be marked with (skip) or so - if these functions are for bindings, it might be better to just return const guint8 * or guint8 * directly instead of gpointer plus annotating the pointer - the out length argument should probably be gsize now instead of guint. We haven't changed it everywhere, but we may just as well do it right in new API even if it doesn't really make a difference. - this whole 'the returned memory is valid as long as the object is valid' thing does not work too well for bindings, I don't think we can express that. I believe bindings will work around that by just making a copy of the bytes in any case. The main implication here is that this works fine for reading, but will very likely not work for writing at all (since the bindings will modified the copied buffer and not the original one). I'm guessing you are only reading data here?
Might be a use-case for GBytes ? Although you have the problem that there is no way to invalidate it after a unmap.. You may want to do a _extract() instead that does a memcpy
I don't know if GBytes solves the problem with writing directly to the original memory for bindings - does it? (I'm sure the answer is somewhere in bug #678663, but I'm too lazy to check now).
GBytes is immutable, yes
It seems as if using GBytes is not the way to go, right? Given the problem of invalidating it after the memory has been unmapped. And if the bindings makes a copy of the original data anyway then it's not a big difference compared to adding an _extract() function. But adding an extract function might make it clearer that it's an actual copy, and it would be similar to the gst_buffer_extract_dup() function. So, which way to go?
Tim: yes, we are only reading data.
Created attachment 244133 [details] [review] New patch, same approach as before but modified according to comments
Comment on attachment 244133 [details] [review] New patch, same approach as before but modified according to comments This doesn't even compile... Please also add the bug number or bug URL at the end of the commit message.
Created attachment 244236 [details] [review] New, compilable, patch. Same approach as before but modified according to comments Tim: Truly sorry about the previous patch. Last minute change of a parameter name, without recompiling it. Unfortunately I missed one occurrence. No last minute changes without compiling and testing this time. While fixing it I rewrote the gst_rtp_buffer_get_extension_bytes() function a little.
I think GBytes should be used here, as that's apparently what the g-i people recommend to use in such cases.
Created attachment 246771 [details] [review] New attmpt, this time using GBytes
Review of attachment 246771 [details] [review]: ::: gst-libs/gst/rtp/gstrtpbuffer.c @@ +713,3 @@ +gboolean +gst_rtp_buffer_get_extension_bytes (GstRTPBuffer * rtp, guint16 * bits, + GBytes ** data) I think this should just return a GBytes instead of having the out parameter @@ +727,3 @@ + *data = g_bytes_new (buf_data, 4 * buf_len); + } else { + *data = NULL; And in the buf_len==0 case just return an empty GBytes
Created attachment 246982 [details] [review] New (last?) attempt using GBytes. Modified according to comments.
commit 4dd5c5b808d1618efbe0364b80c04fb2ca23d65a Author: Branko Subasic <branko@axis.com> Date: Sun Jun 16 22:39:30 2013 +0200 rtpbuffer: add gst_rtp_buffer_get_payload_bytes The function gst_rtp_buffer_get_payload can not be used in Python because it lacks necessary length parameter. This patch adds a new function, gst_rtp_buffer_get_payload_bytes, to use from Python bindings. The new function has the advisory "Rename to:" annotation so it can replace the gst_rtp_buffer_get_payload whan creating bindings. The function gst_rtp_buffer_get_extension_bytes is also added. It wraps gst_rtp_buffer_get_extension_data which doesn't work in Python due to incomplete annotation and because it returns the length as number of 32-bit words. https://bugzilla.gnome.org/show_bug.cgi?id=698562
Must add the functions to win32/common/libgstrtp.def.
Created attachment 248775 [details] [review] Add new functions to win32/common/libgstrtp.def
commit 71fa8f945bdabd6dbcf91886cdc0adebaa429ed4 Author: Branko Subasic <branko@axis.com> Date: Tue Jul 9 23:04:49 2013 +0200 win32: add missing rtp buffer methods