Created attachment 241956 [details] [review]
First try at fixing the issue

Any chance you could add a unit test that fails without the patch too?

It does! without the patch I am getting:

thiblahute@Thibault-laptop ~/devel/pitivi/1.0-uninstalled/base/tests/check ((no branch)) $ GST_CHECKS=test_flush_start_flush_stop make elements/adder.check
Running suite(s): adder
0%: Checks: 1, Failures: 1, Errors: 0
elements/adder.c:1238:F:general:test_flush_start_flush_stop:0: Failure 'GST_PAD_IS_FLUSHING (adder_src)' occured
Running suite(s): adder
0%: Checks: 1, Failures: 1, Errors: 0
elements/adder.c:1238:F:general:test_flush_start_flush_stop:0: Failure 'GST_PAD_IS_FLUSHING (adder_src)' occured
make: *** [elements/adder.check] Error 1

Sorry, I hadn't actually looked at the patch, so didn't see that it already contains a test :)

Trying it now.

Comment on attachment 241956 [details] [review]
First try at fixing the issue

Thanks!

The unit test fails for me randomly:

$ GST_CHECKS=test_flush_start_flush_stop make elements/adder.forever
Running suite(s): adder
100%: Checks: 1, Failures: 0, Errors: 0
Running suite(s): adder
0%: Checks: 1, Failures: 1, Errors: 0
elements/adder.c:1236:F:general:test_flush_start_flush_stop:0: Assertion 'GST_PAD_IS_FLUSHING (adder_src)' failed

Very easy to reproduce here.

Also, could you in future please git commit --amend your commits/patches to include the bug number or link, thanks!

Ok, just managed to reproduce with a while loop (after 15 iteration) I will investigate that.

Really sorry for not adding the fix #X just that I did not think my patch would be merge right away.

OK so I investigated and the race is the following:

In the test: We send the flush_start
Thread 1: The adder sinkpads receives it and set the flush_stop_pending to TRUE
Thread 2: The adder has received buffers on all its sinkpads so collectpad calls gst_adder_collected, flush_stop_pending is set to TRUE so it creates and sends a flush_stop event on adder.src and sets flush_stop_pending to FALSE
          > adder.src.flushing = FALSE
Thread 1: We finaly send our flush_start event downstream
          > adder.src.flushing = TRUE
In the test: We send flush_stop
Thread 1 - Adder detects that flush_stop_pending is FALSE so it "eats" the event

I do not really get how that should be fixed without using plain locks, any guess?

Created attachment 242741 [details] [review]
Fix the last observed issue

I figured that we need to take the streaming thread of collectpad to ensure that the ->collected vmethod won't get called while we forward the flush event.

Does this fix all of it? It looks sensible but if you add a lock there anyway you can get rid of the atomic operations that were added there exactly because of bugs like this one :)

Created attachment 242787 [details] [review]
New patch removing now useless atomic operations

Are atomic ops still used for the adder->*pending fields elsewhere? If not this is good to be pushed I guess

Created attachment 242838 [details] [review]
Remove the remaining atomic operations and take the STREAMING_LOCK where necessary

Thanks, looks good to me. To check if this is bulletproof, please ask Stefan on IRC if he can test it in buzztard :) He's also CC'd here

+1, tried the patch and works for me. Thanks for waiting.

Review of attachment 242838 [details] [review]:

::: gst/adder/gstadder.c
@@ +822,3 @@
     case GST_EVENT_FLUSH_START:
       /* ensure that we will send a flush stop */
+      GST_COLLECT_PADS_STREAM_LOCK (adder->collect);

I'm just curious, with normal pad, we can't take stream lock to send a flush-start, does collect pads stream lock endup taking pads stream lock ?

The collectpad STREAM_LOCK is a special lock from collectpad, it is not related to pad stream's lock.

slomo: Is it good enough to merge then?

Yes

commit 372eddf00e9496a558472e197ec6880663092c14
Author: Thibault Saunier <thibault.saunier@collabora.com>
Date:   Sun Apr 28 20:07:47 2013 +0200

    adder: Get collectpad stream lock when fowarding flush events
    
    Fixes #698410