GNOME Bugzilla – Bug 721078
videodecoder: cannot call gst_video_decoder_negotiate without output_state
Last modified: 2018-08-21 17:09:54 UTC
Created attachment 264898 [details] [review] videodecoder: can call negotiate without output state The line "g_return_val_if_fail (decoder->priv->output_state, FALSE);" in "gst_video_decoder_negotiate" was introduced in master here http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=0c2c909497f5f97074cf1b0d26bd14939f892b6f and then merged in 1.2 branch. Then the line has been removed in master with this commit http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=cd52ff313e2c9eca0d20787f598b1d752da4c5f7 but not merged in 1.2 branch. So that now the line is present 1.2 branch and not in master. Since mfcdec calls gst_video_decoder_set_output_state in subclass function "negotiate", the check always fails in 1.2 branch. After a very quick look I have not seen another decoder which does that. So maybe the pb is mfcdec. But I can confirm that mfcdec works if I remove the check. For encoders, the line is present in both master and 1.2 branch. For audio(dec/enc) there is no point because no output_state. In the end I do not know what the best solution, i.e. if the line "g_return_val_if_fail (decoder->priv->output_state, FALSE);" should be or not be present in gstvideodecoder::gst_video_decoder_negotiate . But I think we should let the user be responsible for this, so just allow to do that. Maybe the check was just introduced by mistake in the first commit.
http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=cd52ff313e2c9eca0d20787f598b1d752da4c5f7 This was not backported to 1.2 at all, so that should happen first before your patch makes any sense :) But I think this change is too intrusive for 1.2 Or did I misunderstand anything here?
(In reply to comment #1) > http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=cd52ff313e2c9eca0d20787f598b1d752da4c5f7 I know and this is why mfcdec (by chance ? :) ) does not trigger this g_return_val_if_fail in master. > > This was not backported to 1.2 at all, so that should happen first before your > patch makes any sense :) But I think this change is too intrusive for 1.2 You mean the commit http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=cd52ff313e2c9eca0d20787f598b1d752da4c5f7 ? if yes then, yup I agree. Or is my patch too intrusive ? > > Or did I misunderstand anything here? Well maybe http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=0c2c909497f5f97074cf1b0d26bd14939f892b6f should not have introduced "g_return_val_if_fail (decoder->priv->output_state, FALSE);" ? In other word, in 1.2 branch do we really want to prevent calling "gst_video_decoder_negotiate" with null output_state ? Because in master we allow it. (but indeed there is the commit that is too intrusive to be merged) Well in the current state, mfcdec triggers the check in 1.2, and not in master. I'm worry if there are other decoders that do that (set output state in negotiate), or mfcdec this is just an exception.
On a sidenote, g_return_if_fail() should only be used to check input arguments of public API, not internal state in code (use g_assert for that)
Created attachment 264930 [details] [review] mfcdec: configure output_state before to call gst_video_decoder_negotiate Ok so I fixed mfcdec instead. Actually it's the only video decoder that defines GstVideoDecoderClass::negotiate. (I did a quick check in -base, -good, -ugly, -bad, gst-vaapi, gst-omx) In almost all video decoders it's done like this: - do not define GstVideoDecoderClass::negotiate - define a function named "myvideodec_negotiate" that successively calls "gst_video_decoder_set_output_state" then "gst_video_decoder_negotiate" - call "myvideodec_negotiate" manually So I did it for mfcdec so that it's now useable in 1.2 branch. I tested it with playbin and seeking works too (Note that the bug title should be changed but it seems I have not the rights to do it)
Review of attachment 264930 [details] [review]: This is only needed for 1.2? I don't think this is correct, what negotiate() was doing should still be done whenever GstVideoDecoder::negotiate() is called
(In reply to comment #5) > Review of attachment 264930 [details] [review]: > > This is only needed for 1.2? > Yes because there is not "g_return_val_if_fail (decoder->priv->output_state, FALSE);" in "gst_video_decoder_negotiate" of master branch due to the commit that has not been merged because to intrusive. > I don't think this is correct, what negotiate() was doing should still be done > whenever GstVideoDecoder::negotiate() is called but what is done in negotiate() (.i.e. "gst_mfc_dec_negotiate") is mainly to call "gst_video_decoder_set_output_state". Whereas there is a "g_return_val_if_fail (decoder->priv->output_state, FALSE);" in "gst_video_decoder_negotiate". So that's a bit contradictious :)
> > This is only needed for 1.2? > > > Yes because there is not "g_return_val_if_fail (decoder->priv->output_state, > FALSE);" in "gst_video_decoder_negotiate" of master branch This statement does not make sense afaict? As I wrote above, any g_return_*_if_fail() line must never be relied on for anything, it might evaluate to nothing with the right defines set (as is often done on embedded builds).
(In reply to comment #6) > > I don't think this is correct, what negotiate() was doing should still be done > > whenever GstVideoDecoder::negotiate() is called > > but what is done in negotiate() (.i.e. "gst_mfc_dec_negotiate") is mainly to > call "gst_video_decoder_set_output_state". Whereas there is a > "g_return_val_if_fail (decoder->priv->output_state, FALSE);" in > "gst_video_decoder_negotiate". So that's a bit contradictious :) Maybe the g_return_val_if_fail() should be in default_negotiate() instead of where it is now. That would make more sense to me at least
Created attachment 265039 [details] [review] videodec/enc: allow to call negiotate without ouput state Move "g_return_val_if_fail" from negotiate() to negotiate_default()
Same for audio decoder/encoder maybe? Please push to the 1.2 branch (only!) :)
(In reply to comment #10) > Same for audio decoder/encoder maybe? Please push to the 1.2 branch (only!) :) Ok. There is no "output_state" in decoder/encoder.
Yeah but there are caps in the audio base classes
commit 3c324cb42d2e2051d1302f1dd2bb1bd43cefcc4e Author: Julien Isorce <julien.isorce@collabora.co.uk> Date: Mon Dec 30 15:28:24 2013 +0000 videodec/enc: allow to call negiotate without ouput state Some decoders call set_output_state from GstVideoDecoder::negotiate() So move the g_return_val_if_fail to default_negotiate(), i.e. where it is actually necessary. Fix https://bugzilla.gnome.org/show_bug.cgi?id=721078
(In reply to comment #12) > Yeah but there are caps in the audio base classes Sorry I do not follow you :) Could you provide a line and what it should check ? thx
It's all good :) In the audio base classes these checks are already in negotiate_default().
ok :)
Sorry to come late, but doesn't that mean that we could endup with no caps on allocation query ?