GNOME Bugzilla – Bug 735861
dataurisrc: make src thread safe
Last modified: 2014-09-12 12:25:09 UTC
In gst_data_uri_src_get_uri(), src is not thread safe. Added extra conditions to make it thread safe. Please review. And i have one doubt, in gst_data_uri_src_create() function, there is a condition else if (*buf != NULL) { which will be used when buffer is already provided. But is there any scenario where buffer will be provided as input? if buffer is provided, then dont we need to check the size of the buffer allocated with the size of source buffer? I was thinking, that else if condition should be executed only when size of provided buffer will be same as size of source buffer.. or we can remove else if condition and something like else { if (*buf != NULL) gst_buffer_unref(*buf); *buf = gst_buffer_copy_region (src->buffer, GST_BUFFER_COPY_ALL, offset, size); ret = GST_FLOW_OK; } Please correct me if i am wrong
Created attachment 285112 [details] [review] proposed patch
No, it can't be != NULL there anywhere. basesrc allows that but gst_pad_get_range() does not. Feel free to attach a new patch for that to this bug report. commit 76e099e5b91bd98ad68c4461d138369351110f6a Author: Vineeth T M <vineeth.tm@samsung.com> Date: Tue Sep 2 12:11:44 2014 +0530 dataurisrc: Make get_uri() threadsafe https://bugzilla.gnome.org/show_bug.cgi?id=735861
Created attachment 285116 [details] [review] Attaching proposed patch Removing the else if condition as it will be invalid. But just to be on safer side, unreferencing the buf in else condition. Please review.
Review of attachment 285116 [details] [review]: ::: gst/dataurisrc/gstdataurisrc.c @@ +232,3 @@ + * just in case a buffer is provided*/ + if (*buf != NULL) + gst_buffer_unref (*buf); Add a GST_FIXME_OBJECT() for this case. It should be fixed once the assumptions are not true anymore
Created attachment 285316 [details] [review] Adding GST_FIXME_OBJECT
commit 3024ae9c38490817a76c83feab3c8472989cafad Author: Vineeth T M <vineeth.tm@samsung.com> Date: Thu Sep 4 13:38:21 2014 +0530 dataurisrc: Remove unnecessary else if condition In gst_data_uri_src_create(), buf cannot be NULL, hence else if (*buf != NULL) will be invalid so removing the else if condition and adding a check to unreference buf in else condition, just in case https://bugzilla.gnome.org/show_bug.cgi?id=735861
Just for the record, commit e03e6c157152a488eb412ec425fc49f32b55a5d8 was reverted as it is allowed to have *buf == NULL
Hi Thiago, Even in case *buf is provided, doesnt it make sense that we unref and create a new buffer with the given size? In case the buf being provided is not big enough to hold the size being copied to it in else if condition, or are we sure that the buf size and the offset being passed will always be proper? Regards, Vineeth
Now we are: commit 126c511e6273c3dea7494d81ed044414a3e355ba Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Sep 12 15:22:19 2014 +0300 pad: Make sure the buffer to get/pull_range() has at least the requested size https://bugzilla.gnome.org/show_bug.cgi?id=735861 The size can still be bigger though, but that's allowed by the docs.