gst-plugins-bad: tsbase: add a guard with an atomic boolean when flushing

[Krzysztof Konopko]

Is this work tracked with the following bug?
https://bugzilla.gnome.org/show_bug.cgi?id=688091

I'm seeing crashes in tsdemuxer when flushing and the commits pointed in
the bug report seem related.

Thanks,
Kris

On 10/11/12 11:38, Edward Hervey wrote:
> Hi,
>
>   This condition shouldn't happen if mpegts_packetizer_flush() was
> called in FLUSH_STOP instead of FLUSH_START (since you are essentially
> guaranteed that no data transfer was happening when FLUSH_STOP was
> received).
>
>   Could you test that this fixes your issues by:
>   * locally reverting this commit and the previous one
>   * moving the call to mpegts_packetizer_flush() to FLUSH_STOP
>   * testing you don't get the issue with your test cases
>
>   Checking for an atomic integer (or using a lock) triggers a memory
> barrier which will be used every buffer (potentially very small ones and
> very often with HD DVB). So I would very much like us to avoid this.
>
>   Edward
>
> P.S. Could you please check with the maintainer of a plugin before
> pushing patches to it. Especially when it is something non-trivial.
> Better yet, open a bug report with your patches so that they can get
> reviewed extensively.
>
> On Fri, 2012-11-09 at 15:10 -0800, Josep Torra wrote:
> > Module: gst-plugins-bad
> > Branch: master
> > Commit: e14e310f7178aa8c020f593e3f71ec92ca2531f7
> > URL:   
> http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=e14e310f7178aa8c020f593e3f71ec92ca2531f7
> >
> > Author: Josep Torra <n770galaxy at gmail.com>
> > Date:   Sat Nov 10 00:08:35 2012 +0100
> >
> > tsbase: add a guard with an atomic boolean when flushing
> >
> > ---
> >
> >  gst/mpegtsdemux/mpegtsbase.c |    9 +++++++++
> >  gst/mpegtsdemux/mpegtsbase.h |    4 ++++
> >  2 files changed, 13 insertions(+), 0 deletions(-)
> >
> > diff --git a/gst/mpegtsdemux/mpegtsbase.c b/gst/mpegtsdemux/mpegtsbase.c
> > index d6bd410..9ff63fd 100644
> > --- a/gst/mpegtsdemux/mpegtsbase.c
> > +++ b/gst/mpegtsdemux/mpegtsbase.c
> > @@ -1362,6 +1362,7 @@ mpegts_base_sink_event (GstPad * pad,
> GstObject * parent, GstEvent * event)
> >        gst_event_unref (event);
> >        break;
> >      case GST_EVENT_FLUSH_START:
> > +      MPEGTSBASE_SET_FLUSHING (base, TRUE);
> >        mpegts_packetizer_flush (base->packetizer);
> >        mpegts_base_flush (base);
> >        res = GST_MPEGTS_BASE_GET_CLASS (base)->push_event (base, event);
> > @@ -1369,6 +1370,9 @@ mpegts_base_sink_event (GstPad * pad,
> GstObject * parent, GstEvent * event)
> >      case GST_EVENT_FLUSH_STOP:
> >        gst_segment_init (&base->segment, GST_FORMAT_UNDEFINED);
> >        base->seen_pat = FALSE;
> > +      res = GST_MPEGTS_BASE_GET_CLASS (base)->push_event (base, event);
> > +      MPEGTSBASE_SET_FLUSHING (base, FALSE);
> > +      break;
> >        /* Passthrough */
> >      default:
> >        res = GST_MPEGTS_BASE_GET_CLASS (base)->push_event (base, event);
> > @@ -1399,6 +1403,11 @@ mpegts_base_push (MpegTSBase * base,
> MpegTSPacketizerPacket * packet,
> >  {
> >    MpegTSBaseClass *klass = GST_MPEGTS_BASE_GET_CLASS (base);
> >  
> > +  if (G_UNLIKELY (MPEGTSBASE_IS_FLUSHING (base))) {
> > +    GST_LOG_OBJECT (base, "Dropping because flushing");
> > +    return GST_FLOW_FLUSHING;
> > +  }
> > +
> >    /* Call implementation */
> >    if (G_UNLIKELY (klass->push == NULL)) {
> >      GST_ERROR_OBJECT (base, "Class doesn't have a 'push'
> implementation !");
> > diff --git a/gst/mpegtsdemux/mpegtsbase.h b/gst/mpegtsdemux/mpegtsbase.h
> > index e872330..999a010 100644
> > --- a/gst/mpegtsdemux/mpegtsbase.h
> > +++ b/gst/mpegtsdemux/mpegtsbase.h
> > @@ -119,6 +119,7 @@ struct _MpegTSBase {
> >    guint8 *is_pes;
> >  
> >    gboolean disposed;
> > +  gboolean flushing;              /* ATOMIC */
> >  
> >    /* size of the MpegTSBaseProgram structure, can be overridden
> >     * by subclasses if they have their own MpegTSBaseProgram
> subclasses. */
> > @@ -183,6 +184,9 @@ struct _MpegTSBaseClass {
> >  #define MPEGTS_BIT_UNSET(field, offs)  ((field)[(offs) >> 3] &= ~(1
> << ((offs) & 0x7)))
> >  #define MPEGTS_BIT_IS_SET(field, offs) ((field)[(offs) >> 3] &   (1
> << ((offs) & 0x7)))
> >  
> > +#define MPEGTSBASE_IS_FLUSHING(self)        (g_atomic_int_get
> (&((self)->flushing)))
> > +#define MPEGTSBASE_SET_FLUSHING(self,state) (g_atomic_int_set
> (&((self)->flushing),state))
> > +
> >  G_GNUC_INTERNAL GType mpegts_base_get_type(void);
> >  
> >  G_GNUC_INTERNAL MpegTSBaseProgram *mpegts_base_get_program
> (MpegTSBase * base, gint program_number);
> >
> > _______________________________________________
> > gstreamer-commits mailing list
> > gstreamer-commits at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/gstreamer-commits
>
>
> _______________________________________________
> gstreamer-devel mailing list
> gstreamer-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel
>