gstoggmux EOS handling issue

[Daniel Drake-5]

Hi,

I reported a problem earlier:
http://marc.info/?l=gstreamer-devel&m=121562099110189&w=2
with this program:
http://dev.laptop.org/~dsd/recordtwice.txt
It tries to record 2 ogg videos of videotestsrc, but always fails to
record the 2nd one. I reproduced this problem on multiple systems.

I have investigated further and I believe it is a bug in the gstoggmux
element.

I only have a little knowledge of gstreamer and don't feel that I have a
100% understanding of the issue, but I'll try anyway:

The first time we stop recording, gstoggmux detects the EOS:
oggpad->buffer is not NULL, oggpad->next_buffer is NULL, and oggpad->eos
is TRUE

However, that buffer is not flushed from the pad. So when we try to
reuse the oggmux later, it finds the non-NULL buffer (final frame of
last session), shifts the NULL next_buffer onto it, and then encounters
this:

gst_ogg_mux_process_best_pad()
  /* best->buffer is non-NULL, either the pad is EOS's or there is a
next
   * buffer */
  if (best->next_buffer == NULL && !best->eos) {
    GST_WARNING_OBJECT (ogg_mux, "no subsequent buffer and EOS not
reached");
    return GST_FLOW_WRONG_STATE;
  }

I think the issue is that after detecting EOS, the appropriate buffers
should be flushed from the pads, leaving clean state for next time.

Am I making any sense?

I'm attaching a patch which fixes the problem - my recordtwice program
now records two vidoes of videotestsrc. Unfortunately it doesn't
actually fix the real application which I was trying to fix, but I
believe that I've found a real bug and fixing this is a necessary step
in the process...

Thanks,
Daniel


From: Daniel Drake <[hidden email]>

Index: gst-plugins-base-0.10.19/ext/ogg/gstoggmux.c
===================================================================
--- gst-plugins-base-0.10.19.orig/ext/ogg/gstoggmux.c
+++ gst-plugins-base-0.10.19/ext/ogg/gstoggmux.c
@@ -1572,6 +1572,15 @@ gst_ogg_mux_clear_collectpads (GstCollec
     }
     g_queue_free (oggpad->pagebuffers);
     oggpad->pagebuffers = NULL;
+
+    if (oggpad->buffer) {
+      gst_buffer_unref(oggpad->buffer);
+      oggpad->buffer = NULL;
+    }
+    if (oggpad->next_buffer) {
+      gst_buffer_unref(oggpad->next_buffer);
+      oggpad->next_buffer = NULL;
+    }
   }
 }
 

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Tim-Philipp Müller-2]

On Wed, 2008-08-06 at 14:07 -0400, Daniel Drake wrote:

Hi Daniel,

> I have investigated further and I believe it is a bug in the gstoggmux
> ...
> I'm attaching a patch which fixes the problem - my recordtwice program
> now records two vidoes of videotestsrc. Unfortunately it doesn't
> actually fix the real application which I was trying to fix, but I
> believe that I've found a real bug and fixing this is a necessary step
> in the process...

I think you're right, but could you put this in bugzilla please?

http://bugzilla.gnome.org (component: gst-plugins-base)

Thanks!

(Btw, not sure what exactly you were trying to fix, and I only had a
very quick look at your code, but at first glance the shutdown sequence
doesn't seem ideal: as far as I can see it won't shut down cleanly with
an EOS making it through the recording bin; for ogg this just means that
in the worst case you're losing a few frames at the end; with other
muxers you might get slightly broken and/or indexless files).

Cheers
 -Tim



-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Daniel Drake-5]

On Fri, 2008-08-08 at 14:11 +0100, Tim-Philipp Müller wrote:

> On Wed, 2008-08-06 at 14:07 -0400, Daniel Drake wrote:
>
> Hi Daniel,
>
> > I have investigated further and I believe it is a bug in the gstoggmux
> > ...
> > I'm attaching a patch which fixes the problem - my recordtwice program
> > now records two vidoes of videotestsrc. Unfortunately it doesn't
> > actually fix the real application which I was trying to fix, but I
> > believe that I've found a real bug and fixing this is a necessary step
> > in the process...
>
> I think you're right, but could you put this in bugzilla please?
>
> http://bugzilla.gnome.org (component: gst-plugins-base)

Thanks, done:
http://bugzilla.gnome.org/show_bug.cgi?id=546955

> (Btw, not sure what exactly you were trying to fix, and I only had a
> very quick look at your code, but at first glance the shutdown sequence
> doesn't seem ideal: as far as I can see it won't shut down cleanly with
> an EOS making it through the recording bin; for ogg this just means that
> in the worst case you're losing a few frames at the end; with other
> muxers you might get slightly broken and/or indexless files).

Can you elaborate on this or point me at the relevant documentation,
please? I am quite new to gstreamer and this bug is likely also present
in the real app that I'm trying to fix: http://wiki.laptop.org/go/Record

Thanks,
Daniel



-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

[Daniel Drake-5]

On Fri, 2008-08-08 at 14:11 +0100, Tim-Philipp Müller wrote:
> (Btw, not sure what exactly you were trying to fix, and I only had a
> very quick look at your code, but at first glance the shutdown sequence
> doesn't seem ideal: as far as I can see it won't shut down cleanly with
> an EOS making it through the recording bin; for ogg this just means that
> in the worst case you're losing a few frames at the end; with other
> muxers you might get slightly broken and/or indexless files).

Tim explained this on IRC. Cleanly shutting down the recording stream
works around the bug I found (if it is indeed a bug).

In case it helps others, the process for cleanly stopping a live capture
is as follows:
 - send an EOS on the live capture element(s) using
   gst_element_send_event(gst_event_new_eos())
 - connect a watch handler to the bus of the pipeline and wait for
   GST_MESSAGE_EOS
 - when EOS arrives, set the pipeline state to NULL

Working code here:
http://dev.laptop.org/~dsd/20080811/recordtwice.txt

Daniel



-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel