GNOME Bugzilla – Bug 412648
[filesink] reports wrong (byte) position after seeking
Last modified: 2007-05-22 15:46:19 UTC
filesink uses a (misnamed?) data_written field to report byte position. However, this variable is not kept properly in sync with the real position in case of seeking backward and forward again (which is a typical scenario following some muxer element).
Created attachment 83463 [details] [review] Possible patch * Fix position reporting to be seek-resistant * [performance] Do not perform a seek/tell before each fwrite
Created attachment 83464 [details] [review] Patch for similar problem with gnomevfssink (plugins-base) * Fix position reporting to be seek-resistant.
As I read it, the code is looking to delay changing the reported position until some data arrives and is written. The current code to do that is confusingly written, and might be wrong, but I'd like to know why it was written that way in the first place before we undo it. Does anyone know?
This seems to have been introduced by this commit: http://webcvs.freedesktop.org/gstreamer/gstreamer/plugins/elements/gstfilesink.c?r1=1.16&r2=1.17 with the commit message "Add support for not incrementing bytecounter while we're not at the end of the stream". Back then, data_written was used to report TOTAL LENGTH of the current file, and not the position, see: http://webcvs.freedesktop.org/gstreamer/gstreamer/plugins/elements/gstfilesink.c?annotate=1.17 So the way I read it is that this code was added to handle the case where the current position is N bytes before the end of the file and the amount of data to be written is X (with X >= N). In this case, data_written will be updated correctly to reflect the new total size, if I'm not mistaken. However, I think this fails for the case where X < N, in this case data_written will wrongly be decremented even though it should stay the same. In short: I think the patch makes sense and should be applied, although it'd probably a good idea to rename the data_written member to current_position or such as well.
Thanks committed (also changed gnomevfssink to update the position variable immediately after a successful seek so behaviour is consistent with filesink): 2007-05-22 Tim-Philipp Müller <tim at centricular dot net> * plugins/elements/gstelements.c: * plugins/elements/gstfilesink.c: (gst_file_sink_do_seek), (gst_file_sink_get_current_offset): * plugins/indexers/gstindexers.c: (plugin_init): Use #ifdef for HAVE_XYZ for consistency. * tests/check/Makefile.am: * tests/check/elements/.cvsignore: * tests/check/elements/filesink.c: (setup_filesink), (cleanup_filesink), (GST_START_TEST), (filesink_suite): Add some unit tests for filesink. 2007-05-22 Tim-Philipp Müller <tim at centricular dot net> Patch by: Mark Nauwelaerts <manauw at skynet be> * plugins/elements/gstfilesink.c: (gst_file_sink_open_file), (gst_file_sink_query), (gst_file_sink_do_seek), (gst_file_sink_get_current_offset), (gst_file_sink_render): * plugins/elements/gstfilesink.h: Fix position reporting; rename data_written member to current_pos to reflect its real meaning (fixes #412648). 2007-05-22 Tim-Philipp Müller <tim at centricular dot net> Patch by: Mark Nauwelaerts <manauw at skynet be> * ext/gnomevfs/gstgnomevfssink.c: (gst_gnome_vfs_sink_init), (gst_gnome_vfs_sink_open_file), (gst_gnome_vfs_sink_handle_event), (gst_gnome_vfs_sink_query), (gst_gnome_vfs_sink_render): * ext/gnomevfs/gstgnomevfssink.h: Fix position reporting, especially after a seek (from upstream), see #412648. 2007-05-22 Tim-Philipp Müller <tim at centricular dot net> * tests/check/Makefile.am: * tests/check/elements/.cvsignore: * tests/check/elements/gnomevfssink.c: (setup_gnomevfssink), (cleanup_gnomevfssink), (GST_START_TEST), (gnomevfssink_suite): Add unit test for gnomevfssink seeking and position reporting for file:// URIs.