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

[Edward Hervey]

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