GNOME Bugzilla – Bug 765072
splitmuxsink: Sometimes creates a small one-frame file after EOS
Last modified: 2016-05-04 15:24:40 UTC
My application records an RTSP stream into splited mkv files, using splitmuxsink. The video is mjpeg 8fps (every frame is a keyframe). When I want to stop recording, I send EOS on the splitmuxsink element, then change its state to NULL. Sometimes (rarely) this produces a small 2nd file, with a single frame in it. The problem seems to be that when it receives EOS, it close the current file, then if state didn't change yet (it's a race) it can have time to start the next file. I'm not fully understanding what's goint on yet, but I guess it could be related with this commit: 8248eecff05b9a54e2aecf559d306090679e8bee.
After pushing the EOS event, do you wait for the EOS message on the bus before changing state?
I'm waiting to get EOS event on the bin's bus. Here is what I think happens: - EOS is catched on all streams in handle_mq_input(). - In handle_gathered_gop() at_eos is TRUE, it sets state to SPLITMUX_STATE_ENDING_FILE, but max_out_running_time is not set to GST_CLOCK_TIME_NONE in that case. So it could still have a possibly incomplete GOP in the mq that won't be allowed to leave. - complete_or_wait_on_out() will call send_eos() on every streams. Note that those EOS are not the EOS queued in the mq, since they are queued behind the last GOP. It generates eos events because state is SPLITMUX_STATE_ENDING_FILE just like if we reached file size limit. - bus_handler() catch EOS. It change state to SPLITMUX_STATE_START_NEXT_FRAGMENT, and won't let that EOS propagate to application, so my app won't change state to NULL yet. - start_next_fragment() goes in the case when reference EOS has in_eos set to TRUE. It sets state to SPLITMUX_STATE_WAITING_GOP_COMPLETE and max_out_running_time to GST_CLOCK_TIME_NONE. Note that it also uselessly set splitmux->have_muxed_something = FALSE; since that value is changed the line just after. - The last GOP goes out in a new file, then queued EOS pop out of the mq. This time the EOS event is propagated up to the pipeline. - My app catch EOS, change state of splitmuxsink to NULL and it teardown everything.
Created attachment 326109 [details] [review] spitmuxsink: Avoid creating small file at EOS When EOS is reached, the current file get closed and the last GOP in the mq was written in a new file.
As always, if you know how to reproduce/trigger this situation it'd be nice to add a small unit test to make sure it stays fixed :)
This was a regression introduced by my 2nd patch in bug #763711. Thanks for tracking it down.
Reopening because the at_eos stuff was wrong, but was there for a reason: EOS event does not always reach sink anymore.
Created attachment 326455 [details] [review] splitmuxsink: Fix deadlock case when source reaches EOS
Problem was that the EOS event was never released into the mq because it blocks forever in the while loop at the end of check_completed_gop(). If we are in the function and in_eos==TRUE that means we just received that event, I see no reason to not let it go into the mq immediately, is there?
Or maybe one exit condition of the loop should be !splitmux->reference_ctx->in_eos ?
When the reference context goes EOS, the max_in_running_time is set to GST_CLOCK_TIME_NONE anyway, which triggers an exit from the loop (max_in_running_time had to be != GST_CLOCK_TIME_NONE beforehand anyway). I think the patch above is sufficient, and re-fixes bug #763711 for me. Only one niggle - I'm not sure why your patch is From: Cerbero Build System <cerbero@gstreamer.freedesktop.org>
> Only one niggle - I'm not sure why your patch is From: Cerbero Build System > <cerbero@gstreamer.freedesktop.org> Because of: https://cgit.freedesktop.org/gstreamer/cerbero/commit/?id=17d4c4d64ca6947da6837fb119c94253d9e32b58 https://cgit.freedesktop.org/gstreamer/cerbero/commit/?id=60b4cb12c1bcb2ce09aec1cee7b640b12d74effc
(In reply to Tim-Philipp Müller from comment #11) > > Only one niggle - I'm not sure why your patch is From: Cerbero Build System > > <cerbero@gstreamer.freedesktop.org> > > Because of: > https://cgit.freedesktop.org/gstreamer/cerbero/commit/ > ?id=17d4c4d64ca6947da6837fb119c94253d9e32b58 > https://cgit.freedesktop.org/gstreamer/cerbero/commit/ > ?id=60b4cb12c1bcb2ce09aec1cee7b640b12d74effc Well that's just mean.
oh, haven't paid attention to this. My workflow is hacking in cerbero's shell/sources so I can do "make install" and test immediately. Dunno how cerbero ended up rewriting me commits.
Created attachment 327304 [details] [review] splitmuxsink: Fix deadlock case when source reaches EOS
Thanks, applied: commit 0fc02f35c7ff97ba895061670743abca5c6fa4b4 Author: Xavier Claessens <xavier.claessens@collabora.com> Date: Wed May 4 11:15:20 2016 -0400 splitmuxsink: Fix deadlock case when source reaches EOS https://bugzilla.gnome.org/show_bug.cgi?id=765072