From be31e7273be6d5b55d3199a87caea5df0c65ae22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Fri, 11 Jan 2013 12:52:10 +0100 Subject: [PATCH] omx: Fix some memory leaks and suboptimal locking --- omx/gstomxaudioenc.c | 25 ++++++++------- omx/gstomxvideodec.c | 75 +++++++++++++++++++++++++++++--------------- omx/gstomxvideoenc.c | 68 ++++++++++++++++++++++++--------------- 3 files changed, 104 insertions(+), 64 deletions(-) diff --git a/omx/gstomxaudioenc.c b/omx/gstomxaudioenc.c index 6b4a3d532f..ffcfb2e82b 100644 --- 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 9dff6822cf..4b3f1cf475 100644 --- 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 5da285ab5f..0832381270 100644 --- 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; } }