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 628337 - [gnomevfssrc] Add support for cancelling read operations
[gnomevfssrc] Add support for cancelling read operations
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
0.10.30
Other Windows
: Normal normal
: 0.10.36
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-08-30 15:15 UTC by American Dynamics
Modified: 2011-06-15 17:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for adding cancelable infrastructure (4.02 KB, patch)
2010-08-30 15:15 UTC, American Dynamics
none Details | Review
Patch for adding cancelable infrastructure (389 bytes, patch)
2010-08-30 15:16 UTC, American Dynamics
none Details | Review
Patch to support canceling events generated via git format-patch (5.47 KB, patch)
2010-09-03 13:20 UTC, American Dynamics
committed Details | Review

Description American Dynamics 2010-08-30 15:15:54 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.
Comment 1 American Dynamics 2010-08-30 15:16:21 UTC
Created attachment 169094 [details] [review]
Patch for adding cancelable infrastructure
Comment 2 Sebastian Dröge (slomo) 2010-08-30 15:21:26 UTC
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
Comment 3 American Dynamics 2010-08-30 15:40:08 UTC
(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).
Comment 4 Tim-Philipp Müller 2010-08-30 17:25:42 UTC
> 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 5 American Dynamics 2010-08-30 17:48:26 UTC
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;
> }
Comment 6 American Dynamics 2010-08-30 17:50:57 UTC
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;
> > }
Comment 7 American Dynamics 2010-09-03 13:20:44 UTC
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
Comment 8 Sebastian Dröge (slomo) 2011-05-26 08:33:30 UTC
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.