GNOME Bugzilla – Bug 767641
videodecoder: Missing drain vfunc GST_FIXME flood on Raspberry Pi
Last modified: 2016-07-05 16:40:50 UTC
Created attachment 329765 [details] sample H264 byte-stream +++ This bug was initially created as a clone of Bug #767232 +++ Since commit eb1ebf226ff8c29ea976169f81135219f1ab412b of gst-plugins-base the decoder will emit GST_FIXME logs: FIXME videodecoder gstvideodecoder.c:1057:gst_video_decoder_drain_out:<omxh264dec-omxh264dec0> Sub-class should implement drain() For every frame handled by the decoder. The use-case is the following: GST_DEBUG=3 gst-launch-1.0 filesrc location=raw.h264 ! "video/x-h264, stream-format=(string)byte-stream, alignment=(string)au, level=(string)1.3, profile=(string)main, width=(int)320, height=(int)240, framerat e=(fraction)2500/83, pixel-aspect-ratio=(fraction)1/1, parsed=(boolean)true" ! decodebin use-buffering=TRUE ! glimagesink
Created attachment 329773 [details] 264parse:6,baseparse:6,videodecoder:6 log
Created attachment 329929 [details] [review] omxvideodec: Implement ::drain() virtual method
Untested patch, please test and let me know :)
It works fine, thanks!
Attachment 329929 [details] pushed as 198e313 - omxvideodec: Implement ::drain() virtual method
I'm sorry I didn't test this properly. With the patch applied and when the decoder drain is activated it never finishes, the drain_cond isn't signaled.
The decoder handles the first frame and is in PAUSED state and then a drain is requested but the drain_cond isn't signaled, I'll attach a log file.
Created attachment 330585 [details] log file
There's a deadlock in gst_omx_videodec_dec_reconfigure_output_port() on the video decoder stream lock acquisition: https://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxvideodec.c#n949 Not sure yet what's going on with that rec mutex :(
The mutex is locked once in gst_video_decoder_chain(), then once in gst_video_decoder_drain_out() and unlocked once only in gst_omx_video_dec_drain().
Are all other callers to drain_out() not taking that lock? Sounds like we need a boolean parameter there then to know if it's already taken or not to distinguish these cases. Can you check?
chain() is the only caller taking the stream lock indeed. Adding the boolean parameter to drain_out() works, the decoder drains and I see the preroll frame on-screen but now there's another deadlock it seems :/
After the drain gst_omx_port_acquire_buffer() in the loop function returns GST_OMX_ACQUIRE_BUFFER_EOS but it shouldn't! I'll debug this a bit further today.
Created attachment 330662 [details] [review] videodecoder patch
Comment on attachment 330662 [details] [review] videodecoder patch If I'm not mistaken, maybe just require that the stream lock is taken when drain_out() is called? And you just take and release it around the call in the callers that don't have it yet? That seems like a cleaner solution
Created attachment 330698 [details] [review] updated videodecoder patch
Review of attachment 330698 [details] [review]: ::: gst-libs/gst/video/gstvideodecoder.c @@ +2194,2 @@ ret = gst_video_decoder_drain_out (decoder, FALSE); + GST_VIDEO_DECODER_STREAM_UNLOCK (decoder); If I'm not missing anything, chain_forward() is always called with that lock taken already: from _chain() which takes it just around, and _flush_parse() is called from _drain_out() (!) for reverse playback and _chain_reverse() which is called with the lock taken in _chain(). @@ +2232,2 @@ gst_video_decoder_drain_out (decoder, FALSE); + GST_VIDEO_DECODER_STREAM_UNLOCK (decoder); Same here
Created attachment 330889 [details] [review] videodecoder: Take stream lock one time only on drain When the drain is triggered from the chain function the lock is already taken so there is no need to take it one more time.
Attachment 330889 [details] pushed as 6338146 - videodecoder: Take stream lock one time only on drain