omx: Fix some memory leaks and suboptimal locking

diff --git a/omx/gstomxaudioenc.c b/omx/gstomxaudioenc.c
index 6b4a3d532f58a5470d5832a19cbda306845d78a5..ffcfb2e82b016a0a1ff8586c24f851015781a465 100644 (file)
--- a/omx/gstomxaudioenc.c
+++ b/omx/gstomxaudioenc.c
@@ -275,6 +275,7 @@ gst_omx_audio_enc_loop (GstOMXAudioEnc * self)
     return;
   }
 
+  GST_AUDIO_ENCODER_STREAM_LOCK (self);
   if (!gst_pad_has_current_caps (GST_AUDIO_ENCODER_SRC_PAD (self))
       || acq_return == GST_OMX_ACQUIRE_BUFFER_RECONFIGURED) {
     GstAudioInfo *info =
@@ -283,7 +284,6 @@ gst_omx_audio_enc_loop (GstOMXAudioEnc * self)
 
     GST_DEBUG_OBJECT (self, "Port settings have changed, updating caps");
 
-    GST_AUDIO_ENCODER_STREAM_LOCK (self);
     caps = klass->get_caps (self, self->out_port, info);
     if (!caps) {
       if (buf)
@@ -302,17 +302,18 @@ gst_omx_audio_enc_loop (GstOMXAudioEnc * self)
       goto caps_failed;
     }
     gst_caps_unref (caps);
-    GST_AUDIO_ENCODER_STREAM_UNLOCK (self);
 
     /* Now get a buffer */
-    if (acq_return != GST_OMX_ACQUIRE_BUFFER_OK)
+    if (acq_return != GST_OMX_ACQUIRE_BUFFER_OK) {
+      GST_AUDIO_ENCODER_STREAM_UNLOCK (self);
       return;
+    }
   }
+  GST_AUDIO_ENCODER_STREAM_UNLOCK (self);
 
   g_assert (acq_return == GST_OMX_ACQUIRE_BUFFER_OK);
 
   if (buf) {
-
     GST_DEBUG_OBJECT (self, "Handling buffer: 0x%08x %lu", buf->omx_buf->nFlags,
         buf->omx_buf->nTimeStamp);
 
@@ -757,9 +758,6 @@ gst_omx_audio_enc_handle_frame (GstAudioEncoder * encoder, GstBuffer * inbuf)
   }
 
   if (self->downstream_flow_ret != GST_FLOW_OK) {
-    GST_ERROR_OBJECT (self, "Downstream returned %s",
-        gst_flow_get_name (self->downstream_flow_ret));
-
     return self->downstream_flow_ret;
   }
 
@@ -778,28 +776,31 @@ gst_omx_audio_enc_handle_frame (GstAudioEncoder * encoder, GstBuffer * inbuf)
      * because no input buffers are released */
     GST_AUDIO_ENCODER_STREAM_UNLOCK (self);
     acq_ret = gst_omx_port_acquire_buffer (self->in_port, &buf);
-    GST_AUDIO_ENCODER_STREAM_LOCK (self);
 
     if (acq_ret == GST_OMX_ACQUIRE_BUFFER_ERROR) {
+      GST_AUDIO_ENCODER_STREAM_LOCK (self);
       goto component_error;
     } else if (acq_ret == GST_OMX_ACQUIRE_BUFFER_FLUSHING) {
+      GST_AUDIO_ENCODER_STREAM_LOCK (self);
       goto flushing;
     } else if (acq_ret == GST_OMX_ACQUIRE_BUFFER_RECONFIGURE) {
-      if (gst_omx_port_reconfigure (self->in_port) != OMX_ErrorNone)
+      if (gst_omx_port_reconfigure (self->in_port) != OMX_ErrorNone) {
+        GST_AUDIO_ENCODER_STREAM_LOCK (self);
         goto reconfigure_error;
+      }
       /* Now get a new buffer and fill it */
+      GST_AUDIO_ENCODER_STREAM_LOCK (self);
       continue;
     } else if (acq_ret == GST_OMX_ACQUIRE_BUFFER_RECONFIGURED) {
       /* TODO: Anything to do here? Don't think so */
+      GST_AUDIO_ENCODER_STREAM_LOCK (self);
       continue;
     }
+    GST_AUDIO_ENCODER_STREAM_LOCK (self);
 
     g_assert (acq_ret == GST_OMX_ACQUIRE_BUFFER_OK && buf != NULL);
 
     if (self->downstream_flow_ret != GST_FLOW_OK) {
-      GST_ERROR_OBJECT (self, "Downstream returned %s",
-          gst_flow_get_name (self->downstream_flow_ret));
-
       gst_omx_port_release_buffer (self->in_port, buf);
       return self->downstream_flow_ret;
     }

diff --git a/omx/gstomxvideodec.c b/omx/gstomxvideodec.c
index 9dff6822cf9c91167f71871608d8a2922fdaa000..4b3f1cf475220b9b3b64e8ce6672239003b0421d 100644 (file)
--- a/omx/gstomxvideodec.c
+++ b/omx/gstomxvideodec.c
@@ -538,6 +538,7 @@ gst_omx_video_dec_loop (GstOMXVideoDec * self)
     return;
   }
 
+  GST_VIDEO_DECODER_STREAM_LOCK (self);
   if (!gst_pad_has_current_caps (GST_VIDEO_DECODER_SRC_PAD (self))
       || acq_return == GST_OMX_ACQUIRE_BUFFER_RECONFIGURED) {
     GstVideoCodecState *state;
@@ -546,7 +547,6 @@ gst_omx_video_dec_loop (GstOMXVideoDec * self)
 
     GST_DEBUG_OBJECT (self, "Port settings have changed, updating caps");
 
-    GST_VIDEO_DECODER_STREAM_LOCK (self);
     gst_omx_port_get_port_definition (port, &port_def);
     g_assert (port_def.format.video.eCompressionFormat ==
         OMX_VIDEO_CodingUnused);
@@ -588,34 +588,35 @@ gst_omx_video_dec_loop (GstOMXVideoDec * self)
       if (buf)
         gst_omx_port_release_buffer (self->out_port, buf);
       gst_video_codec_state_unref (state);
-      GST_VIDEO_DECODER_STREAM_UNLOCK (self);
       goto caps_failed;
     }
 
     gst_video_codec_state_unref (state);
-    GST_VIDEO_DECODER_STREAM_UNLOCK (self);
 
     /* Now get a buffer */
-    if (acq_return != GST_OMX_ACQUIRE_BUFFER_OK)
+    if (acq_return != GST_OMX_ACQUIRE_BUFFER_OK) {
+      GST_VIDEO_DECODER_STREAM_UNLOCK (self);
       return;
+    }
   }
+  GST_VIDEO_DECODER_STREAM_UNLOCK (self);
 
   g_assert (acq_return == GST_OMX_ACQUIRE_BUFFER_OK);
 
+  /* This prevents a deadlock between the srcpad stream
+   * lock and the videocodec stream lock, if ::reset()
+   * is called at the wrong time
+   */
+  if (gst_omx_port_is_flushing (self->out_port)) {
+    GST_DEBUG_OBJECT (self, "Flushing");
+    gst_omx_port_release_buffer (self->out_port, buf);
+    goto flushing;
+  }
+
   if (buf) {
     GST_DEBUG_OBJECT (self, "Handling buffer: 0x%08x %lu", buf->omx_buf->nFlags,
         buf->omx_buf->nTimeStamp);
 
-    /* This prevents a deadlock between the srcpad stream
-     * lock and the videocodec stream lock, if ::reset()
-     * is called at the wrong time
-     */
-    if (gst_omx_port_is_flushing (self->out_port)) {
-      GST_DEBUG_OBJECT (self, "Flushing");
-      gst_omx_port_release_buffer (self->out_port, buf);
-      goto flushing;
-    }
-
     GST_VIDEO_DECODER_STREAM_LOCK (self);
     frame = _find_nearest_frame (self, buf);
 
@@ -628,6 +629,7 @@ gst_omx_video_dec_loop (GstOMXVideoDec * self)
           "Frame is too late, dropping (deadline %" GST_TIME_FORMAT ")",
           GST_TIME_ARGS (-deadline));
       flow_ret = gst_video_decoder_drop_frame (GST_VIDEO_DECODER (self), frame);
+      frame = NULL;
     } else if (!frame && buf->omx_buf->nFilledLen > 0) {
       GstBuffer *outbuf;
 
@@ -658,16 +660,18 @@ gst_omx_video_dec_loop (GstOMXVideoDec * self)
         if (!gst_omx_video_dec_fill_buffer (self, buf, frame->output_buffer)) {
           gst_buffer_replace (&frame->output_buffer, NULL);
           flow_ret =
-              gst_video_decoder_finish_frame (GST_VIDEO_DECODER (self), frame);
+              gst_video_decoder_drop_frame (GST_VIDEO_DECODER (self), frame);
+          frame = NULL;
           gst_omx_port_release_buffer (self->out_port, buf);
           goto invalid_buffer;
         }
         flow_ret =
             gst_video_decoder_finish_frame (GST_VIDEO_DECODER (self), frame);
+        frame = NULL;
       }
     } else if (frame != NULL) {
-      flow_ret =
-          gst_video_decoder_finish_frame (GST_VIDEO_DECODER (self), frame);
+      flow_ret = gst_video_decoder_drop_frame (GST_VIDEO_DECODER (self), frame);
+      frame = NULL;
     }
 
     GST_DEBUG_OBJECT (self, "Read frame from component");
@@ -775,6 +779,7 @@ caps_failed:
     GST_ELEMENT_ERROR (self, LIBRARY, SETTINGS, (NULL), ("Failed to set caps"));
     gst_pad_push_event (GST_VIDEO_DECODER_SRC_PAD (self), gst_event_new_eos ());
     gst_pad_pause_task (GST_VIDEO_DECODER_SRC_PAD (self));
+    GST_VIDEO_DECODER_STREAM_UNLOCK (self);
     self->downstream_flow_ret = GST_FLOW_NOT_NEGOTIATED;
     self->started = FALSE;
     return;
@@ -1204,6 +1209,7 @@ gst_omx_video_dec_handle_frame (GstVideoDecoder * decoder,
 
   if (self->eos) {
     GST_WARNING_OBJECT (self, "Got frame after EOS");
+    gst_video_codec_frame_unref (frame);
     return GST_FLOW_EOS;
   }
 
@@ -1211,9 +1217,7 @@ gst_omx_video_dec_handle_frame (GstVideoDecoder * decoder,
   duration = frame->duration;
 
   if (self->downstream_flow_ret != GST_FLOW_OK) {
-    GST_ERROR_OBJECT (self, "Downstream returned %s",
-        gst_flow_get_name (self->downstream_flow_ret));
-
+    gst_video_codec_frame_unref (frame);
     return self->downstream_flow_ret;
   }
 
@@ -1224,6 +1228,7 @@ gst_omx_video_dec_handle_frame (GstVideoDecoder * decoder,
     if (ret != GST_FLOW_OK) {
       GST_ERROR_OBJECT (self, "Preparing frame failed: %s",
           gst_flow_get_name (ret));
+      gst_video_codec_frame_unref (frame);
       return ret;
     }
   }
@@ -1235,21 +1240,28 @@ gst_omx_video_dec_handle_frame (GstVideoDecoder * decoder,
      * because no input buffers are released */
     GST_VIDEO_DECODER_STREAM_UNLOCK (self);
     acq_ret = gst_omx_port_acquire_buffer (self->in_port, &buf);
-    GST_VIDEO_DECODER_STREAM_LOCK (self);
 
     if (acq_ret == GST_OMX_ACQUIRE_BUFFER_ERROR) {
+      GST_VIDEO_DECODER_STREAM_LOCK (self);
       goto component_error;
     } else if (acq_ret == GST_OMX_ACQUIRE_BUFFER_FLUSHING) {
+      GST_VIDEO_DECODER_STREAM_LOCK (self);
       goto flushing;
     } else if (acq_ret == GST_OMX_ACQUIRE_BUFFER_RECONFIGURE) {
-      if (gst_omx_port_reconfigure (self->in_port) != OMX_ErrorNone)
+      if (gst_omx_port_reconfigure (self->in_port) != OMX_ErrorNone) {
+        GST_VIDEO_DECODER_STREAM_LOCK (self);
         goto reconfigure_error;
+      }
+
       /* Now get a new buffer and fill it */
+      GST_VIDEO_DECODER_STREAM_LOCK (self);
       continue;
     } else if (acq_ret == GST_OMX_ACQUIRE_BUFFER_RECONFIGURED) {
       /* TODO: Anything to do here? Don't think so */
+      GST_VIDEO_DECODER_STREAM_LOCK (self);
       continue;
     }
+    GST_VIDEO_DECODER_STREAM_LOCK (self);
 
     g_assert (acq_ret == GST_OMX_ACQUIRE_BUFFER_OK && buf != NULL);
 
@@ -1259,11 +1271,8 @@ gst_omx_video_dec_handle_frame (GstVideoDecoder * decoder,
     }
 
     if (self->downstream_flow_ret != GST_FLOW_OK) {
-      GST_ERROR_OBJECT (self, "Downstream returned %s",
-          gst_flow_get_name (self->downstream_flow_ret));
-
       gst_omx_port_release_buffer (self->in_port, buf);
-      return self->downstream_flow_ret;
+      goto flow_error;
     }
 
     if (self->codec_data) {
@@ -1341,20 +1350,31 @@ gst_omx_video_dec_handle_frame (GstVideoDecoder * decoder,
     gst_omx_port_release_buffer (self->in_port, buf);
   }
 
+  gst_video_codec_frame_unref (frame);
+
   GST_DEBUG_OBJECT (self, "Passed frame to component");
 
   return self->downstream_flow_ret;
 
 full_buffer:
   {
+    gst_video_codec_frame_unref (frame);
     GST_ELEMENT_ERROR (self, LIBRARY, FAILED, (NULL),
         ("Got OpenMAX buffer with no free space (%p, %u/%u)", buf,
             buf->omx_buf->nOffset, buf->omx_buf->nAllocLen));
     return GST_FLOW_ERROR;
   }
 
+flow_error:
+  {
+    gst_video_codec_frame_unref (frame);
+
+    return self->downstream_flow_ret;
+  }
+
 too_large_codec_data:
   {
+    gst_video_codec_frame_unref (frame);
     GST_ELEMENT_ERROR (self, STREAM, FORMAT, (NULL),
         ("codec_data larger than supported by OpenMAX port (%u > %u)",
             gst_buffer_get_size (codec_data),
@@ -1364,6 +1384,7 @@ too_large_codec_data:
 
 component_error:
   {
+    gst_video_codec_frame_unref (frame);
     GST_ELEMENT_ERROR (self, LIBRARY, FAILED, (NULL),
         ("OpenMAX component in error state %s (0x%08x)",
             gst_omx_component_get_last_error_string (self->component),
@@ -1373,11 +1394,13 @@ component_error:
 
 flushing:
   {
+    gst_video_codec_frame_unref (frame);
     GST_DEBUG_OBJECT (self, "Flushing -- returning FLUSHING");
     return GST_FLOW_FLUSHING;
   }
 reconfigure_error:
   {
+    gst_video_codec_frame_unref (frame);
     GST_ELEMENT_ERROR (self, LIBRARY, SETTINGS, (NULL),
         ("Unable to reconfigure input port"));
     return GST_FLOW_ERROR;

diff --git a/omx/gstomxvideoenc.c b/omx/gstomxvideoenc.c
index 5da285ab5f8efa132a3ed2a6165c413317a6f76b..0832381270bb2c94a3a44680cc388cc1f3e7bba0 100644 (file)
--- a/omx/gstomxvideoenc.c
+++ b/omx/gstomxvideoenc.c
@@ -632,8 +632,10 @@ gst_omx_video_enc_handle_output_frame (GstOMXVideoEnc * self, GstOMXPort * port,
         gst_video_encoder_set_output_state (GST_VIDEO_ENCODER (self), caps,
         self->input_state);
     state->codec_data = codec_data;
-    if (!gst_video_encoder_negotiate (GST_VIDEO_ENCODER (self)))
+    if (!gst_video_encoder_negotiate (GST_VIDEO_ENCODER (self))) {
+      gst_video_codec_frame_unref (frame);
       return GST_FLOW_NOT_NEGOTIATED;
+    }
     flow_ret = GST_FLOW_OK;
   } else if (buf->omx_buf->nFilledLen > 0) {
     GstBuffer *outbuf;
@@ -714,13 +716,12 @@ gst_omx_video_enc_loop (GstOMXVideoEnc * self)
     return;
   }
 
+  GST_VIDEO_ENCODER_STREAM_LOCK (self);
   if (!gst_pad_has_current_caps (GST_VIDEO_ENCODER_SRC_PAD (self))
       || acq_return == GST_OMX_ACQUIRE_BUFFER_RECONFIGURED) {
     GstCaps *caps;
     GstVideoCodecState *state;
 
-    GST_VIDEO_ENCODER_STREAM_LOCK (self);
-
     GST_DEBUG_OBJECT (self, "Port settings have changed, updating caps");
 
     caps = klass->get_caps (self, self->out_port, self->input_state);
@@ -745,29 +746,30 @@ gst_omx_video_enc_loop (GstOMXVideoEnc * self)
       goto caps_failed;
     }
 
-    GST_VIDEO_ENCODER_STREAM_UNLOCK (self);
-
     /* Now get a buffer */
-    if (acq_return != GST_OMX_ACQUIRE_BUFFER_OK)
+    if (acq_return != GST_OMX_ACQUIRE_BUFFER_OK) {
+      GST_VIDEO_ENCODER_STREAM_UNLOCK (self);
       return;
+    }
   }
+  GST_VIDEO_ENCODER_STREAM_UNLOCK (self);
 
   g_assert (acq_return == GST_OMX_ACQUIRE_BUFFER_OK);
 
+  /* This prevents a deadlock between the srcpad stream
+   * lock and the videocodec stream lock, if ::reset()
+   * is called at the wrong time
+   */
+  if (gst_omx_port_is_flushing (self->out_port)) {
+    GST_DEBUG_OBJECT (self, "Flushing");
+    gst_omx_port_release_buffer (self->out_port, buf);
+    goto flushing;
+  }
+
   if (buf) {
     GST_DEBUG_OBJECT (self, "Handling buffer: 0x%08x %lu", buf->omx_buf->nFlags,
         buf->omx_buf->nTimeStamp);
 
-    /* This prevents a deadlock between the srcpad stream
-     * lock and the videocodec stream lock, if ::reset()
-     * is called at the wrong time
-     */
-    if (gst_omx_port_is_flushing (self->out_port)) {
-      GST_DEBUG_OBJECT (self, "Flushing");
-      gst_omx_port_release_buffer (self->out_port, buf);
-      goto flushing;
-    }
-
     GST_VIDEO_ENCODER_STREAM_LOCK (self);
     frame = _find_nearest_frame (self, buf);
 
@@ -795,7 +797,6 @@ gst_omx_video_enc_loop (GstOMXVideoEnc * self)
     gst_omx_port_release_buffer (port, buf);
 
     self->downstream_flow_ret = flow_ret;
-
   } else {
     g_assert ((klass->cdata.hacks & GST_OMX_HACK_NO_EMPTY_EOS_BUFFER));
     GST_VIDEO_ENCODER_STREAM_LOCK (self);
@@ -1265,13 +1266,12 @@ gst_omx_video_enc_handle_frame (GstVideoEncoder * encoder,
 
   if (self->eos) {
     GST_WARNING_OBJECT (self, "Got frame after EOS");
+    gst_video_codec_frame_unref (frame);
     return GST_FLOW_EOS;
   }
 
   if (self->downstream_flow_ret != GST_FLOW_OK) {
-    GST_ERROR_OBJECT (self, "Downstream returned %s",
-        gst_flow_get_name (self->downstream_flow_ret));
-
+    gst_video_codec_frame_unref (frame);
     return self->downstream_flow_ret;
   }
 
@@ -1284,21 +1284,27 @@ gst_omx_video_enc_handle_frame (GstVideoEncoder * encoder,
      * because no input buffers are released */
     GST_VIDEO_ENCODER_STREAM_UNLOCK (self);
     acq_ret = gst_omx_port_acquire_buffer (self->in_port, &buf);
-    GST_VIDEO_ENCODER_STREAM_LOCK (self);
 
     if (acq_ret == GST_OMX_ACQUIRE_BUFFER_ERROR) {
+      GST_VIDEO_ENCODER_STREAM_LOCK (self);
       goto component_error;
     } else if (acq_ret == GST_OMX_ACQUIRE_BUFFER_FLUSHING) {
+      GST_VIDEO_ENCODER_STREAM_LOCK (self);
       goto flushing;
     } else if (acq_ret == GST_OMX_ACQUIRE_BUFFER_RECONFIGURE) {
-      if (gst_omx_port_reconfigure (self->in_port) != OMX_ErrorNone)
+      if (gst_omx_port_reconfigure (self->in_port) != OMX_ErrorNone) {
+        GST_VIDEO_ENCODER_STREAM_LOCK (self);
         goto reconfigure_error;
+      }
       /* Now get a new buffer and fill it */
+      GST_VIDEO_ENCODER_STREAM_LOCK (self);
       continue;
     } else if (acq_ret == GST_OMX_ACQUIRE_BUFFER_RECONFIGURED) {
       /* TODO: Anything to do here? Don't think so */
+      GST_VIDEO_ENCODER_STREAM_LOCK (self);
       continue;
     }
+    GST_VIDEO_ENCODER_STREAM_LOCK (self);
 
     g_assert (acq_ret == GST_OMX_ACQUIRE_BUFFER_OK && buf != NULL);
 
@@ -1308,11 +1314,8 @@ gst_omx_video_enc_handle_frame (GstVideoEncoder * encoder,
     }
 
     if (self->downstream_flow_ret != GST_FLOW_OK) {
-      GST_ERROR_OBJECT (self, "Downstream returned %s",
-          gst_flow_get_name (self->downstream_flow_ret));
-
       gst_omx_port_release_buffer (self->in_port, buf);
-      return self->downstream_flow_ret;
+      goto flow_error;
     }
 
     /* Now handle the frame */
@@ -1368,6 +1371,8 @@ gst_omx_video_enc_handle_frame (GstVideoEncoder * encoder,
     GST_DEBUG_OBJECT (self, "Passed frame to component");
   }
 
+  gst_video_codec_frame_unref (frame);
+
   return self->downstream_flow_ret;;
 
 full_buffer:
@@ -1375,33 +1380,44 @@ full_buffer:
     GST_ELEMENT_ERROR (self, LIBRARY, FAILED, (NULL),
         ("Got OpenMAX buffer with no free space (%p, %u/%u)", buf,
             buf->omx_buf->nOffset, buf->omx_buf->nAllocLen));
+    gst_video_codec_frame_unref (frame);
     return GST_FLOW_ERROR;
   }
 
+flow_error:
+  {
+    gst_video_codec_frame_unref (frame);
+    return self->downstream_flow_ret;
+  }
+
 component_error:
   {
     GST_ELEMENT_ERROR (self, LIBRARY, FAILED, (NULL),
         ("OpenMAX component in error state %s (0x%08x)",
             gst_omx_component_get_last_error_string (self->component),
             gst_omx_component_get_last_error (self->component)));
+    gst_video_codec_frame_unref (frame);
     return GST_FLOW_ERROR;
   }
 
 flushing:
   {
     GST_DEBUG_OBJECT (self, "Flushing -- returning FLUSHING");
+    gst_video_codec_frame_unref (frame);
     return GST_FLOW_FLUSHING;
   }
 reconfigure_error:
   {
     GST_ELEMENT_ERROR (self, LIBRARY, SETTINGS, (NULL),
         ("Unable to reconfigure input port"));
+    gst_video_codec_frame_unref (frame);
     return GST_FLOW_ERROR;
   }
 buffer_fill_error:
   {
     GST_ELEMENT_ERROR (self, RESOURCE, WRITE, (NULL),
         ("Failed to write input into the OpenMAX buffer"));
+    gst_video_codec_frame_unref (frame);
     return GST_FLOW_ERROR;
   }
 }