GNOME Bugzilla – Bug 628337
[gnomevfssrc] Add support for cancelling read operations
Last modified: 2011-06-15 17:42:43 UTC
Created attachment 169093 [details] [review] Patch for adding cancelable infrastructure We had to raise the thread priority in our streaming thread (which contained a pipeline and this element) that had an adverse affect on the gstgnomevfssrc element. Basically what was happening is there was a lock starvation in this element due to the fact that we had raised the priority of the streaming thread. To get around this problem we developed some code to support cancellable reads.
Created attachment 169094 [details] [review] Patch for adding cancelable infrastructure
Thanks for your patch but please attach it in git format-patch format. Also, you shouldn't really use gnomevfs anymore, it's completely unmaintained. GIO is the new replacement for it. The patch looks good from a first look though. But if the read is cancelled you should return WRONG_STATE instead of UNEXPECTED
(In reply to comment #2) > Thanks for your patch but please attach it in git format-patch format. Also, > you shouldn't really use gnomevfs anymore, it's completely unmaintained. GIO is > the new replacement for it. > > The patch looks good from a first look though. But if the read is cancelled you > should return WRONG_STATE instead of UNEXPECTED Hi Sebastian, Thanks for the info. I will change the patch to return GST_FLOW_WRONG_STATE instead of GST_FLOW_UNEXPECTED. On that vain, I can do the git format-patch format but I'm not sure what command line args you would like me to include (how you would like patch formatted).
> I can do the git format-patch > format but I'm not sure what command line args you would like me to include > (how you would like patch formatted). See http://gstreamer.freedesktop.org/wiki/SubmittingPatches ("Create a diff from a git commit")
Comment on attachment 169093 [details] [review] Patch for adding cancelable infrastructure >--- gstgnomevfssrc.c 2010-07-04 10:37:40.000000000 -0400 >+++ gstgnomevfssrc.c 2010-08-26 10:21:41.000000000 -0400 >@@ -70,6 +70,7 @@ > #include "gst/gst-i18n-plugin.h" > > #include "gstgnomevfssrc.h" >+#include <gnome-vfs-module-2.0/libgnomevfs/gnome-vfs-cancellable-ops.h> > > #include <stdio.h> > #include <stdlib.h> >@@ -132,6 +133,8 @@ > static gboolean gst_gnome_vfs_src_start (GstBaseSrc * src); > static gboolean gst_gnome_vfs_src_is_seekable (GstBaseSrc * src); > static gboolean gst_gnome_vfs_src_check_get_range (GstBaseSrc * src); >+static gboolean gst_gnome_vfs_src_unlock (GstBaseSrc * basesrc); >+static gboolean gst_gnome_vfs_src_unlock_stop (GstBaseSrc * basesrc); > static gboolean gst_gnome_vfs_src_get_size (GstBaseSrc * src, guint64 * size); > static GstFlowReturn gst_gnome_vfs_src_create (GstBaseSrc * basesrc, > guint64 offset, guint size, GstBuffer ** buffer); >@@ -240,6 +243,9 @@ > > gstbasesrc_class->start = GST_DEBUG_FUNCPTR (gst_gnome_vfs_src_start); > gstbasesrc_class->stop = GST_DEBUG_FUNCPTR (gst_gnome_vfs_src_stop); >+ gstbasesrc_class->unlock = GST_DEBUG_FUNCPTR (gst_gnome_vfs_src_unlock); >+ gstbasesrc_class->unlock_stop = >+ GST_DEBUG_FUNCPTR (gst_gnome_vfs_src_unlock_stop); > gstbasesrc_class->get_size = GST_DEBUG_FUNCPTR (gst_gnome_vfs_src_get_size); > gstbasesrc_class->is_seekable = > GST_DEBUG_FUNCPTR (gst_gnome_vfs_src_is_seekable); >@@ -254,7 +260,9 @@ > { > gnomevfssrc->uri = NULL; > gnomevfssrc->uri_name = NULL; >+ gnomevfssrc->context = NULL; > gnomevfssrc->handle = NULL; >+ gnomevfssrc->interrupted = FALSE; > gnomevfssrc->curoffset = 0; > gnomevfssrc->seekable = FALSE; > >@@ -632,9 +640,19 @@ > data = GST_BUFFER_DATA (buf); > > todo = size; >- while (todo > 0) { >+ while (!src->interrupted && todo > 0) { > /* this can return less that we ask for */ >- res = gnome_vfs_read (src->handle, data, todo, &readbytes); >+ res = gnome_vfs_read_cancellable (src->handle, data, todo, &readbytes, src->context); >+ >+ if (G_UNLIKELY (res == GNOME_VFS_ERROR_CANCELLED)) { >+ GST_DEBUG_OBJECT (src, "interrupted"); >+ >+ /* Just take what we've so far gotten and return */ >+ size = size - todo; >+ GST_BUFFER_SIZE (buf) = size; >+ todo = 0; >+ break; >+ } > > if (G_UNLIKELY (res == GNOME_VFS_ERROR_EOF || (res == GNOME_VFS_OK > && readbytes == 0))) >@@ -651,6 +669,11 @@ > } > GST_LOG (" got size %" G_GUINT64_FORMAT, readbytes); > } >+ >+ /* Handle interrupts implicitly, since the flag may have been cleared by now */ >+ if (size == todo) >+ goto interrupted; >+ > GST_BUFFER_OFFSET (buf) = src->curoffset; > src->curoffset += size; > >@@ -680,6 +703,11 @@ > ("Failed to read data: %s", gnome_vfs_result_to_string (res))); > return GST_FLOW_WRONG_STATE; > } >+interrupted: >+ { >+ gst_buffer_unref (buf); >+ return GST_FLOW_UNEXPECTED; >+ } > eos: > { > gst_buffer_unref (buf); >@@ -765,6 +793,38 @@ > } > } > >+/* Interrupt a blocking request. */ >+static gboolean >+gst_gnome_vfs_src_unlock (GstBaseSrc * basesrc) >+{ >+ GstGnomeVFSSrc *src; >+ >+ src = GST_GNOME_VFS_SRC (basesrc); >+ GST_DEBUG_OBJECT (src, "unlock()"); >+ src->interrupted = TRUE; >+ if (src->context) >+ { >+ GnomeVFSCancellation *cancel = >+ gnome_vfs_context_get_cancellation (src->context); >+ if (cancel) >+ gnome_vfs_cancellation_cancel (cancel); >+ } >+ return TRUE; >+} >+ >+/* Interrupt interrupt. */ >+static gboolean >+gst_gnome_vfs_src_unlock_stop (GstBaseSrc * basesrc) >+{ >+ GstGnomeVFSSrc *src; >+ >+ src = GST_GNOME_VFS_SRC (basesrc); >+ GST_DEBUG_OBJECT (src, "unlock_stop()"); >+ >+ src->interrupted = FALSE; >+ return TRUE; >+} >+ > static gboolean > gst_gnome_vfs_src_get_size (GstBaseSrc * basesrc, guint64 * size) > { >@@ -818,6 +878,7 @@ > > gst_gnome_vfs_src_push_callbacks (src); > >+ src->context = gnome_vfs_context_new(); > if (src->uri != NULL) { > GnomeVFSOpenMode mode = GNOME_VFS_OPEN_READ; > >@@ -889,6 +950,9 @@ > src->handle = NULL; > } > src->curoffset = 0; >+ src->interrupted = FALSE; >+ gnome_vfs_context_free( src->context ); >+ src->context = NULL; > > return TRUE; > }
PLEASE DISREGARD THIS COMMENT! (In reply to comment #5) > (From update of attachment 169093 [details] [review]) > >--- gstgnomevfssrc.c 2010-07-04 10:37:40.000000000 -0400 > >+++ gstgnomevfssrc.c 2010-08-26 10:21:41.000000000 -0400 > >@@ -70,6 +70,7 @@ > > #include "gst/gst-i18n-plugin.h" > > > > #include "gstgnomevfssrc.h" > >+#include <gnome-vfs-module-2.0/libgnomevfs/gnome-vfs-cancellable-ops.h> > > > > #include <stdio.h> > > #include <stdlib.h> > >@@ -132,6 +133,8 @@ > > static gboolean gst_gnome_vfs_src_start (GstBaseSrc * src); > > static gboolean gst_gnome_vfs_src_is_seekable (GstBaseSrc * src); > > static gboolean gst_gnome_vfs_src_check_get_range (GstBaseSrc * src); > >+static gboolean gst_gnome_vfs_src_unlock (GstBaseSrc * basesrc); > >+static gboolean gst_gnome_vfs_src_unlock_stop (GstBaseSrc * basesrc); > > static gboolean gst_gnome_vfs_src_get_size (GstBaseSrc * src, guint64 * size); > > static GstFlowReturn gst_gnome_vfs_src_create (GstBaseSrc * basesrc, > > guint64 offset, guint size, GstBuffer ** buffer); > >@@ -240,6 +243,9 @@ > > > > gstbasesrc_class->start = GST_DEBUG_FUNCPTR (gst_gnome_vfs_src_start); > > gstbasesrc_class->stop = GST_DEBUG_FUNCPTR (gst_gnome_vfs_src_stop); > >+ gstbasesrc_class->unlock = GST_DEBUG_FUNCPTR (gst_gnome_vfs_src_unlock); > >+ gstbasesrc_class->unlock_stop = > >+ GST_DEBUG_FUNCPTR (gst_gnome_vfs_src_unlock_stop); > > gstbasesrc_class->get_size = GST_DEBUG_FUNCPTR (gst_gnome_vfs_src_get_size); > > gstbasesrc_class->is_seekable = > > GST_DEBUG_FUNCPTR (gst_gnome_vfs_src_is_seekable); > >@@ -254,7 +260,9 @@ > > { > > gnomevfssrc->uri = NULL; > > gnomevfssrc->uri_name = NULL; > >+ gnomevfssrc->context = NULL; > > gnomevfssrc->handle = NULL; > >+ gnomevfssrc->interrupted = FALSE; > > gnomevfssrc->curoffset = 0; > > gnomevfssrc->seekable = FALSE; > > > >@@ -632,9 +640,19 @@ > > data = GST_BUFFER_DATA (buf); > > > > todo = size; > >- while (todo > 0) { > >+ while (!src->interrupted && todo > 0) { > > /* this can return less that we ask for */ > >- res = gnome_vfs_read (src->handle, data, todo, &readbytes); > >+ res = gnome_vfs_read_cancellable (src->handle, data, todo, &readbytes, src->context); > >+ > >+ if (G_UNLIKELY (res == GNOME_VFS_ERROR_CANCELLED)) { > >+ GST_DEBUG_OBJECT (src, "interrupted"); > >+ > >+ /* Just take what we've so far gotten and return */ > >+ size = size - todo; > >+ GST_BUFFER_SIZE (buf) = size; > >+ todo = 0; > >+ break; > >+ } > > > > if (G_UNLIKELY (res == GNOME_VFS_ERROR_EOF || (res == GNOME_VFS_OK > > && readbytes == 0))) > >@@ -651,6 +669,11 @@ > > } > > GST_LOG (" got size %" G_GUINT64_FORMAT, readbytes); > > } > >+ > >+ /* Handle interrupts implicitly, since the flag may have been cleared by now */ > >+ if (size == todo) > >+ goto interrupted; > >+ > > GST_BUFFER_OFFSET (buf) = src->curoffset; > > src->curoffset += size; > > > >@@ -680,6 +703,11 @@ > > ("Failed to read data: %s", gnome_vfs_result_to_string (res))); > > return GST_FLOW_WRONG_STATE; > > } > >+interrupted: > >+ { > >+ gst_buffer_unref (buf); > >+ return GST_FLOW_UNEXPECTED; > >+ } > > eos: > > { > > gst_buffer_unref (buf); > >@@ -765,6 +793,38 @@ > > } > > } > > > >+/* Interrupt a blocking request. */ > >+static gboolean > >+gst_gnome_vfs_src_unlock (GstBaseSrc * basesrc) > >+{ > >+ GstGnomeVFSSrc *src; > >+ > >+ src = GST_GNOME_VFS_SRC (basesrc); > >+ GST_DEBUG_OBJECT (src, "unlock()"); > >+ src->interrupted = TRUE; > >+ if (src->context) > >+ { > >+ GnomeVFSCancellation *cancel = > >+ gnome_vfs_context_get_cancellation (src->context); > >+ if (cancel) > >+ gnome_vfs_cancellation_cancel (cancel); > >+ } > >+ return TRUE; > >+} > >+ > >+/* Interrupt interrupt. */ > >+static gboolean > >+gst_gnome_vfs_src_unlock_stop (GstBaseSrc * basesrc) > >+{ > >+ GstGnomeVFSSrc *src; > >+ > >+ src = GST_GNOME_VFS_SRC (basesrc); > >+ GST_DEBUG_OBJECT (src, "unlock_stop()"); > >+ > >+ src->interrupted = FALSE; > >+ return TRUE; > >+} > >+ > > static gboolean > > gst_gnome_vfs_src_get_size (GstBaseSrc * basesrc, guint64 * size) > > { > >@@ -818,6 +878,7 @@ > > > > gst_gnome_vfs_src_push_callbacks (src); > > > >+ src->context = gnome_vfs_context_new(); > > if (src->uri != NULL) { > > GnomeVFSOpenMode mode = GNOME_VFS_OPEN_READ; > > > >@@ -889,6 +950,9 @@ > > src->handle = NULL; > > } > > src->curoffset = 0; > >+ src->interrupted = FALSE; > >+ gnome_vfs_context_free( src->context ); > >+ src->context = NULL; > > > > return TRUE; > > }
Created attachment 169435 [details] [review] Patch to support canceling events generated via git format-patch gstreamer folks: Please take a look at this patch I've generated with git format-patch and please let me know if this is okay or if there's still something I need to correctly. Thanks
commit 99188bce77ffa9958480410e87a1b52e1b32ddab Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Thu May 26 10:31:11 2011 +0200 gnomevfssrc: Keep track of interruptions during read with a flag commit 847d274a5c4c2fb7d5f636bdf91bf65fab3db142 Author: American Dynamics <GStreamer-Bugs@tycosp.com> Date: Fri Sep 3 09:11:30 2010 -0400 gnomevfssrc: Add support for cancelling the read operations This allows the state change from PAUSED to READY to be faster. Fixes bug #628337.