GNOME Bugzilla – Bug 754885
Re-enable passthrough mode when no longer scaling
Last modified: 2017-08-24 11:22:00 UTC
Created attachment 311149 [details] [review] Optimisation to unset GST_VAAPI_POSTPROC_FLAG_SIZE when scaling is no longer being performed I am using the vaapipostproc element to do scaling of video frames after they are decoded. I need the ability to dynamically change the scale as the video is playing. This is fine as I can change the width and height properties of vaapipostproc on the fly and then make a call to gst_base_transform_reconfigure_src to make sure the settings are applied. However when I set the width and height to 0 (i.e. no scaling) after having performed some scaling previously I noticed the vaapipostproc element would not return to pass-through mode as I expected. This is because currently there is no code to unset the GST_VAAPI_POSTPROC_FLAG_SIZE flag. I have a patch to fix the problem, I suspect there are similar issues with all of the other flags and could probably be fixed in a similar way but as I have not tested these cases I did not change them.
Thanks for the patch! I haven't tried it, but it looks correct. If you could complete the patch for all the properties, and upload it in git format would be great!!!
Review of attachment 311149 [details] [review]: reset all the properties and use a git format
Hello, sorry I forgot about this. Actually I'm not sure my code change is the right thing to do now that I look at doing it for the rest of the flags. The problem is that the flags is used for two things: 1) To mark that something has changed and the properties need to be applied to the filter 2) To see the post processing is needed or if it can go direct to pass-through For #2, we need to reset the flag when it goes back to default. But for #1 we still need the flag set so that the properties can be changed in the filter. I think my patch breaks down if you change lots of properties to non-default, then change some back to default (but not all of them). So you still need to do the post processing, but the properties that were reset back to default will not be passed onto the filter. Will probably need two separate flags? One flag to say if it is default or not, and another to say if something has changed. What do you think?
There seems to be other *huge* regressions in vaapipostproc. Let me fix those first... :)
(In reply to Martin Sherburn from comment #3) > The problem is that the flags is used for two things: > > 1) To mark that something has changed and the properties need to be applied > to the filter > 2) To see the post processing is needed or if it can go direct to > pass-through > > For #2, we need to reset the flag when it goes back to default. But for #1 > we still need the flag set so that the properties can be changed in the > filter. > > I think my patch breaks down if you change lots of properties to > non-default, then change some back to default (but not all of them). So you > still need to do the post processing, but the properties that were reset > back to default will not be passed onto the filter. > > Will probably need two separate flags? One flag to say if it is default or > not, and another to say if something has changed. What do you think? Thanks for reporting it, This is raising some interesting questions :) First of all we need to fix this: https://bugzilla.gnome.org/show_bug.cgi?id=758548 As you can see, right now we only change the size if there is an update_caps() (unlike other vpp properties). if #1 applied, it should always do postprocessing. Right now, we do not apply(post-processing) even the default values, unless we explicitly set the vpp properties. Unfortunately we are busy with fixing other high priority bugs for the immediate 0.7 release. So I am postponing this for next release...
Moving to Product:GStreamer, Component:gstreamer-vaapi
Created attachment 355383 [details] [review] postproc: reconfigure when width or height is changed
Review of attachment 355383 [details] [review]: ::: gst/vaapi/gstvaapipostproc.c @@ +1438,3 @@ case PROP_FORMAT: postproc->format = g_value_get_enum (value); + do_reconf = TRUE; what if format or width are the same? @@ +1445,3 @@ break; case PROP_HEIGHT: postproc->height = g_value_get_uint (value); shouldn't height be handled as the format and width?
Created attachment 355385 [details] [review] postproc: reconfigure when width or height is changed Sorry for wrong patch, my bad :(
Created attachment 355386 [details] [review] postproc: reconfigure when width or height is changed
Review of attachment 355386 [details] [review]: ::: gst/vaapi/gstvaapipostproc.c @@ +1444,2 @@ postproc->width = g_value_get_uint (value); + do_reconf = (prev_width != postproc->width) ? TRUE : FALSE; you can simplify this as do_reconf = (prev_width != postproc->width);
Comment on attachment 311149 [details] [review] Optimisation to unset GST_VAAPI_POSTPROC_FLAG_SIZE when scaling is no longer being performed @Hyunjun: is this patch still required?
Created attachment 355397 [details] [review] postproc: reconfigure when width or height is changed
(In reply to Víctor Manuel Jáquez Leal from comment #12) > Comment on attachment 311149 [details] [review] [review] > Optimisation to unset GST_VAAPI_POSTPROC_FLAG_SIZE when scaling is no longer > being performed > > @Hyunjun: is this patch still required? No, since it's been changed a bit by the commit https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/commit/?id=a01c0bc3
Created attachment 355475 [details] [review] tests: elements: add test sample of vaapipostproc
Review of attachment 355475 [details] [review]: ::: tests/elements/test-vaapipostproc.c @@ +17,3 @@ + gboolean ret; + + usleep (300000); why this sleep? it shouldn't be better to use g_timeout_add_seconds() to call this funciton? @@ +22,3 @@ + + if (ret) + g_print ("Now this pipeline is on passthrough mode\n"); I would use gst_println() @@ +34,3 @@ + static gfloat value = 1.0; + + value = value == 1.0 ? 0.5 : 1.0; uh? is this to iterate between 1 and 0.5? looks odd @@ +35,3 @@ + + value = value == 1.0 ? 0.5 : 1.0; + g_object_set (G_OBJECT (data->postproc), "contrast", value, NULL); it is not required to do that G_OBJECT() casting, since g_object_set accepts gpointer, thus it does the proper casting.
Review of attachment 355397 [details] [review]: ::: gst/vaapi/gstvaapipostproc.c @@ +1455,1 @@ case PROP_FORCE_ASPECT_RATIO: shouldn't we do the same for force_aspect_ratio and deinterlace_mode? i'm not sure regarding deinterlace_method...
(In reply to Víctor Manuel Jáquez Leal from comment #16) > Review of attachment 355475 [details] [review] [review]: > > @@ +34,3 @@ > + static gfloat value = 1.0; > + > + value = value == 1.0 ? 0.5 : 1.0; > > uh? is this to iterate between 1 and 0.5? looks odd > It just tries to confirm that it switches passthrough/non-passthrough. I'm following rest of your suggesstions.
Created attachment 355567 [details] [review] tests: elements: add test of vaapipostproc
(In reply to Víctor Manuel Jáquez Leal from comment #17) > Review of attachment 355397 [details] [review] [review]: > > ::: gst/vaapi/gstvaapipostproc.c > @@ +1455,1 @@ > case PROP_FORCE_ASPECT_RATIO: > > shouldn't we do the same for > > force_aspect_ratio and deinterlace_mode? > > i'm not sure regarding deinterlace_method... Hmm, Regarding force-aspect-ratio, I think it should be running according to caps of both side as is. Regarding deinterlace, I'm not sure either...
Attachment 355397 [details] pushed as 8a04f39 - postproc: reconfigure when width or height changes Attachment 355567 [details] pushed as dd78e03 - tests: elements: add test for vaapipostproc
Also pushed for branch 1.12 * 13a6c6e2 tests: elements: add test for vaapipostproc * c64e05d3 postproc: reconfigure when width or height changes