GNOME Bugzilla – Bug 764316
vaapidecode: simplify downstream negotiation
Last modified: 2016-04-29 08:41:51 UTC
Lastest commits enhance the downstream negotiation for VAAPI, but they can be simplified and improved because they overlaps some logic contained in the base class GstVideoDecoder.
Created attachment 324950 [details] [review] vaapidecode: move gst_vaapidecode_negotiate() code With it we can remove a function declaration, making the code a bit more readable.
Created attachment 324951 [details] [review] simplify the downstream renegotiation This patch is a simplification of the last commits regarding the downstream renegotiation given the processed surface. This simplification is oriented to avoid the duplication of responsibilities with GstVideoDecoder super-class, in particular with those related with the pool negotiation. As a side gain some global state variables are now unneeded.
Created attachment 324952 [details] [review] vaapidecode: remove spurious class variable active class variable was used as a flag to negotiate downstream once, but now that each time a new surface resolution is pop out a renegotation is done, this variable is not required anymore, thus removed.
Hm, these patches are causing more regressions . Not only the vp9 sample i have shared before. Patch1 is trivial , just removing the function declaration, which is okay to push if you want. But others require careful review and regression testing. So please don't push those now. we can think of removing more global state variables, but on the other hand I have a feeling that it is more readable when keeping separate VideoInfo for decoded_surface and display_info. Better optimizations also possible by removing Caps variables from GstVaapiDecode structure and utilize the GstVideoInfo instead.
(In reply to sreerenj from comment #4) > Hm, these patches are causing more regressions . Not only the vp9 sample i > have shared before. Can you point me to a failing test? Or email me a vaapidecode:7 log? > > Patch1 is trivial , just removing the function declaration, which is okay to > push > if you want. Yes. > But others require careful review and regression testing. So please don't > push those now. > we can think of removing more global state variables, but on the other hand > I have a feeling that it is more readable when keeping separate VideoInfo > for decoded_surface and display_info. Better optimizations also possible by > removing Caps variables from GstVaapiDecode structure and utilize the > GstVideoInfo instead. The only argument of keeping a output_{width,height,format} variables instead of GstVideoInfo is because of memory saving. But I can agree on change it. What I can't understand is the reason of having both decoded_info and display_info, when we only care about the downstream caps negotiation.
(In reply to Víctor Manuel Jáquez Leal from comment #5) > (In reply to sreerenj from comment #4) > > Hm, these patches are causing more regressions . Not only the vp9 sample i > > have shared before. > > Can you point me to a failing test? Or email me a vaapidecode:7 log? I have found one when using ximagesink as videso sink.
(In reply to Víctor Manuel Jáquez Leal from comment #6) > (In reply to Víctor Manuel Jáquez Leal from comment #5) > > (In reply to sreerenj from comment #4) > > > Hm, these patches are causing more regressions . Not only the vp9 sample i > > > have shared before. > > > > Can you point me to a failing test? Or email me a vaapidecode:7 log? > > I have found one when using ximagesink as videso sink. False alarm. It is working too with these patches. I can't find regressions :/
Comment on attachment 324950 [details] [review] vaapidecode: move gst_vaapidecode_negotiate() code Attachment 324950 [details] pushed as fe08f7e - vaapidecode: move gst_vaapidecode_negotiate() code
Created attachment 325021 [details] sample.264 Try this for eg: gst-launch-1.0 filesrc location=sample.264 ! h264parse ! vaapidecode ! videoconvert ! xvimagesink
(In reply to sreerenj from comment #9) > Created attachment 325021 [details] > sample.264 > > Try this for eg: > > gst-launch-1.0 filesrc location=sample.264 ! h264parse ! vaapidecode ! > videoconvert ! xvimagesink Aha! Thanks!
Created attachment 325098 [details] [review] simplify the downstream renegotiation This patch is a simplification of the last commits regarding the downstream renegotiation given the processed surface. This simplification is oriented to avoid the duplication of responsibilities with GstVideoDecoder super-class, in particular with those related with the pool negotiation. As a side gain some global state variables are now unneeded.
Created attachment 325099 [details] [review] vaapidecode: remove spurious class variable active class variable was used as a flag to negotiate downstream once, but now that each time a new surface resolution is pop out a renegotation is done, this variable is not required anymore, thus removed.
Created attachment 325408 [details] [review] simplify the downstream renegotiation This patch is a simplification of the last commits regarding the downstream renegotiation given the processed surface. This simplification is oriented to avoid the duplication of responsibilities with GstVideoDecoder super-class, in particular with those related with the pool negotiation. As a side gain some global state variables are now unneeded.
Created attachment 325409 [details] [review] vaapidecode: remove spurious class variable active class variable was used as a flag to negotiate downstream once, but now that each time a new surface resolution is pop out a renegotation is done, this variable is not required anymore, thus removed.
Since bug 764421 is closed, we can push these two patches. Sree, your call ;)
Do you still prefer to keep a GstVideoInfo instead of the output_{height,width,format} ???
(In reply to Víctor Manuel Jáquez Leal from comment #15) > Since bug 764421 is closed, we can push these two patches. Sree, your call ;) Could you please hold on a bit more ?, I can do some testing (assuming you have no h/w to test the vp9 streams), but not immediate,,,damn busy with some other major issues..
(In reply to sreerenj from comment #17) > (In reply to Víctor Manuel Jáquez Leal from comment #15) > > Since bug 764421 is closed, we can push these two patches. Sree, your call ;) > > Could you please hold on a bit more ?, I can do some testing (assuming you > have no h/w to test the vp9 streams), but not immediate,,,damn busy with > some other major issues.. No rush. First thing first. btw, I will test VP9 decoding with hybrid driver (software).
Okay, let me try to give one example where your patch is going to fail ;) surface_resolution: 640x480 crop_rectangle: 320x240 everything is working fine... now in the middle there is a surface resolution change: new_surface_resolution: 1280x720 new_crop_rectangle: 320x240 here the crop_values are the same, but actual surface dimension get changed. As per what i can see from the patch, the code will not try negotiate the pool in gstvaapidecode even though it get negotiated inside libgstvaapi. Am i missing something? :)
(In reply to sreerenj from comment #19) > Okay, let me try to give one example where your patch is going to fail ;) > > surface_resolution: 640x480 > crop_rectangle: 320x240 > > everything is working fine... > now in the middle there is a surface resolution change: > > new_surface_resolution: 1280x720 > new_crop_rectangle: 320x240 > > here the crop_values are the same, but actual surface dimension get changed. > As per what i can see from the patch, the code will not try negotiate the > pool in gstvaapidecode even though it get negotiated inside libgstvaapi. Nice catch! I will rework the patch :)
Created attachment 325604 [details] [review] simplify the downstream renegotiation This patch is a simplification of the last commits regarding the downstream renegotiation given the processed surface. This simplification is oriented to avoid the duplication of responsibilities with GstVideoDecoder super-class, in particular with those related with the pool negotiation.
Created attachment 325605 [details] [review] vaapidecode: remove spurious class variable active class variable was used as a flag to negotiate downstream once, but now that each time a new surface resolution is pop out a renegotation is done, this variable is not required anymore, thus removed.
Created attachment 325738 [details] [review] simplify the downstream renegotiation This patch is a simplification of the last commits regarding the downstream renegotiation given the processed surface. This simplification is oriented to avoid the duplication of responsibilities with GstVideoDecoder super-class, in particular with those related with the pool negotiation.
Created attachment 325739 [details] [review] vaapidecode: remove spurious class variable active class variable was used as a flag to negotiate downstream once, but now that each time a new surface resolution is pop out a renegotation is done, this variable is not required anymore, thus removed.
Just found a minor bug: the negotiated caps and the allocations caps should have the same color format. @sree, I guess I already addressed your concerns :)
Review of attachment 325738 [details] [review]: ::: gst/vaapi/gstvaapidecode.c @@ +429,3 @@ } + if (caps_changed || caps_changed) { caps_changed || allocation_changed
Review of attachment 325738 [details] [review]: ::: gst/vaapi/gstvaapidecode.c @@ +390,3 @@ + /* allocation caps changes if cropping differs */ + alloc_changed = output_width != surface_width + || output_height != surface_height; Are you intended to keep allocation_width/allocation_height zero until there is resolution change? if state==NULL && output_width == surface_width && output_height == surface_height then decode->allocation_width = decode->allocation_height = 0 which is misleading...
Review of attachment 325738 [details] [review]: ::: gst/vaapi/gstvaapidecode.c @@ +390,3 @@ + /* allocation caps changes if cropping differs */ + alloc_changed = output_width != surface_width + || output_height != surface_height; if state == NULL and if output size is equal to surface size, then the allocation size is zero. Thus, in gst_vaapidecode_update_src_caps() if (decode->allocation_width != 0 || decode->allocation_height != 0) { state->allocation_caps = ... This means that the state allocation caps is not assigned. Later, if in gstvideodecoder, if allocation_caps is not assigned (NULL) the negotiation caps are used for allocation query too. @@ +429,3 @@ } + if (caps_changed || caps_changed) { d'uh!!
(In reply to Víctor Manuel Jáquez Leal from comment #28) > + if (caps_changed || caps_changed) { > > d'uh!! Sorry, I made that change after running my tests because I thought that adding that log info would be interesting. But clearly I rebased it wrongly :S
Created attachment 325985 [details] [review] simplify the downstream renegotiation This patch is a simplification of the last commits regarding the downstream renegotiation given the processed surface. This simplification is oriented to avoid the duplication of responsibilities with GstVideoDecoder super-class, in particular with those related with the pool negotiation.
Created attachment 325986 [details] [review] vaapidecode: remove spurious class variable active class variable was used as a flag to negotiate downstream once, but now that each time a new surface resolution is pop out a renegotation is done, this variable is not required anymore, thus removed.
(In reply to Víctor Manuel Jáquez Leal from comment #28) > Review of attachment 325738 [details] [review] [review]: > > ::: gst/vaapi/gstvaapidecode.c > @@ +390,3 @@ > + /* allocation caps changes if cropping differs */ > + alloc_changed = output_width != surface_width > + || output_height != surface_height; > > if state == NULL and if output size is equal to surface size, then the > allocation size is zero. > > Thus, in gst_vaapidecode_update_src_caps() > > if (decode->allocation_width != 0 || decode->allocation_height != 0) { > state->allocation_caps = ... > > This means that the state allocation caps is not assigned. Later, if in > gstvideodecoder, if allocation_caps is not assigned (NULL) the negotiation > caps are used for allocation query too. > I understood it, but IMHO it is confusing for someone who read/experiment with the code. Even printing allocation_width/allocation_height (that too globally maintained) will show NULL values...
Also would be good if we can club the repetitive fields if possible. Saying this, since you are doing simplifications :) The patch is extending from it's initial simplified form, except it helping to avoid the decide_allocation_query() changes which is good of course) GstCaps *sinkpad_caps; GstCaps *srcpad_caps; guint output_width; guint output_height; guint allocation_width; guint allocation_height; GstVideoFormat allocation_format;
(In reply to sreerenj from comment #33) > Also would be good if we can club the repetitive fields if possible. Saying > this, since you are doing simplifications :) > The patch is extending from it's initial simplified form, except it helping > to avoid the decide_allocation_query() changes which is good of course) > > > GstCaps *sinkpad_caps; > GstCaps *srcpad_caps; > guint output_width; > guint output_height; > guint allocation_width; > guint allocation_height; > GstVideoFormat allocation_format; Ok. Agreed. Let me rework that.
Created attachment 326014 [details] [review] remove custom allocation query When resolving bug 753914, a custom allocation query was added, overlapping the responsibilities of GstVideoDecoder. But with the merge of the patches from bug 764421 this overlapping was not required anymore. This patch restores this situation setting the allocation_caps in the GstVideoCodecState when needed.
Created attachment 326015 [details] [review] vaapidecode: remove spurious class variables active, do_pool_renego and do_outstate_renego class variables were used to indicate when negotiate downstream once, but now that each time a new surface resolution is pop out a renegotation verified, these variable are not required anymore.
Created attachment 326016 [details] [review] vaapidecode: init {decoded,display}_info at open() It is required to initialize {decoded,display}_info variables when the decoder is open, not only at instance initialization.
Created attachment 326017 [details] [review] vaapidecode: code style fixes No functional changes.
Created attachment 326018 [details] [review] vaapidecode: always a valid format in decoded_info Always set a valid format in decoded_info class variable.
Created attachment 326019 [details] [review] vaapidecode: decoded_info is valid at src caps update As decoded_info is assured to be valid when gst_vaapidecode_update_src_caps() is called, then we don't need to verify or replace it with the sinkpad info (reference state).
Created attachment 326022 [details] [review] vaapidecode: keep only display_{width,height} Instead of keeping the structure GstVideoInfo when we are using its width and height, we only keep these two guints.
Created attachment 326023 [details] [review] vaapidecode: refactor is_display_resolution_changed() Make the comparisons more readable and simple.
I have split the bug into logical chunks, hopping to ease the review O:) Also, if you're not agree with a specific commit we can ditch it without pain.
Seems to be in good shape now... You can push and keep an eye on new bugs from QA ;)...
(In reply to sreerenj from comment #44) > Seems to be in good shape now... > You can push and keep an eye on new bugs from QA ;)... yuhu!
Attachment 326014 [details] pushed as 8169c6a - remove custom allocation query Attachment 326015 [details] pushed as 3478b27 - vaapidecode: remove spurious class variables Attachment 326016 [details] pushed as 80eb682 - vaapidecode: init {decoded,display}_info at open() Attachment 326017 [details] pushed as ade0c7b - vaapidecode: code style fixes Attachment 326018 [details] pushed as 3e97d71 - vaapidecode: always a valid format in decoded_info Attachment 326019 [details] pushed as 737fca4 - vaapidecode: decoded_info is valid at src caps update Attachment 326022 [details] pushed as 49a59f0 - vaapidecode: keep only display_{width,height} Attachment 326023 [details] pushed as aa16804 - vaapidecode: refactor is_display_resolution_changed()