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 589849 - [segment] Clipping fails to handle start=stop<segment_start correctly
[segment] Clipping fails to handle start=stop<segment_start correctly
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
0.10.24
Other Linux
: Normal blocker
: 0.10.25
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-07-27 10:24 UTC by Damien Lespiau
Modified: 2009-08-11 11:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
baseaudiosink-render-empty-buffer.diff (602 bytes, patch)
2009-08-11 10:30 UTC, Sebastian Dröge (slomo)
none Details | Review
gstsegment-clip.diff (499 bytes, patch)
2009-08-11 10:56 UTC, Sebastian Dröge (slomo)
none Details | Review

Description Damien Lespiau 2009-07-27 10:24:25 UTC
I'm playing a theora/flac video, and doing a seek to the end of the file with: 

gst_element_seek (priv->playbin,
		    1.0,
		    GST_FORMAT_TIME,
		    GST_SEEK_FLAG_FLUSH,
		    GST_SEEK_TYPE_SET,
		    position,
		    0, 0);

where position is the duration of the video.

* This works with the trio gstreamer git/gst-plugins-base git/gst-plugins-good 0.10.14
* It segfaults with the trio gstreamer git/gst-plugins-base git/gst-plugins-good 0.10.15
* It segfaults with the trio gstreamer git/gst-plugins-base git/gst-plugins-good git

So, if I haven't mix things with the git checkouts and make installs it means the regression is due to a change in pulsesink, introduced between 0.10.14 and 0.10.15. (yes, ready for a git bisect, will do it later)

The segfault has the following backtrace.

Program received signal SIGSEGV, Segmentation fault.

Thread 2989403024 (LWP 865)

  • #0 memcpy
    at ../sysdeps/i386/i586/memcpy.S line 118
  • #1 gst_pulseringbuffer_commit
    at pulsesink.c line 1232
  • #2 gst_ring_buffer_commit_full
    at gstringbuffer.c line 1727
  • #3 gst_base_audio_sink_render
    at gstbaseaudiosink.c line 1495
  • #4 gst_base_sink_render_object
    at gstbasesink.c line 2711
  • #5 gst_base_sink_queue_object_unlocked
    at gstbasesink.c line 2952
  • #6 gst_base_sink_chain_unlocked
    at gstbasesink.c line 3322
  • #7 gst_base_sink_chain_main
    at gstbasesink.c line 3360
  • #8 gst_pad_chain_data_unchecked
    at gstpad.c line 4057
  • #9 gst_pad_push_data
    at gstpad.c line 4287
  • #10 gst_proxy_pad_do_chain
    at gstghostpad.c line 179
  • #11 gst_pad_chain_data_unchecked
    at gstpad.c line 4057
  • #12 gst_pad_push_data
    at gstpad.c line 4287
  • #13 gst_proxy_pad_do_chain
    at gstghostpad.c line 179
  • #14 gst_pad_chain_data_unchecked
    at gstpad.c line 4057
  • #15 gst_pad_push_data
    at gstpad.c line 4287
  • #16 gst_proxy_pad_do_chain
    at gstghostpad.c line 179
  • #17 gst_pad_chain_data_unchecked
    at gstpad.c line 4057
  • #18 gst_pad_push_data
    at gstpad.c line 4287
  • #19 gst_base_transform_chain
    at gstbasetransform.c line 2039
  • #20 gst_pad_chain_data_unchecked
    at gstpad.c line 4057
  • #21 gst_pad_push_data
    at gstpad.c line 4287
  • #22 gst_base_transform_chain
    at gstbasetransform.c line 2039
  • #23 gst_pad_chain_data_unchecked
    at gstpad.c line 4057
  • #24 gst_pad_push_data
    at gstpad.c line 4287
  • #25 gst_base_transform_chain
    at gstbasetransform.c line 2039
  • #26 gst_pad_chain_data_unchecked
    at gstpad.c line 4057
  • #27 gst_pad_push_data
    at gstpad.c line 4287
  • #28 gst_proxy_pad_do_chain
    at gstghostpad.c line 179
  • #29 gst_pad_chain_data_unchecked
    at gstpad.c line 4057
  • #30 gst_pad_push_data
    at gstpad.c line 4287
  • #31 gst_queue_push_one
    at gstqueue.c line 1047
  • #32 gst_queue_loop
    at gstqueue.c line 1149
  • #33 gst_task_func
    at gsttask.c line 234
  • #34 default_func
    at gsttaskpool.c line 70
  • #35 g_thread_pool_thread_proxy
    at gthreadpool.c line 265
  • #36 g_thread_create_proxy
    at gthread.c line 635
  • #37 start_thread
    at pthread_create.c line 297
  • #38 clone
    at ../sysdeps/unix/sysv/linux/i386/clone.S line 130
  • #0 memcpy
    at ../sysdeps/i386/i586/memcpy.S line 118
  • #1 gst_pulseringbuffer_commit
    at pulsesink.c line 1232
  • #2 gst_ring_buffer_commit_full
    at gstringbuffer.c line 1727
  • #3 gst_base_audio_sink_render
    at gstbaseaudiosink.c line 1495
  • #4 gst_base_sink_render_object
    at gstbasesink.c line 2711

Comment 1 Damien Lespiau 2009-07-27 17:45:48 UTC
Interestingly git bisect points to a change in flacdec:

commit a7902054bdbe4229a50a5d73843526fc21dc0c2f
Author: Thomas Vander Stichele <thomas (at) apestaart (dot) org>
Date:   Sat Feb 21 12:47:00 2009 +0100

    respect DEFAULT segment by clipping the last buffer to be sent

and indeed commenting the change introduced in that commit "fixes" the segfault in pulsesink.

so gstreamer git / gst-plugins-base git / gst-plugins-good git + a7902054bdbe4229a50a5d73843526fc21dc0c2f works-for-me
Comment 2 Damien Lespiau 2009-07-27 17:46:45 UTC
I meant:

so gstreamer git / gst-plugins-base git / gst-plugins-good git +
a7902054bdbe4229a50a5d73843526fc21dc0c2f *reverted* works-for-me
Comment 3 Sebastian Dröge (slomo) 2009-07-28 20:05:27 UTC
Any idea why? Also flacdec should use gst_audio_buffer_clip() which handles the clipping for TIME and DEFAULT format and also before the beginning of the segment.

Could you check if flacdec writes to invalid memory location when clipping by using valgrind?

In any case, this should be fixed before next -good release.
Comment 4 Sebastian Dröge (slomo) 2009-07-29 13:11:35 UTC
I can't reproduce this here with latest GIT... can you reproduce it?
Comment 5 Damien Lespiau 2009-07-29 18:12:18 UTC
Yes, I can reproduce it with latest git. I'll try to run valgrind on it as you suggested and understand a bit better what could go wrong. Unfortunately spare time is really hard to find these days.
Comment 6 Sebastian Dröge (slomo) 2009-07-30 10:44:11 UTC
Can you provide a sample file and sample code that shows this behaviour? I've tried really hard to reproduce this :)
Comment 7 Damien Lespiau 2009-07-30 13:54:02 UTC
Sure, here it is http://damien.lespiau.name/files/temp/878f969df5a65983d9f40018739e6d98/big-buck-bunny.ogv

I'm sorry to not be able to test/debug this further in a timely fashion, I'm really busy right now.

Thanks for trying hard to reproduce it! Some notes:

* I'm actually playing this with clutter-gst which using a thin layer on top of playbin. I did not review the code that was written something like 2 years but will do as the new clutter-gst "maintainer". So a bug and/or arbitrary memory corruption could come from clutter-gst. I would say it's quite unlikely.

* I'll write a simple test case trying to reproduce it.

Comment 8 Tim-Philipp Müller 2009-08-10 16:39:34 UTC
Sebastian: you marked this as blocker for the upcoming release.

Have you been able to reproduce the issue yet? Is anyone looking into this or working on a fix?
Comment 9 Sebastian Dröge (slomo) 2009-08-11 03:45:41 UTC
I wasn't able to reproduce it yet but someone should definitely look at it I guess. I'll try with the linked ogv file later.
Comment 10 Sebastian Dröge (slomo) 2009-08-11 10:09:27 UTC
Yes, I can confirm this bug with the big-buck-bunny file...
Comment 11 Sebastian Dröge (slomo) 2009-08-11 10:10:44 UTC
...but it happens with the latest core/base/good releases too unfortunately so
it's at least no regression in gst-plugins-good. Maybe some
basesink/audiosink/ringbuffer change in 0.10.24 has broken this...
Comment 12 Sebastian Dröge (slomo) 2009-08-11 10:27:57 UTC
This is apparently a bug in baseaudiosink, proposed patch follows.
Comment 13 Sebastian Dröge (slomo) 2009-08-11 10:30:29 UTC
Created attachment 140420 [details] [review]
baseaudiosink-render-empty-buffer.diff

Problem here is, that the buffer has data==NULL, 0 duration but a start timestamp. baseaudiosink doesn't notice that data==NULL, syncs (i.e. data += something) and then tries to commit this into the pulse ringbuffer.
Comment 14 Sebastian Dröge (slomo) 2009-08-11 10:56:40 UTC
Created attachment 140421 [details] [review]
gstsegment-clip.diff

This seems to be the real problem. gst_segment_clip() in baseaudiosink should drop the buffer even if start = stop < segment_start.
Comment 15 Sebastian Dröge (slomo) 2009-08-11 11:04:39 UTC
commit ca8a0376c5dda9aad1c726190585ba175160db4e
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Tue Aug 11 12:59:13 2009 +0200

    gstsegment: Clipping should detect start=stop<segment_start as outside the s
    
    Before it returned that [start,stop] is inside the segment and that the
    difference between segment_start and start needs to be clipped. If the
    clipping is done on a buffer (like in baseaudiosink) this will result
    in the data pointer being at a invalid memory position.
    
    Fixes bug #589849.