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 599769 - [gstbasesrc] [gstfilesrc] : stat in get_size failure handling.
[gstbasesrc] [gstfilesrc] : stat in get_size failure handling.
Status: RESOLVED WONTFIX
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-10-27 14:29 UTC by Aurelien Grimaud
Modified: 2012-05-09 10:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gst_file_src_get_size modification (734 bytes, patch)
2009-10-27 14:31 UTC, Aurelien Grimaud
none Details | Review

Description Aurelien Grimaud 2009-10-27 14:29:34 UTC
I use gstreamer to play audio raw PCMA files through RTP.
Pipeline looks like filesrc -> capsfilter -> pcmapay -> rtp.
For performance concern, filesrc uses mmap (mmapsize=163840, blocksize=4000).
The problem being that using mmap may lead to bus error if file is removed/changed while buffer is still flowing ...

I made an ugly workaround to prevent mmap to lead to core dump.
The idea is to trap sigbus, and change the faulting address value to provide a buffer full of 0xd5 instead.

My test was based on a 21916 bytes file (2.8 secs), which is removed when being played.
A sigbus is raised into rtppcmapay (the adapter stores buffer which are after the delete invalid). I replace faulting buffer by my silence buffer and return from sighandler. Silence is played through RTP, everything is ok.
Or looks like. Instead of getting .5 sec silence (my 4000 bytes of block out of filesrc), I have 20.5 secs silence (beign the mmapsize length).

The explanation :
Basesrc is getting size from subclass (get_size / gst_file_src_get_size) which issue a stat and because file does no longer exist, fails and returns FALSE. 
Therefore gst_base_src_update_length compute a maxsize to (guint64) -1.
(offset >= maxsize) is false, (offset + *length >= maxsize) is false, length is though unchanged (previously set to mmapsize) and gst_base_src_update_length returns TRUE.
gst_basesrc_get_range then tries to create a buffer of 4000 bytes as if file was still there, and moreover, as if file was big enough which is not sure.

To change as little as possible, here is a patch which modify the get_size of filesrc. When stat fails, the size ist set to 0 and returns TRUE. on ESTALE (I use files accessed through NFS), the size is set to the previously size saved in basesrc->size. 

This fixed the issue.
When I remove the file, I have a sigbus but filesrc eos when reading next buffer, so I have .5 sec silence maximum.
Comment 1 Aurelien Grimaud 2009-10-27 14:31:02 UTC
Created attachment 146337 [details] [review]
gst_file_src_get_size modification
Comment 2 Wim Taymans 2009-10-27 15:28:22 UTC
can't it just always return 0 and TRUE to make basesrc stop as soon as possible?
Comment 3 Aurelien Grimaud 2009-10-27 17:02:31 UTC
ESTALE only means NFS server was too busy to fullfill the request, not that the file has changed, nor that mmap buffer is unreachable.
Is there a problem using errno ? ESTALE ? or basesrc->size ?
Comment 4 Aurelien Grimaud 2009-10-30 20:16:41 UTC
But It can just return 0 on ESTALE if you think it's better.
What I want most is filesrc to eos if file is removed or truncated to a smaller size that it was before. so returning 0 when stat failed whatever the reason was is ok.
I was just pointing ESTALE issues when working with NFS.
Comment 5 Tobias Mueller 2010-06-05 11:37:30 UTC
Wim, can you address questions Aurelien raised?
There's a patch lying around, too :-)
Comment 6 Tobias Mueller 2010-11-07 12:03:41 UTC
Reopening as I don't see any open non-developer question.
Comment 7 Stefan Sauer (gstreamer, gtkdoc dev) 2011-03-07 07:34:30 UTC
Wim, I don't see any bad side effects and the patch is also relative small.
Comment 8 Sebastian Dröge (slomo) 2011-05-23 13:06:47 UTC
Wouldn't it be better to just retry to get the size when it returned ESTALE?
Comment 9 Akhil Laddha 2011-07-07 05:18:47 UTC
Aurelien, do you agree with comment#8 ?
Comment 10 Aurelien Grimaud 2011-07-07 21:14:36 UTC
And set size to 0 and return TRUE if fstat fails again ?
I do not think, there is much chance fstat will succeed such a short time later.

Let's not handle the ESTALE case for the moment and just set size to 0 and return TRUE when fstat errors.

I will run more tests to figure out exactly when does the ESTALE situation occurs.
Comment 11 Akhil Laddha 2011-09-22 09:14:54 UTC
Aurelien, ping
Comment 12 Tim-Philipp Müller 2012-01-15 22:32:22 UTC
Why was this set back to UNCONFIRMED?

IMHO this is a WONTFIX, just don't use mmap() mode, it's disabled for a reason, and the performance gain is negligible in most cases, even more so with such tiny files (unless you are reading loads of them concurrently all the time, perhaps).
Comment 13 Tobias Mueller 2012-05-09 10:37:04 UTC
Closing a WONTFIX as per comment #12. Note that there is a patch lying around which probably wants to be set to rejected.