GNOME Bugzilla – Bug 753914
vaapidecode: Always report cropped resolution in src caps
Last modified: 2017-03-07 16:16:14 UTC
All gstreamer elements are supposed to set the cropped width/hight in GST_CAPS_EVENT irrespective of whether it is doing cropping by itself or not. There are multiple issues in vaapidecode: --- if there is a resolution change in the encoded stream in the middle, vaapidecode won't advertise that resolution change in src caps --- if there is a resolution change in the encoded stream, vaapidecode ! software_rendererer pipeline won't work -- The input codec state GstVideoCodecState handling is not correct in vaapidecode..
Created attachment 310090 [details] [review] vaapidecode: Always keep a copy of input codec state
Created attachment 310091 [details] [review] vaapidecode: Rework the re-negotiation code to handle multi resoultion videos
In theory , we should advertise the cropped values (even though we are not doing the cropping in decoder) in decoder src caps. But this seems bit more complicated, since we are not setting src_caps in sub class(GstVaapiDecode) implementation. We may need to override the negotiate() virtual function to fix this correctly.. The gst_video_decoder_set_output_state() is the one who sets the src caps, but vaapidecode is supposed to use un-cropped values here since we have to allocate GStVaapiVideoMemory, meta, pool and related stuffs with un-cropped values. By the way, the attached patches should be sufficient to fix the existing issues(eg: hevc decoding problem with multi resolution video), but in longer term, we have to find a proper way to do the "cropped-resolution-advertisement". Might be some changes needed in upstream too.
Review of attachment 310090 [details] [review]: Sree, Check my patch attachment 310101 [details] [review] for bug 750835. If it works for your use-case, perhaps we should use it instead of doing a copy of the GstVideoCodecState.
Review of attachment 310090 [details] [review]: ::: gst/vaapi/gstvaapidecode.c @@ +139,3 @@ } +static GstVideoCodecState * Perhaps it would be nice to move this function to gstvideoutils.c in gst-plugins-base
(In reply to Víctor Manuel Jáquez Leal from comment #4) > Review of attachment 310090 [details] [review] [review]: > > Sree, > > Check my patch attachment 310101 [details] [review] [review] for bug 750835. If it > works for your use-case, perhaps we should use it instead of doing a copy of > the GstVideoCodecState. Make sense,,I will push it together with some other patches..thanks :)
(In reply to sreerenj from comment #6) > (In reply to Víctor Manuel Jáquez Leal from comment #4) > > Review of attachment 310090 [details] [review] [review] [review]: > > > > Sree, > > > > Check my patch attachment 310101 [details] [review] [review] [review] for bug 750835. If it > > works for your use-case, perhaps we should use it instead of doing a copy of > > the GstVideoCodecState. > > Make sense,,I will push it together with some other patches..thanks :) Hm Sorry, I have to rethink about this... It could be better to keep a clear separation between core libarary and plugins...Which means "use individual copies in both" so that later we can easily make separation (core lib and plugins) without changing the logic...Also there are some other changes which I wanna push , will make this patch more complicated, we have to do gst_caps_make_writable() in the core lib implementation....
(In reply to sreerenj from comment #7) > Hm Sorry, I have to rethink about this... It could be better to keep a clear > separation between core libarary and plugins...Which means "use individual > copies in both" so that later we can easily make separation (core lib and > plugins) without changing the logic...Also there are some other changes > which I wanna push , will make this patch more complicated, we have to do > gst_caps_make_writable() in the core lib implementation.... D'accord. Sounds sensible.
commit 32d1c5adff4c478a2b7aab0dc9a87de4839cd03b Author: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> Date: Sat Aug 29 00:27:05 2015 +0300 vaapidecode: renegotiate if caps are not equal commit 6eba201f3252eba6a99ab7da7a4c662091a3e884 Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Sat Aug 29 00:18:57 2015 +0300 vaapidecode: Rework the re-negotiation code to handle multi resoultion videos commit ba8fcf54356d84ec5794a14892c263ae9a870ded Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Fri Aug 28 23:43:47 2015 +0300 vaapidecode: Always keep a copy of input codec state
@Sree, shall we close this bug?
(In reply to Víctor Manuel Jáquez Leal from comment #10) > @Sree, shall we close this bug? No, this is not yet fixed...We need different scenarios for vp9 decoding... We usually negotiate the bufferpool (destory the vacontext, surface pool etc too) when ever there is a resolution change.This is not true for vp9. For vp9, we negotiate a pool only when a new frame come up with higher resolution than what we configured for the pool . If a new frame come up with low resolution than what we configured in pool, no need to negotiate. And we are not reporting the cropped resolution. More explanation in comment 3. IMO, there are multiple issues to be fixed (not just for vp9), I will look into this after 0.7 release.
There are more changes needed, I am Pushing my patches here: https://cgit.freedesktop.org/~sree/gstreamer-vaapi/
Review of attachment 310090 [details] [review]: This has been pushed already.
Review of attachment 310091 [details] [review]: This has been pushed already.
Pushed, clsoing.. Note: vaapidecode ! xvimagesink will fail if there is crop_meta attached to the buffer. As per the discussion here in #762543, there is a fix need in gst_video_frame_map () to handle the crop values. OR We should replace the crop_meta implementation with GstVideoAlignment , which requires some major rewrite in gstvaapivideomemory, gstvaapipostproc, gstvaapisink and other places too. commit 959d14ce8a9e0de184dfc7e73090e5287fa1f11e Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Thu Mar 24 15:09:43 2016 +0200 vaapidecode: Fix decide_allocation handling Set the already configured pool in decide_allocation query in cases where pool renegotiation is not required. https://bugzilla.gnome.org/show_bug.cgi?id=753914 commit 6b17ed90601e3849c657d9c233f247cff744d3f6 Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Thu Mar 24 15:09:15 2016 +0200 vaapidecode: Derive and save the decoded surface format After the decoding of first frame, try to extract the exact decoded surface format using vaDeriveImage and keep this as the format in decoded_info. https://bugzilla.gnome.org/show_bug.cgi?id=753914 commit 859a2b2f4f15fda98fd831555295f37f0db07802 Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Thu Mar 24 15:08:50 2016 +0200 Make vaapidecode to advertise the cropped values in srcpad, but negotiate pool only if needed -- Maintaing decoded surface resoluton and actual display resoultion separately -- Before pushing every frames downstream, check for the requirement of pool negoation and output_state negotiation: This is needed to avoid multiple issuses with cropping, multi-resoluton video handling, more complex multi resolution decode scenarios for vp9decode, possible wrong behaviour from upstream element to report uncropped values etc. Due to these reasons, We can't just reliably use the resolution change notification from libgstvaapi for pool renegotiation too. This is slight overhead, but safe enough. Optimization could be possible though. https://bugzilla.gnome.org/show_bug.cgi?id=753914 commit c2aa405a3eeabd3621414d4d5f0190223461d96a Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Thu Mar 24 15:08:27 2016 +0200 vaapidecode: Delay the output format setting until we have a decoded surface This will help to consoidate the out caps negotiation to a single place, which will make the code simpler, allows to get the exact decoded format if needed and the selected chroma type too. https://bugzilla.gnome.org/show_bug.cgi?id=753914
*** Bug 759302 has been marked as a duplicate of this bug. ***