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 665921 - filesrc: Incorrect EOF detection in create_read
filesrc: Incorrect EOF detection in create_read
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
0.10.35
Other Linux
: Normal normal
: 0.10.36
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-12-10 19:07 UTC by Marshall Pierce
Modified: 2011-12-13 11:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
filesrc: do not mistake short reads for EOS (2.86 KB, patch)
2011-12-12 13:09 UTC, Vincent Penquerc'h
needs-work Details | Review
filesrc: do not mistake short reads for EOS (2.92 KB, patch)
2011-12-12 13:37 UTC, Vincent Penquerc'h
committed Details | Review

Description Marshall Pierce 2011-12-10 19:07:01 UTC
I'm seeing occasional issues playing files on a CIFS mountpoint in Clementine. https://code.google.com/p/clementine-player/issues/detail?id=2469 is the Clementine bug I filed. The log lines there indicate that gstreamer is getting "unexpected end of file."

The logging made it easy to find gst_file_src_create_read in gstfilesrc.c

Here's an abbreviated version of the function that shows what I think is the incorrect code path.

ret = read (src->fd, GST_BUFFER_DATA (buf), length);
...
/* seekable regular files should have given us what we expected */
if (G_UNLIKELY ((guint) ret < length && src->seekable))
  goto unexpected_eos;
...
unexpected_eos:
  {
    GST_ELEMENT_ERROR (src, RESOURCE, READ, (NULL),
        ("unexpected end of file."));
    gst_buffer_unref (buf);
    return GST_FLOW_ERROR;
  }


Saying that (ret < length && src->seekable) is an error seems contrary to the man page of the read(2) syscall:

RETURN VALUE
(snip) It is not an error if this number is smaller than the number of bytes requested (snip)
Comment 1 Vincent Penquerc'h 2011-12-12 12:16:46 UTC
The code after your quote seems to do the right thing wrt testing for EOS (ie, testing for 0 instead of short reads), so it would seem to be enough to just remove that particular test, though if callers rely on 'length' more bytes being available, one might have to loop instead, to return with either a real EOS plus a short read, or the requested amount of data.
Comment 2 Sebastian Dröge (slomo) 2011-12-12 12:33:25 UTC
gst_pad_pull_range() is supposed to return the requested number of bytes and short reads are only allowed at the end of the stream. Looping has to be done in filesrc.
Comment 3 Vincent Penquerc'h 2011-12-12 13:09:27 UTC
Created attachment 203247 [details] [review]
filesrc: do not mistake short reads for EOS

While local filesystems will usually not cause short reads,
this may happen on seekable files on some remote filesystems.
Instead, loop till we get the requested amount of data, or
an actual EOS (ie, 0 bytes).
Comment 4 Vincent Penquerc'h 2011-12-12 13:11:10 UTC
That works for me, but I don't have remote filesystems to test with.
I also added retries on EAGAIN/EINTR, not 100% sure it could not end up with an infinite loop though. Any experts on EAGAIN/EINTR ? :D
Comment 5 Sebastian Dröge (slomo) 2011-12-12 13:29:36 UTC
Review of attachment 203247 [details] [review]:

Looks good but:

::: plugins/elements/gstfilesrc.c
@@ +828,3 @@
+        length, offset + woffset);
+    ret = read (src->fd, GST_BUFFER_DATA (buf) + woffset, length);
+    if (G_UNLIKELY (ret < 0 && ret != -EAGAIN && ret != -EINTR))

read() returns -1 on error and you have to check errno for EAGAIN and EINTR (not -EAGAIN and -EINTR)
Comment 6 Vincent Penquerc'h 2011-12-12 13:33:41 UTC
Oooh, shame on me. Fixing...
Comment 7 Vincent Penquerc'h 2011-12-12 13:37:45 UTC
Created attachment 203250 [details] [review]
filesrc: do not mistake short reads for EOS

While local filesystems will usually not cause short reads,
this may happen on seekable files on some remote filesystems.
Instead, loop till we get the requested amount of data, or
an actual EOS (ie, 0 bytes).
Comment 8 Vincent Penquerc'h 2011-12-12 14:15:04 UTC
I'll wait till the reported confirms this actually works when the fs causes short reads before pushing, so setting to NEEDINFO.
Comment 9 Marshall Pierce 2011-12-12 17:03:56 UTC
I'll apply the patch and see if the problem reoccurs.
Comment 10 Tim-Philipp Müller 2011-12-13 00:43:01 UTC
> I also added retries on EAGAIN/EINTR, not 100% sure it could not end up with an
> infinite loop though.

Could attempt to make it interruptible by implementing GstBaseSrcClass::unlock() (simple atomic boolean/int should be enough). But then it's not that a file read() is interruptable either ;-)
Comment 11 Marshall Pierce 2011-12-13 07:19:57 UTC
I left Clementine playing all day and it hadn't had any issues after ~10 hours. You can't prove a negative, of course, but that's at least better than it usually does. :)
Comment 12 Vincent Penquerc'h 2011-12-13 11:25:57 UTC
Thanks for checking.

commit 431fc714c927381e359c07a8599b8b2bc301f415
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Mon Dec 12 13:05:36 2011 +0000

    filesrc: do not mistake short reads for EOS
    
    While local filesystems will usually not cause short reads,
    this may happen on seekable files on some remote filesystems.
    Instead, loop till we get the requested amount of data, or
    an actual EOS (ie, 0 bytes).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=665921