From 610e477565b13ca300d03d95d21857187d00b84a Mon Sep 17 00:00:00 2001 From: Xu Guangxin Date: Thu, 14 May 2020 11:03:49 +0800 Subject: [PATCH] msdkdec: hold a reference for the surfaces locked by msdk previous code releases GstBuffer too earlier. so we will see ERROR default gstmsdkvideomemory.c:77:gst_msdk_video_allocator_get_surface: failed to get surface available ERROR msdkbufferpool gstmsdkbufferpool.c:270:gst_msdk_buffer_pool_alloc_buffer: failed to create new MSDK memory We need to hold GstBuffer reference for msdk if the surfaced locked by msdk. step to reproduce. 1. ffmpeg -f lavfi -i testsrc=duration=10:size=320x240:rate=30 -pix_fmt yuv420p -c:v libx265 test.265 2. GST_GL_PLATFORM=egl gst-launch-1.0 -v filesrc location=test.265 ! h265parse ! msdkh265dec ! queue ! glimagesink Part-of: --- sys/msdk/gstmsdkdec.c | 192 ++++++++++++++++++++++++++++------------------- sys/msdk/gstmsdkdec.h | 12 +-- sys/msdk/gstmsdkvc1dec.c | 1 - 3 files changed, 116 insertions(+), 89 deletions(-) diff --git a/sys/msdk/gstmsdkdec.c b/sys/msdk/gstmsdkdec.c index 86ba58b..595a6e9 100644 --- a/sys/msdk/gstmsdkdec.c +++ b/sys/msdk/gstmsdkdec.c @@ -71,6 +71,14 @@ typedef struct _MsdkSurface GstVideoFrame copy; } MsdkSurface; +struct _MsdkDecTask +{ + MsdkSurface *surface; + mfxSyncPoint sync_point; + + gboolean decode_only; +}; + static gboolean gst_msdkdec_drain (GstVideoDecoder * decoder); static gboolean gst_msdkdec_flush (GstVideoDecoder * decoder); static gboolean gst_msdkdec_negotiate (GstMsdkDec * thiz, gboolean hard_reset); @@ -119,36 +127,45 @@ gst_msdkdec_get_oldest_frame (GstVideoDecoder * decoder) return frame; } +static inline void +free_surface (MsdkSurface * s) +{ + gst_buffer_unref (s->buf); + g_slice_free (MsdkSurface, s); +} + static void -free_surface (GstMsdkDec * thiz, MsdkSurface * s) +unmap_frame (GstMsdkDec * thiz, MsdkSurface * s) { if (s->copy.buffer) { - gst_video_frame_unmap (&s->copy); + /* we allocate this buffer from down stream, we need ref-1 for it */ gst_buffer_unref (s->copy.buffer); + gst_video_frame_unmap (&s->copy); + s->copy.buffer = NULL; } - if (s->data.buffer) + if (s->data.buffer) { gst_video_frame_unmap (&s->data); - - gst_buffer_unref (s->buf); - thiz->decoded_msdk_surfaces = g_list_remove (thiz->decoded_msdk_surfaces, s); - - g_slice_free (MsdkSurface, s); + s->data.buffer = NULL; + } } static void -gst_msdkdec_free_unlocked_msdk_surfaces (GstMsdkDec * thiz, - MsdkSurface * curr_surface) +gst_msdkdec_free_unlocked_msdk_surfaces (GstMsdkDec * thiz) { GList *l; MsdkSurface *surface; - for (l = thiz->decoded_msdk_surfaces; l;) { + for (l = thiz->locked_msdk_surfaces; l;) { + GList *next = l->next; surface = l->data; - l = l->next; - - if (surface != curr_surface && surface->surface->Data.Locked == 0) - free_surface (thiz, surface); + if (surface->surface->Data.Locked == 0) { + unmap_frame (thiz, surface); + free_surface (surface); + thiz->locked_msdk_surfaces = + g_list_delete_link (thiz->locked_msdk_surfaces, l); + } + l = next; } } @@ -170,8 +187,7 @@ allocate_output_buffer (GstMsdkDec * thiz, GstBuffer ** buffer) if (!frame->output_buffer) { /* Free un-unsed msdk surfaces firstly, hence the associated mfx * surfaces will be moved from used list to available list */ - if (thiz->postpone_free_surface || thiz->use_video_memory) - gst_msdkdec_free_unlocked_msdk_surfaces (thiz, NULL); + gst_msdkdec_free_unlocked_msdk_surfaces (thiz); flow = gst_video_decoder_allocate_output_frame (decoder, frame); if (flow != GST_FLOW_OK) { @@ -184,9 +200,6 @@ allocate_output_buffer (GstMsdkDec * thiz, GstBuffer ** buffer) gst_buffer_replace (&frame->output_buffer, NULL); gst_video_codec_frame_unref (frame); - if (thiz->postpone_free_surface) - GST_MINI_OBJECT_FLAG_SET (*buffer, GST_MINI_OBJECT_FLAG_LOCKABLE); - return GST_FLOW_OK; } @@ -233,7 +246,8 @@ get_surface (GstMsdkDec * thiz, GstBuffer * buffer) gst_msdk_update_mfx_frame_info_from_mfx_video_param (&i->surface->Info, &thiz->param); - thiz->decoded_msdk_surfaces = g_list_append (thiz->decoded_msdk_surfaces, i); + + thiz->locked_msdk_surfaces = g_list_append (thiz->locked_msdk_surfaces, i); return i; failed_unref_buffer2: @@ -619,6 +633,22 @@ _find_msdk_surface (gconstpointer msdk_surface, gconstpointer comp_surface) return cached_surface ? cached_surface->surface != _surface : -1; } +static void +finish_task (GstMsdkDec * thiz, MsdkDecTask * task) +{ + MsdkSurface *surface = task->surface; + if (surface) { + if (G_UNLIKELY (surface->copy.buffer)) { + unmap_frame (thiz, surface); + } + thiz->locked_msdk_surfaces = + g_list_append (thiz->locked_msdk_surfaces, surface); + } + task->sync_point = NULL; + task->surface = NULL; + task->decode_only = FALSE; +} + static GstFlowReturn gst_msdkdec_finish_task (GstMsdkDec * thiz, MsdkDecTask * task) { @@ -627,7 +657,6 @@ gst_msdkdec_finish_task (GstMsdkDec * thiz, MsdkDecTask * task) GstVideoCodecFrame *frame; MsdkSurface *surface; mfxStatus status; - GList *l; guint64 pts = MFX_TIMESTAMP_UNKNOWN; if (G_LIKELY (task->sync_point)) { @@ -640,13 +669,14 @@ gst_msdkdec_finish_task (GstMsdkDec * thiz, MsdkDecTask * task) } } - if (task->surface) { + surface = task->surface; + if (surface) { GST_DEBUG_OBJECT (thiz, "Decoded MFX TimeStamp: %" G_GUINT64_FORMAT, - (guint64) task->surface->Data.TimeStamp); - pts = task->surface->Data.TimeStamp; + (guint64) surface->surface->Data.TimeStamp); + pts = surface->surface->Data.TimeStamp; } - if (G_LIKELY (task->sync_point || (task->surface && task->decode_only))) { + if (G_LIKELY (task->sync_point || (surface && task->decode_only))) { gboolean decode_only = task->decode_only; frame = gst_msdkdec_get_oldest_frame (decoder); @@ -661,21 +691,16 @@ gst_msdkdec_finish_task (GstMsdkDec * thiz, MsdkDecTask * task) frame = gst_msdkdec_get_oldest_frame (decoder); } - l = g_list_find_custom (thiz->decoded_msdk_surfaces, task->surface, - _find_msdk_surface); - if (l) { - surface = l->data; - } else { - GST_ERROR_OBJECT (thiz, "Couldn't find the cached MSDK surface"); - return GST_FLOW_ERROR; - } - if (G_LIKELY (frame)) { if (G_LIKELY (surface->copy.buffer == NULL)) { + /* gst_video_decoder_finish_frame will call gst_buffer_make_writable + * we need this to avoid copy buffer */ + GST_MINI_OBJECT_FLAG_SET (surface->buf, GST_MINI_OBJECT_FLAG_LOCKABLE); frame->output_buffer = gst_buffer_ref (surface->buf); } else { gst_video_frame_copy (&surface->copy, &surface->data); frame->output_buffer = gst_buffer_ref (surface->copy.buffer); + unmap_frame (thiz, surface); } GST_DEBUG_OBJECT (thiz, "surface %p TimeStamp: %" G_GUINT64_FORMAT " frame %p TimeStamp: %" G_GUINT64_FORMAT, @@ -683,11 +708,7 @@ gst_msdkdec_finish_task (GstMsdkDec * thiz, MsdkDecTask * task) frame, GST_TO_MFX_TIME (frame->pts)); } - if (!thiz->postpone_free_surface) - free_surface (thiz, surface); - task->sync_point = NULL; - task->surface = NULL; - task->decode_only = FALSE; + finish_task (thiz, task); if (!frame) return GST_FLOW_FLUSHING; @@ -699,6 +720,7 @@ gst_msdkdec_finish_task (GstMsdkDec * thiz, MsdkDecTask * task) GST_ERROR_OBJECT (thiz, "Failed to finish frame"); return flow; } + finish_task (thiz, task); return GST_FLOW_OK; } @@ -842,12 +864,19 @@ release_msdk_surfaces (GstMsdkDec * thiz) { GList *l; MsdkSurface *surface; - - for (l = thiz->decoded_msdk_surfaces; l;) { - surface = l->data; - l = l->next; - free_surface (thiz, surface); + gint locked = 0; + gst_msdkdec_free_unlocked_msdk_surfaces (thiz); + + for (l = thiz->locked_msdk_surfaces; l; l = l->next) { + surface = (MsdkSurface *) l->data; + unmap_frame (thiz, surface); + free_surface (surface); + locked++; } + if (locked) + GST_ERROR_OBJECT (thiz, "msdk still locked %d surfaces", locked); + g_list_free (thiz->locked_msdk_surfaces); + thiz->locked_msdk_surfaces = NULL; } /* This will get invoked in the following situations: @@ -925,6 +954,26 @@ error_negotiate: return FALSE; } +static inline gboolean +find_msdk_surface (GstMsdkDec * thiz, MsdkDecTask * task, + mfxFrameSurface1 * out_surface) +{ + GList *l; + task->surface = NULL; + if (!out_surface) + return TRUE; + l = g_list_find_custom (thiz->locked_msdk_surfaces, out_surface, + _find_msdk_surface); + if (!l) { + GST_ERROR_OBJECT (thiz, "msdk return an invalid surface %p", out_surface); + return FALSE; + } + task->surface = (MsdkSurface *) l->data; + thiz->locked_msdk_surfaces = + g_list_delete_link (thiz->locked_msdk_surfaces, l); + return TRUE; +} + static GstFlowReturn gst_msdkdec_handle_frame (GstVideoDecoder * decoder, GstVideoCodecFrame * frame) { @@ -936,6 +985,7 @@ gst_msdkdec_handle_frame (GstVideoDecoder * decoder, GstVideoCodecFrame * frame) MsdkDecTask *task = NULL; mfxBitstream bitstream; MsdkSurface *surface = NULL; + mfxFrameSurface1 *out_surface = NULL; mfxSession session; mfxStatus status; GstMapInfo map_info; @@ -1116,7 +1166,13 @@ gst_msdkdec_handle_frame (GstVideoDecoder * decoder, GstVideoCodecFrame * frame) status = MFXVideoDECODE_DecodeFrameAsync (session, &bitstream, surface->surface, - &task->surface, &task->sync_point); + &out_surface, &task->sync_point); + + if (!find_msdk_surface (thiz, task, out_surface)) { + flow = GST_FLOW_ERROR; + goto done; + } + GST_DEBUG_OBJECT (decoder, "DecodeFrameAsync => %d", status); /* media-sdk requires complete reset since the surface is inadequate @@ -1152,19 +1208,12 @@ gst_msdkdec_handle_frame (GstVideoDecoder * decoder, GstVideoCodecFrame * frame) if (surface->surface->Data.Locked > 0) surface = NULL; - else if (!thiz->use_video_memory) { - /* The for loop will continue because DataLength is greater than 0 so - * free the surface right away if not in use. */ - if (bitstream.DataLength > 0 && task->surface != surface->surface) - free_surface (thiz, surface); - surface = NULL; - } if (bitstream.DataLength == 0) { flow = GST_FLOW_OK; /* Don't release it if the current surface is in use */ - if (surface && task->surface == surface->surface) + if (surface && task->surface->surface == surface->surface) surface = NULL; break; @@ -1186,11 +1235,8 @@ gst_msdkdec_handle_frame (GstVideoDecoder * decoder, GstVideoCodecFrame * frame) /* If device is busy, wait 1ms and retry, as per MSDK's recommendation */ g_usleep (1000); - if (task->surface && - task->surface == surface->surface && !task->sync_point) { - free_surface (thiz, surface); + if (surface->surface->Data.Locked > 0) surface = NULL; - } /* If the current surface is still busy, we should do sync operation, * then try to decode again @@ -1218,9 +1264,6 @@ gst_msdkdec_handle_frame (GstVideoDecoder * decoder, GstVideoCodecFrame * frame) gst_msdkdec_finish_task (thiz, task); done: - if (surface) - free_surface (thiz, surface); - gst_buffer_unmap (input_buffer, &map_info); gst_buffer_unref (input_buffer); return flow; @@ -1302,8 +1345,9 @@ gst_msdkdec_create_buffer_pool (GstMsdkDec * thiz, GstVideoInfo * info, } config = gst_buffer_pool_get_config (GST_BUFFER_POOL_CAST (pool)); + /* we need register all bufffers when we create the msdk context, so the buffer pool is not resize able */ gst_buffer_pool_config_set_params (config, caps, - GST_VIDEO_INFO_SIZE (info), num_buffers, 0); + GST_VIDEO_INFO_SIZE (info), num_buffers, num_buffers); gst_buffer_pool_config_add_option (config, GST_BUFFER_POOL_OPTION_VIDEO_META); gst_buffer_pool_config_add_option (config, GST_BUFFER_POOL_OPTION_VIDEO_ALIGNMENT); @@ -1317,6 +1361,7 @@ gst_msdkdec_create_buffer_pool (GstMsdkDec * thiz, GstVideoInfo * info, GST_BUFFER_POOL_OPTION_MSDK_USE_DMABUF); } + gst_buffer_pool_config_set_video_alignment (config, &align); gst_buffer_pool_config_set_allocator (config, allocator, ¶ms); gst_object_unref (allocator); @@ -1487,6 +1532,7 @@ gst_msdkdec_drain (GstVideoDecoder * decoder) GstBuffer *buffer; MsdkDecTask *task; MsdkSurface *surface = NULL; + mfxFrameSurface1 *out_surface; mfxSession session; mfxStatus status; guint i; @@ -1515,13 +1561,15 @@ gst_msdkdec_drain (GstVideoDecoder * decoder) status = MFXVideoDECODE_DecodeFrameAsync (session, NULL, surface->surface, - &task->surface, &task->sync_point); + &out_surface, &task->sync_point); + + if (!find_msdk_surface (thiz, task, out_surface)) { + return GST_FLOW_ERROR; + } + GST_DEBUG_OBJECT (decoder, "DecodeFrameAsync => %d", status); if (G_LIKELY (status == MFX_ERR_NONE)) { thiz->next_task = (thiz->next_task + 1) % thiz->tasks->len; - - if (surface->surface->Data.Locked == 0) - free_surface (thiz, surface); surface = NULL; } else if (status == MFX_WRN_VIDEO_PARAM_CHANGED) { continue; @@ -1541,8 +1589,6 @@ gst_msdkdec_drain (GstVideoDecoder * decoder) } else if (status < MFX_ERR_NONE) return GST_FLOW_ERROR; } - if (surface) - free_surface (thiz, surface); for (i = 0; i < thiz->tasks->len; i++) { task = &g_array_index (thiz->tasks, MsdkDecTask, thiz->next_task); @@ -1550,8 +1596,6 @@ gst_msdkdec_drain (GstVideoDecoder * decoder) thiz->next_task = (thiz->next_task + 1) % thiz->tasks->len; } - release_msdk_surfaces (thiz); - return GST_FLOW_OK; } @@ -1649,11 +1693,7 @@ gst_msdkdec_finalize (GObject * object) g_array_unref (thiz->tasks); thiz->tasks = NULL; - /* NULL is the empty list. */ - if (G_UNLIKELY (thiz->decoded_msdk_surfaces != NULL)) { - GST_ERROR_OBJECT (thiz, "leaking %u surfaces", - g_list_length (thiz->decoded_msdk_surfaces)); - } + release_msdk_surfaces (thiz); G_OBJECT_CLASS (parent_class)->finalize (object); } @@ -1762,10 +1802,8 @@ gst_msdkdec_init (GstMsdkDec * thiz) thiz->do_renego = TRUE; thiz->do_realloc = TRUE; thiz->force_reset_on_res_change = TRUE; - thiz->postpone_free_surface = FALSE; thiz->adapter = gst_adapter_new (); thiz->input_state = NULL; thiz->pool = NULL; thiz->context = NULL; - thiz->decoded_msdk_surfaces = NULL; } diff --git a/sys/msdk/gstmsdkdec.h b/sys/msdk/gstmsdkdec.h index b15c855..efc5b5a 100644 --- a/sys/msdk/gstmsdkdec.h +++ b/sys/msdk/gstmsdkdec.h @@ -85,8 +85,6 @@ struct _GstMsdkDec * include downstream requirement, msdk suggestion and extra * surface allocation for smooth display in render pipeline */ guint min_prealloc_buffers; - /* postpone surface free */ - gboolean postpone_free_surface; /* MFX context */ GstMsdkContext *context; @@ -95,7 +93,7 @@ struct _GstMsdkDec GArray *tasks; guint next_task; - GList *decoded_msdk_surfaces; + GList *locked_msdk_surfaces; /* element properties */ gboolean hardware; @@ -117,14 +115,6 @@ struct _GstMsdkDecClass gboolean (*postinit_decoder) (GstMsdkDec * decoder); }; -struct _MsdkDecTask -{ - mfxFrameSurface1 *surface; - mfxSyncPoint sync_point; - - gboolean decode_only; -}; - GType gst_msdkdec_get_type (void); G_END_DECLS diff --git a/sys/msdk/gstmsdkvc1dec.c b/sys/msdk/gstmsdkvc1dec.c index 5438ed1..c658c58 100644 --- a/sys/msdk/gstmsdkvc1dec.c +++ b/sys/msdk/gstmsdkvc1dec.c @@ -82,7 +82,6 @@ gst_msdkvc1dec_configure (GstMsdkDec * decoder) if (!structure) return FALSE; - decoder->postpone_free_surface = TRUE; decoder->param.mfx.CodecId = MFX_CODEC_VC1; profile_str = gst_structure_get_string (structure, "profile"); -- 2.7.4