GNOME Bugzilla – Bug 707108
There are more than 10 frames will lost if we paused a video.
Last modified: 2013-11-21 10:20:25 UTC
current we use a timeout mechanism in decode thread.Which introduce many problem . 1. if you pause a video. and wait a moment(10s for example), then resume it. You we will see many frames are dropped in video.Because we have a 1 seconds timeout in our decode thread, it will drop frame every second. 2. we only return two value in handle_frames, GST_FLOW_OK and GST_FLOW_EOS. It did not follow gst specs and some demux such as avidemux will treat this as error. We need forward down stream's return to up stream. especially expected failures. steps to reproduce: a. checkout git://bee.sh.intel.com/git/media/simple-player branch ivi b. apply sego.patch in attachments c. make gtkplayer1 d. run gtkplayer, click open button, select a video(720p or 1080p)is better. e. clip play. to reproduce 1. you need click pause, wait more than 10 seconds, click play. pay attention to video. to reproduce 2. you need click pause, seek video use your middle button. you will see error or video freeze.
Created attachment 253577 [details] [review] to init value in right place.
Created attachment 253578 [details] [review] we need compare all objects(free+used) to capacity
Created attachment 253579 [details] [review] add mutex in videopool, since it will touched by two threads
Created attachment 253580 [details] [review] remove timeout in decode thread and forward gst_pad_push return to upstream
Created attachment 253581 [details] [review] patch to simple player to help you reproduce this bug
I have made four patches to fix this problem first three patch is fix error handling. real code is in fourth one. and I have reviewed 0.10's code. It only have one thread. It's really necessary to introduce other thread? what is your main concern? BR.
Review of attachment 253579 [details] [review]: (GMutex *) to (GMutex) type with g_mutex_init() / clear() in init/finalize hooks. The pointer variants (g_mutex_new()/g_mutex_free()) are deprecated and probably even removed nowadays. I prefer the following naming convention: default entry point remains as is and please rename _with_lock to _unlocked.
Review of attachment 253577 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapidecoder_objects.c @@ +104,3 @@ gboolean success; + picture->param_id = VA_INVALID_ID; Thanks. I think there are other instances too, e.g. for other VA objects. ::: gst-libs/gst/vaapi/gstvaapisurfaceproxy.c @@ +68,2 @@ proxy = (GstVaapiSurfaceProxy *) + gst_vaapi_mini_object_new0(gst_vaapi_surface_proxy_class()); Could you please find and initialize the missing member instead? Thanks.
Review of attachment 253580 [details] [review]: Yes, that's what I wanted to do next. However, are we guaranteed that the downstream sink gets unpaused first? That's because vaapidecode may be cond waiting in the main demux thread too.
Review of attachment 253578 [details] [review]: seems this fix is not necessary. g_queue_pop_head return NULL mean get length == 0 too.I will remove this late.
I am not sure what is your mean. But I try to give some my opinions: 1. there are one component named multiqueue between demux and audio/video decoder. It create two threads one for audio decoder, one for video decoder.And it accept buffers from demux thread, than push them to audio/video decoder thread. So vaapidecode will not block in demux thread. It have it's own thread. 2. From thread view, pause only do one thing. Trap gst_pad_push call in audio/video sink. unpause will release all threads trap in sink element. So you do not need worry about downstream things. 3. gstreamer will insert a queue (size = 3) between video decoder and sink. So it's no benefit to create a stand alone output thread in decoder. 4.If we pause a video. The entire pipeline will stopped after all queue have been filled. But if we use a timeout we do not take this advantage. Because we polling surface status. So one thread, no any timeout is good for us. BR. (In reply to comment #9) > Review of attachment 253580 [details] [review]: > > Yes, that's what I wanted to do next. However, are we guaranteed that the > downstream sink gets unpaused first? That's because vaapidecode may be cond > waiting in the main demux thread too.
Created attachment 254144 [details] [review] seems all va id is init in right place excpet param_id in gst_vaapi_slice_create 1. set proxy->destroy_func to NULL instead of new0 entire class. 2. init all va id to VA_INVALID_ID at the start of gst_vaapi_slice_create
Created attachment 254145 [details] [review] chang _with_lock to unlocked.
Applied locally + some additional changes to fix regressions in GStreamer 0.10 with certain streams. I also applied the GstVideoPool thread-safety patch but I am doubtful about the initial intent as it seems usages were already guarded by upper layer locks? Do you remember the exact cause/situation that made you add this patch? Thanks.
(In reply to comment #14) > Applied locally + some additional changes to fix regressions in GStreamer 0.10 > with certain streams. > > I also applied the GstVideoPool thread-safety patch but I am doubtful about the > initial intent as it seems usages were already guarded by upper layer locks? Do > you remember the exact cause/situation that made you add this patch? Thanks. not sure your upper layer means. we push surface to down stream in gst_vaapidecode_decode_loop. the downstream queue in playsink will catch the surface and wait time to render. after surface render done, it will return to surface pool. We get the surface from decoder input thread. put them to pool in queue thread or output thread. I am not sure the lock you said can protect both case.
commit 6e85f08e33db4a227e4c9c373926a156698e5403 Author: XuGuangxin <guangxin.xu@intel.com> Date: Thu Aug 29 14:12:10 2013 +0800 vaapidecode: fix hard reset for seek cases. Fix hard reset for seek cases by flushing the GstVaapiDecoder queue and completely purge any decoded output frame that may come out from it. At this stage, the GstVaapiDecoder shall be in a complete clean state to start decoding over new buffers. Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit 367944ba322f66ecb7435583fcde7663f853aa40 Author: XuGuangxin <guangxin.xu@intel.com> Date: Thu Aug 29 14:12:10 2013 +0800 vaapidecode: drop decode timeout, always wait for a free surface. vaapidecode used to wait up to one second past the expected time of presentation for the last decoded frame. This is not realistic in practice when it comes to video pause/resume. Changed behaviour to unconditionnally wait for a free VA surface prior to continuing the decoding. The decode task will continue pushing the output frames to the downstream element while also reporting errors at the same time to the main thread. https://bugzilla.gnome.org/show_bug.cgi?id=707108 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
commit f9ee8703cfdea8e9ac69dd957be20d6732a8615f Author: XuGuangxin <guangxin.xu@intel.com> Date: Thu Aug 29 13:44:22 2013 +0800 libs: make GstVaapiVideoPool thread-safe. https://bugzilla.gnome.org/show_bug.cgi?id=707108 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit f9d0d6e2720fa8b9004e839a5f9049c3d5cd3f32 Author: XuGuangxin <guangxin.xu@intel.com> Date: Thu Aug 29 14:04:06 2013 +0800 libs: robustify decoder objects and surface proxy initialization. Fix GstVaapiPicture, GstVaapiSlice and GstVaapiSurfaceProxy initialization sequences to have the expected default values set beforehand in case of an error raising up further during creation. i.e. make it possible to cleanly destroy those partially initialized objects. https://bugzilla.gnome.org/show_bug.cgi?id=707108 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
*** Bug 706400 has been marked as a duplicate of this bug. ***