After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 707108 - There are more than 10 frames will lost if we paused a video.
There are more than 10 frames will lost if we paused a video.
Status: RESOLVED FIXED
Product: gstreamer-vaapi
Classification: Other
Component: general
git master
Other Linux
: Normal major
: ---
Assigned To: gstreamer-vaapi maintainer(s)
gstreamer-vaapi maintainer(s)
: 706400 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-08-30 05:55 UTC by XuGuangxin
Modified: 2013-11-21 10:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
to init value in right place. (1.77 KB, patch)
2013-08-30 05:57 UTC, XuGuangxin
none Details | Review
we need compare all objects(free+used) to capacity (1.13 KB, patch)
2013-08-30 05:59 UTC, XuGuangxin
none Details | Review
add mutex in videopool, since it will touched by two threads (5.50 KB, patch)
2013-08-30 06:00 UTC, XuGuangxin
none Details | Review
remove timeout in decode thread and forward gst_pad_push return to upstream (7.35 KB, patch)
2013-08-30 06:01 UTC, XuGuangxin
none Details | Review
patch to simple player to help you reproduce this bug (398 bytes, patch)
2013-08-30 06:02 UTC, XuGuangxin
none Details | Review
seems all va id is init in right place excpet param_id in gst_vaapi_slice_create (2.42 KB, patch)
2013-09-05 02:31 UTC, XuGuangxin
none Details | Review
chang _with_lock to unlocked. (6.19 KB, patch)
2013-09-05 02:33 UTC, XuGuangxin
none Details | Review

Description XuGuangxin 2013-08-30 05:55:52 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.
Comment 1 XuGuangxin 2013-08-30 05:57:59 UTC
Created attachment 253577 [details] [review]
to init value in right place.
Comment 2 XuGuangxin 2013-08-30 05:59:43 UTC
Created attachment 253578 [details] [review]
we need compare all objects(free+used) to capacity
Comment 3 XuGuangxin 2013-08-30 06:00:22 UTC
Created attachment 253579 [details] [review]
add mutex in videopool, since it will touched by two threads
Comment 4 XuGuangxin 2013-08-30 06:01:45 UTC
Created attachment 253580 [details] [review]
remove timeout in decode thread and forward gst_pad_push return to upstream
Comment 5 XuGuangxin 2013-08-30 06:02:31 UTC
Created attachment 253581 [details] [review]
patch to simple player to help you reproduce this bug
Comment 6 XuGuangxin 2013-08-30 06:08:26 UTC
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.
Comment 7 Gwenole Beauchesne 2013-08-30 07:52:29 UTC
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.
Comment 8 Gwenole Beauchesne 2013-08-30 07:54:49 UTC
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.
Comment 9 Gwenole Beauchesne 2013-08-30 07:58:14 UTC
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.
Comment 10 XuGuangxin 2013-08-30 15:16:20 UTC
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.
Comment 11 XuGuangxin 2013-08-30 15:36:08 UTC
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.
Comment 12 XuGuangxin 2013-09-05 02:31:17 UTC
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
Comment 13 XuGuangxin 2013-09-05 02:33:13 UTC
Created attachment 254145 [details] [review]
chang _with_lock to unlocked.
Comment 14 Gwenole Beauchesne 2013-11-21 06:33:41 UTC
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.
Comment 15 XuGuangxin 2013-11-21 08:36:00 UTC
(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.
Comment 16 Gwenole Beauchesne 2013-11-21 10:13:24 UTC
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>
Comment 17 Gwenole Beauchesne 2013-11-21 10:13:50 UTC
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>
Comment 18 Gwenole Beauchesne 2013-11-21 10:20:25 UTC
*** Bug 706400 has been marked as a duplicate of this bug. ***