GNOME Bugzilla – Bug 665921
filesrc: Incorrect EOF detection in create_read
Last modified: 2011-12-13 11:26:10 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)
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.
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.
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).
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
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)
Oooh, shame on me. Fixing...
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).
I'll wait till the reported confirms this actually works when the fs causes short reads before pushing, so setting to NEEDINFO.
I'll apply the patch and see if the problem reoccurs.
> 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 ;-)
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. :)
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