GNOME Bugzilla – Bug 626815
vp8dec: infinite loop if EOS event before started
Last modified: 2010-08-19 09:19:13 UTC
gstbasevideodecoder.c has this loop when handling GST_EVENT_EOS: do { flow_ret = base_video_decoder_class->parse_data (base_video_decoder, TRUE); } while (flow_ret == GST_FLOW_OK); gstvp8dec.c implements parse_data as such: static GstFlowReturn gst_vp8_dec_parse_data (GstBaseVideoDecoder * decoder, gboolean at_eos) { return GST_FLOW_OK; } The result is quite obvious. I don't know what the design of basevideodecoder is supposed to be or what parse_data should mean, but my quickfix is simply returning GST_FLOW_NOT_SUPPORTED in gst_vp8_dec_parse_data, as it is both true and has the desired side-effect of not hanging the decoder. I don't understand why this bug doesn't rear its ugly head all the time, perhaps for some reason GST_EVENT_EOS isn't always sent?
Created attachment 167804 [details] [review] quickfix
Marking as blocker for the release.
How can parse_data be called at all in vp8dec? It's only called by the video decoder base class if packetized is FALSE. It is TRUE for vp8dec...
The only way we could reproduce this in Opera was by shutting down the page quickly after opening it, which indicates a race condition of some sort. packetized is set to TRUE in gst_vp8_dec_start, and I assume that there's no guarantee that this is called before GST_EVENT_EOS reaches gst_base_video_decoder_sink_event. Note that our custom src sends an eos event when it needs to shut down, which is probably why you won't see this is normal use.
Then the better solution might be to set packetized to TRUE in gst_vp8_dec_init() already And start should maybe be called earlier already, maybe while the pads are activated
Yes, setting it in gst_vp8_dec_init should fix the problem as well. Calling start earlier might be good for other reasons, but I think it would only make the window smaller, not eliminate the problem.
Ok, do you want to provide a patch for that? :)
Created attachment 168038 [details] [review] patch Since you ask so nicely :)
Comment on attachment 168038 [details] [review] patch Looks good, someone please push this, but with a better commit message if possible (no need to describe what the code is doing, but under what conditions this happened before and why, so that it's clear what this fixes without the need to refer to the bug which btw should also be mentioned in the msg). thanks!
commit e72574124f7470d8abd1fd221aab620593cad8b5 Author: Philip Jägenstedt <philipj@opera.com> Date: Fri Aug 13 14:34:21 2010 +0200 vp8dec: Set GstBaseVideoDecoder::packetized to TRUE as soon as possible This fixes an infinite loop if an EOS event is received before GstBaseVideoDecoder::start() is called, e.g. immediately when the pads are activated. Fixes bug #626815.