GStreamer-devel

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

classic Classic list List threaded Threaded
♦
♦
 2 messages Options
Edward Hervey
Reply | Threaded
Open this post in threaded view
♦
♦
|

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

Edward Hervey
Administrator
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 <[hidden email]>
> 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
> [hidden email]
> http://lists.freedesktop.org/mailman/listinfo/gstreamer-commits


_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel
Krzysztof Konopko
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: 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 <[hidden email]>
> > 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
> > [hidden email]
> > http://lists.freedesktop.org/mailman/listinfo/gstreamer-commits
>
>
> _______________________________________________
> gstreamer-devel mailing list
> [hidden email]
> http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel
>

_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel
Free forum by Nabble Edit this page