Re: gst-plugins-good: videocrop mark crop properties as mutable in playing state

[Nicolas Dufresne-4]

Le lundi 23 mai 2016 à 18:17 +0000, Tim Müller a écrit :

> Module: gst-plugins-good
> Branch: master
> Commit: 3d979d4e879f1c18ffa64ecfd9887669eba97220
> URL:    http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit
> /?id=3d979d4e879f1c18ffa64ecfd9887669eba97220
>
> Author: Tim-Philipp Müller <[hidden email]>
> Date:   Sun May 22 20:14:18 2016 +0100
>
> videocrop mark crop properties as mutable in playing state
>

Last time I tested, changing those in playing state lead to a crash or
spurious negotiation error. I had to use a probe in front to safely be
able to change this.

Nicolas

> ---
>
>  gst/videocrop/gstvideocrop.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/gst/videocrop/gstvideocrop.c
> b/gst/videocrop/gstvideocrop.c
> index 511161e..d9a8f1a 100644
> --- a/gst/videocrop/gstvideocrop.c
> +++ b/gst/videocrop/gstvideocrop.c
> @@ -180,19 +180,23 @@ gst_video_crop_class_init (GstVideoCropClass *
> klass)
>    g_object_class_install_property (gobject_class, PROP_LEFT,
>        g_param_spec_int ("left", "Left",
>            "Pixels to crop at left (-1 to auto-crop)", -1, G_MAXINT,
> 0,
> -          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
> +          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS |
> +          GST_PARAM_MUTABLE_PLAYING));
>    g_object_class_install_property (gobject_class, PROP_RIGHT,
>        g_param_spec_int ("right", "Right",
>            "Pixels to crop at right (-1 to auto-crop)", -1, G_MAXINT,
> 0,
> -          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
> +          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS |
> +          GST_PARAM_MUTABLE_PLAYING));
>    g_object_class_install_property (gobject_class, PROP_TOP,
> -      g_param_spec_int ("top", "Top",
> -          "Pixels to crop at top (-1 to auto-crop)", -1, G_MAXINT,
> 0,
> -          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
> +      g_param_spec_int ("top", "Top", "Pixels to crop at top (-1 to
> auto-crop)",
> +          -1, G_MAXINT, 0,
> +          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS |
> +          GST_PARAM_MUTABLE_PLAYING));
>    g_object_class_install_property (gobject_class, PROP_BOTTOM,
>        g_param_spec_int ("bottom", "Bottom",
>            "Pixels to crop at bottom (-1 to auto-crop)", -1,
> G_MAXINT, 0,
> -          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
> +          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS |
> +          GST_PARAM_MUTABLE_PLAYING));
>  
>    gst_element_class_add_static_pad_template (element_class,
> &sink_template);
>    gst_element_class_add_static_pad_template (element_class,
> &src_template);
>
> _______________________________________________
> gstreamer-commits mailing list
> [hidden email]
> https://lists.freedesktop.org/mailman/listinfo/gstreamer-commits

_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

[Arjen Veenhuizen]

@Nicolas: are you referring to videocrop or videobox? Videocrop does not crash on me when changing its properties on the fly, for videobox it is a whole different SEGFAULT story...

[Tim-Philipp Müller-2]

On Mon, 2016-05-23 at 20:24 -0400, Nicolas Dufresne wrote:

> > 
> > videocrop mark crop properties as mutable in playing state
> >
> Last time I tested, changing those in playing state lead to a crash
> or spurious negotiation error. I had to use a probe in front to
> safely be able to change this.

It will depend on the pipeline I think? Were the crashes in videocrop?

./videocrop2-test changes these properties at runtime and that seems to
work fine for me. Not that I'd be surprised if there are bugs given the
async nature of this all.

I see there's https://bugzilla.gnome.org/show_bug.cgi?id=761163 of
course, but it's a bit all over the place really - as far as I can tell
vieocrop already tries to queue the changes, but perhaps not in all
places where it should? 

Are you suggesting we don't mark them as mutable because it's not
necessarily 100% bulletproof in all scenarios even though it mostly
works fine? I don't really mind, I was just under the impression that
it should work in principle and remembered the test programs we had
worked fine, and someone asked about it on irc :)

Cheers
 -Tim
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

[Nicolas Dufresne-5]

Le mardi 24 mai 2016 à 10:03 +0100, Tim-Philipp Müller a écrit :

> On Mon, 2016-05-23 at 20:24 -0400, Nicolas Dufresne wrote:
>
> >
> > >
> > >  
> > > videocrop mark crop properties as mutable in playing state
> > >
> > Last time I tested, changing those in playing state lead to a crash
> > or spurious negotiation error. I had to use a probe in front to
> > safely be able to change this.
> It will depend on the pipeline I think? Were the crashes in
> videocrop?
>
> ./videocrop2-test changes these properties at runtime and that seems
> to
> work fine for me. Not that I'd be surprised if there are bugs given
> the
> async nature of this all.
>
> I see there's https://bugzilla.gnome.org/show_bug.cgi?id=761163 of
> course, but it's a bit all over the place really - as far as I can
> tell
> vieocrop already tries to queue the changes, but perhaps not in all
> places where it should? 

There is code that tries indeed. But it needs some more work. Basically
it needs to queue the changes until they are applied, while atm they
are applied at multiple places, which can lead to inconstancy (and
sometimes crash). Another issue, is that changing one crop property at
the time lead to bunch of caps renegotiation, to be really useful, we'd
be able to change all of these properties atomically.

>
> Are you suggesting we don't mark them as mutable because it's not
> necessarily 100% bulletproof in all scenarios even though it mostly
> works fine? I don't really mind, I was just under the impression that
> it should work in principle and remembered the test programs we had
> worked fine, and someone asked about it on irc :)

I'm suggesting that marking them as mutable is a bad indication for the
user. As least without, it's clear that it's an untested domain. You
could have fixed the bugs first, that would have been nicer. At the
same time, I suppose this won't crash with controllers, since their
behaviors are similar to using a buffer probe on the sink pad, and
setting the values within that probe callback.

regards,
Nicolas
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

[Nicolas Dufresne-4]

Le mardi 24 mai 2016 à 01:16 -0700, Arjen Veenhuizen a écrit :
> @Nicolas: are you referring to videocrop or videobox? Videocrop does
> not
> crash on me when changing its properties on the fly, for videobox it
> is a
> whole different SEGFAULT story...

It does crash if you are agressive enough. It's a known race in the
code. There is some code to try and delay the changes, but it was never
complete. It's actually a corner case of base transform. If not, you
often endup with caps that cannot be negotiated, as an intermediate
caps get pushed (changing those properties are not atomic). This can of
cours be worked around by using a buffer probe, and only setting the
properties in the probe callback. I which someone would have taken the
time to fix before advertising it as good to change in playing state.

Nicolas
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/gstreamer-devel