GNOME Bugzilla – Bug 701763
audiodecoder: May return not-negotiated when flushing
Last modified: 2013-07-24 09:39:06 UTC
Created attachment 246199 [details] [review] Proposed patch to fix the issue The problem pops when running: GST_CHECKS=test_basic_playback make check-integration-forever on my integration branch : https://github.com/MathieuDuponchelle/PitiviGes/commits/integration The end result is that "Gstreamer encountered a general stream error" gets posted on the bus. The steps are : 1) the demuxer tries to push a buffer 2) gst_pad_push_data locks the pad and checks for flushing but it is not flushing. 3) The demuxer gets a seek flush event, pushes a flush_start_event and waits for the lock. 4) _push_data unlocks the pad and calls chain_data_unchecked, which checks for flushing but the pad is not flushing yet. 5) in the meantime, the lock has been taken in push_event, the pad has been set to flushing, and an element returns NOT_NEGOTIATED. 6) _push_data returns NOT_NEGOTIATED. 7) the demuxer freaks out. The proposed patch checks if the pad has gone to flushing while it was unlocked, and returns GST_FLOW_FLUSHING in that case. It breaks one test case though, in gstghostpad, test_ghost_pads_remove_while_playing() , that test adds a robe, which deactivates the ghostpad and removes it, the test waits a GST_FLOW_OK return, but instead it gets GST_FLOW_FLUSHING, which seems reasonable as the pad was actually deactivated, so the patch also changes that assertion.
I don't think this patch is necessarily correct. You ignore the idle pad probes right below... and there's also nothing fundamentally wrong with just returning the downstream flow return if the pad just went flushing after the data was passed downstream. More interesting would be the reason why something returns NOT_NEGOTIATED if it's flushing. It should return FLUSHING! What is returning NOT_NEGOTIATED (I assume the push_event in 5) should be a push_data?) and why, can you explain? :)
(In reply to comment #1) > More interesting would be the reason why something returns NOT_NEGOTIATED if > it's flushing. It should return FLUSHING! What is returning NOT_NEGOTIATED (I > assume the push_event in 5) should be a push_data?) and why, can you explain? > :) Might be the same as the bug I fixed in BaseSrc, where it would negotiate, which fails because it's flushing. But negotiate return a boolean, so you need to manually check the flushing flags on the pad before assuming it's NOT_NEG. Just remainding, in case it's the same.
Any news on this Mathieu?
(In reply to comment #3) > Any news on this Mathieu? Yes, I continued investigating today and found out who returned NOT_NEGOTIATED and why. 1) A multiqueue loop tries to push a buffer on vorbisdec. 2) Vorbisdec (actually baseaudiodecoder) tries to renegotiate and fails because the pad is flushing. 3) It returns NOT_NEGOTIATED, multiqueue sets that as its src result. 4) A buffer is pushed on the multiqueue, and it returns its src result, which not conveniently is GST_FLOW_NOT_NEGOTIATED 5) All hell breaks loose (bis) baseaudiodecoder should check if the pad was flushing after a failed negotiation. The next patch does that.
Created attachment 247486 [details] [review] Proposed patch to fix the issue
Comment on attachment 247486 [details] [review] Proposed patch to fix the issue commit 97e68b36c743c67dc90fe44dc7ad5238f3d6e9c1 Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Fri Jun 21 20:41:15 2013 +0200 audiodecoder: Don't return not-negotiated if flushing If the pad is flushing after a failed negotiation, return GST_FLOW_FLUSHING. https://bugzilla.gnome.org/show_bug.cgi?id=701763
commit 85eac2c31ce7f7ed2aa0dbd4a9dc2ad42b734788 Author: Sebastian Dröge <slomo@circular-chaos.org> Date: Sun Jun 30 18:17:15 2013 +0200 video(enc|dec)oder: Don't return not-negotiated if flushing If the pad is flushing after a failed negotiation, return GST_FLOW_FLUSHING instead from finish_frame(). https://bugzilla.gnome.org/show_bug.cgi?id=701763 commit 50fd867a43c6ec11306f21ea0053b4a8cef7ebd4 Author: Sebastian Dröge <slomo@circular-chaos.org> Date: Sun Jun 30 18:16:35 2013 +0200 audioencoder: Don't return not-negotiated if flushing If the pad is flushing after a failed negotiation, return GST_FLOW_FLUSHING instead from finish_frame(). https://bugzilla.gnome.org/show_bug.cgi?id=701763
One more question about these commits: Is it ok to use the GST_PAD_IS_FLUSHING (dec->srcpad) macro here? Shouldn't we perhaps add a gst_pad_is_flushing() that takes the lock?
Might be cleaner indeed, though in the audiodecoder the flush start/stop is protected by GST_AUDIO_DECODER_STREAM_LOCK(), which prevent the flags from being removed before we check on FLUSH_STOP, I did not check the other cases.
these flags are set also in gstpad.c, and protected by the pad's object lock. I was merely thinking the access via the macro without taking GST_OBJECT_LOCK(pad) isn't necessarily thread-safe.
In theory you're right but I don't think it matters much. We check a single bit of an integer, and the functions that would've returned the error would've taken the pad's object lock themselves already. After the flag is set, it is only ever unset from the streaming thread with the stream lock. And we hold that.
*** Bug 697215 has been marked as a duplicate of this bug. ***