Re: [Spice-devel] [spice-gtk v1 3/4] channel-display-gst: improve check for decoder element

[Victor Toso]

Hi,

On Wed, Oct 19, 2016 at 06:26:18PM +0200, Francois Gouget wrote:
> On Wed, 19 Oct 2016, Victor Toso wrote:
> [...]
> > > But I don't think this necessarily impacts gstvideo_has_codec() if
> > > implemented using create_gstreamer_decoder() whereas with your code it
> > > will necessarily be impacted.
> >
> > Well, my point is simply why create the whole pipeline if we only need
> > to check if element is present. As I said above, it might even be enough
> > to check if we have hw decoding by adding vaapijpegdec, vaapih264dec,
> > and so on.
>
> So gstvideo_has_codec() will have to know about vaapijpegdec,
> vaapih264dec, and so on. And create_gstreamer_decoder() will need to
> know about them too. Whereas with the current code only
> create_gstreamer_decoder() has to know about them.
>
> > Once again, I don't really mind to drop this patch, I just want to
> > understand why it is error prone / not reliable.
>
> It relies on GStreamer elements being 'visible' only if their resources
> are available which is not how GStreamer worked up to August 2016
> (1.8.3). That it works this way in 1.9 for the vaapi set of decoders is
> nice, but will it be true of other hardware decoders too?

That's a good question. So, the current code makes it possible to detect
hw decoding for more versions of gstreamer... sounds good reason to me.

>
> Also are we sure we will not want to support hardware decoding with
> GStreamer 1.8? 1.8.3 is only a few months old.
>
> But really I don't see any tangible benefit to this change so I'm in
> favor of keeping the code simple.

I do. For me it seems simpler checking if an element exist instead of
creating the whole pipeline and then dropping it.

I'll remove this change for now and send a v2 with the other requested
changes.

Many thanks for the review and discussion,
  toso


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1 3/4] channel-display-gst: improve check for decoder element

[Francois Gouget]

On Wed, 19 Oct 2016, Victor Toso wrote:
[...]
> > But I don't think this necessarily impacts gstvideo_has_codec() if
> > implemented using create_gstreamer_decoder() whereas with your code it
> > will necessarily be impacted.
> 
> Well, my point is simply why create the whole pipeline if we only need
> to check if element is present. As I said above, it might even be enough
> to check if we have hw decoding by adding vaapijpegdec, vaapih264dec,
> and so on.

So gstvideo_has_codec() will have to know about vaapijpegdec, 
vaapih264dec, and so on. And create_gstreamer_decoder() will need to 
know about them too. Whereas with the current code only 
create_gstreamer_decoder() has to know about them.


> Once again, I don't really mind to drop this patch, I just want to
> understand why it is error prone / not reliable.

It relies on GStreamer elements being 'visible' only if their resources 
are available which is not how GStreamer worked up to August 2016 
(1.8.3). That it works this way in 1.9 for the vaapi set of decoders is 
nice, but will it be true of other hardware decoders too?

Also are we sure we will not want to support hardware decoding with 
GStreamer 1.8? 1.8.3 is only a few months old.

But really I don't see any tangible benefit to this change so I'm in 
favor of keeping the code simple.

-- 
Francois Gouget 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1 3/4] channel-display-gst: improve check for decoder element

[Victor Toso]

Hi,

On Wed, Oct 19, 2016 at 04:43:12PM +0200, Francois Gouget wrote:
> On Wed, 19 Oct 2016, Victor Toso wrote:
> [...]
> > > In a different context I have found that I can fnid dfbvideosink and
> > > even instantiate it. But it will refuse to switch to the READY state and
> > > is thus unusable. That makes sense since I was not using it in a Direct
> > > FB context.
> > 
> > Interesting. Was it possible to set a dfb context later on? Otherwise it
> > could be a bug, no?
> 
> I don't think that's a bug. The GStreamer documentation says:
> 
> * GST_STATE_NULL: this is the default state. No resources are allocated 
>   in this state, so, transitioning to it will free all resources. 
>   [...]
> 
> * GST_STATE_READY: in the ready state, an element has allocated all of 
>   its global resources, that is, resources that can be kept within 
>   streams. You can think about opening devices, allocating buffers and 
>   so on. However, the stream is not opened in this state, so the stream 
>   positions is automatically zero. If a stream was previously opened, it 
>   should be closed in this state, and position, properties and such 
>   should be reset. 
> 
> When the dfbvideosink is instantiated is is in the NULL state and since 
> no resource is allocated in this state it does not matter whether the 
> device is available or not. It's only when transitioning to the READY 
> state that it would try to open the relevant device and so that's where 
> it returns an error.

Right, thanks for clarifying.

> > > I expect hardware decoders will be even worse. For instance vaapidecode
> > > is likely to be present but may not work if you don't have an Intel
> > > card. Even if you can instantiate it, it may cause a VP8 pipeline caps
> > > negotiation to fail if your graphics does not support VP8.
> >
> > Yes, that's why I think we need a different approach for hardware
> > decoders.
>
> We will need code that can try the hardware decoder first and fallback 
> to the software one if it's not usable.

If I understand correctly what ceyusa said [0], the fact that we can
only find the vaapi hardware decoder element if hw supports it.

[0] https://lists.freedesktop.org/archives/spice-devel/2016-October/032825.html

> But I don't think this necessarily impacts gstvideo_has_codec() if
> implemented using create_gstreamer_decoder() whereas with your code it
> will necessarily be impacted.

Well, my point is simply why create the whole pipeline if we only need
to check if element is present. As I said above, it might even be enough
to check if we have hw decoding by adding vaapijpegdec, vaapih264dec,
and so on.

Once again, I don't really mind to drop this patch, I just want to
understand why it is error prone / not reliable.

  toso


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1 3/4] channel-display-gst: improve check for decoder element

[Francois Gouget]

On Wed, 19 Oct 2016, Victor Toso wrote:
[...]
> > In a different context I have found that I can fnid dfbvideosink and
> > even instantiate it. But it will refuse to switch to the READY state and
> > is thus unusable. That makes sense since I was not using it in a Direct
> > FB context.
> 
> Interesting. Was it possible to set a dfb context later on? Otherwise it
> could be a bug, no?

I don't think that's a bug. The GStreamer documentation says:

* GST_STATE_NULL: this is the default state. No resources are allocated 
  in this state, so, transitioning to it will free all resources. 
  [...]

* GST_STATE_READY: in the ready state, an element has allocated all of 
  its global resources, that is, resources that can be kept within 
  streams. You can think about opening devices, allocating buffers and 
  so on. However, the stream is not opened in this state, so the stream 
  positions is automatically zero. If a stream was previously opened, it 
  should be closed in this state, and position, properties and such 
  should be reset. 

When the dfbvideosink is instantiated is is in the NULL state and since 
no resource is allocated in this state it does not matter whether the 
device is available or not. It's only when transitioning to the READY 
state that it would try to open the relevant device and so that's where 
it returns an error.
 

> > I expect hardware decoders will be even worse. For instance vaapidecode
> > is likely to be present but may not work if you don't have an Intel
> > card. Even if you can instantiate it, it may cause a VP8 pipeline caps
> > negotiation to fail if your graphics does not support VP8.
> 
> Yes, that's why I think we need a different approach for hardware
> decoders.

We will need code that can try the hardware decoder first and fallback 
to the software one if it's not usable. But I don't think this 
necessarily impacts gstvideo_has_codec() if implemented using 
create_gstreamer_decoder() whereas with your code it will necessarily be 
impacted.


-- 
Francois Gouget   
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1 3/4] channel-display-gst: improve check for decoder element

[Victor Toso]

Hey,

On Wed, Oct 19, 2016 at 12:42:34PM +0200, Victor Toso wrote:
> Hi,
> 
> On Wed, Oct 19, 2016 at 12:11:53PM +0200, Francois Gouget wrote:
> > On Tue, 18 Oct 2016, Victor Toso wrote:
> > [...]
> > > +elements = g_strsplit(gst_opts[codec_type].dec_name, "!", 0);
> >
> > This would not work if one of the elements takes options.
> > (not the case right now but if we can keep the option open)
> 
> Indeed.
> 
> >
> > > +for (i = 0; elements[i] != NULL; i++) {
> > > +GstElementFactory *factory;
> > >
> > > -return has_codec;
> > > +factory = gst_element_factory_find(g_strstrip(elements[i]));
> > > +if (factory == NULL) {
> > > +SPICE_DEBUG("no element %s", elements[i]);
> > > +g_strfreev(elements);
> > > +return FALSE;
> > > +}
> >
> > This will likely not be an issue with software decoders but with
> > hardware ones that approach will not guarantee that the decoder is
> > usable.
> 
> That's correct but I think the same apply for the current code in regard
> of hw decoders - We might be able to create the pipeline at this time
> without errors but failed later on when we have a stream to decode.
> 
> IMHO, we will need a whole different approach for hardware decoder.
> 
> > In a different context I have found that I can fnid dfbvideosink and
> > even instantiate it. But it will refuse to switch to the READY state and
> > is thus unusable. That makes sense since I was not using it in a Direct
> > FB context.
> 
> Interesting. Was it possible to set a dfb context later on? Otherwise it
> could be a bug, no?
> 
> > I expect hardware decoders will be even worse. For instance vaapidecode
> > is likely to be present but may not work if you don't have an Intel
> > card. Even if you can instantiate it, it may cause a VP8 pipeline caps
> > negotiation to fail if your graphics does not support VP8.
> 
> Yes, that's why I think we need a different approach for hardware
> decoders. We should have an api in gstreamer-vaapi to tell what we can
> hw decode, I think (somewhat, a parser for vainfo command + validation
> with GStreamer vaapi elements).

I've asked this info to Víctor M. Jáques (ceyusa) in #gstreamer

11:48:04  toso | ceyusa: hey, is there a way to verify if we can hw
 decode using vaapi? some sort of api that translate
 what vainfo and validate with gstreamer-vaapi maybe?
11:48:51  toso | s/what vainfo/the output of vainfo/
11:48:54ceyusa | toso: using gstreamer-vaapi 1.9, only the available
 decoder entries are registered
11:49:19ceyusa | so, using a gst-inspect-1.0 you'll see only the
 available decoders
11:49:31  toso | ceyusa: and the available decoders takes in
 consideration my actual hw then?
11:49:42ceyusa | toso: yes
11:49:49  toso | ceyusa: awesome

Cheers,
  toso

>
> > Given that this code is not speed critical I don't think there's much
> > point in making it more complex and likely less reliable.
> 
> True, I don't really mind to drop it but I don't find it less reliable
> for software decoders and we don't have the correct handling for
> hardware decoders yet. So, all in all for me it is an improvement.
> 
> Thanks for your review!
>   toso




signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1 3/4] channel-display-gst: improve check for decoder element

[Victor Toso]

Hi,

On Wed, Oct 19, 2016 at 12:11:53PM +0200, Francois Gouget wrote:
> On Tue, 18 Oct 2016, Victor Toso wrote:
> [...]
> > +elements = g_strsplit(gst_opts[codec_type].dec_name, "!", 0);
>
> This would not work if one of the elements takes options.
> (not the case right now but if we can keep the option open)

Indeed.

>
> > +for (i = 0; elements[i] != NULL; i++) {
> > +GstElementFactory *factory;
> >
> > -return has_codec;
> > +factory = gst_element_factory_find(g_strstrip(elements[i]));
> > +if (factory == NULL) {
> > +SPICE_DEBUG("no element %s", elements[i]);
> > +g_strfreev(elements);
> > +return FALSE;
> > +}
>
> This will likely not be an issue with software decoders but with
> hardware ones that approach will not guarantee that the decoder is
> usable.

That's correct but I think the same apply for the current code in regard
of hw decoders - We might be able to create the pipeline at this time
without errors but failed later on when we have a stream to decode.

IMHO, we will need a whole different approach for hardware decoder.

> In a different context I have found that I can fnid dfbvideosink and
> even instantiate it. But it will refuse to switch to the READY state and
> is thus unusable. That makes sense since I was not using it in a Direct
> FB context.

Interesting. Was it possible to set a dfb context later on? Otherwise it
could be a bug, no?

> I expect hardware decoders will be even worse. For instance vaapidecode
> is likely to be present but may not work if you don't have an Intel
> card. Even if you can instantiate it, it may cause a VP8 pipeline caps
> negotiation to fail if your graphics does not support VP8.

Yes, that's why I think we need a different approach for hardware
decoders. We should have an api in gstreamer-vaapi to tell what we can
hw decode, I think (somewhat, a parser for vainfo command + validation
with GStreamer vaapi elements).

> Given that this code is not speed critical I don't think there's much
> point in making it more complex and likely less reliable.

True, I don't really mind to drop it but I don't find it less reliable
for software decoders and we don't have the correct handling for
hardware decoders yet. So, all in all for me it is an improvement.

Thanks for your review!
  toso


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1 3/4] channel-display-gst: improve check for decoder element

[Francois Gouget]

On Tue, 18 Oct 2016, Victor Toso wrote:
[...]
> +elements = g_strsplit(gst_opts[codec_type].dec_name, "!", 0);

This would not work if one of the elements takes options.
(not the case right now but if we can keep the option open)


> +for (i = 0; elements[i] != NULL; i++) {
> +GstElementFactory *factory;
>  
> -return has_codec;
> +factory = gst_element_factory_find(g_strstrip(elements[i]));
> +if (factory == NULL) {
> +SPICE_DEBUG("no element %s", elements[i]);
> +g_strfreev(elements);
> +return FALSE;
> +}

This will likely not be an issue with software decoders but with 
hardware ones that approach will not guarantee that the decoder is 
usable.

In a different context I have found that I can fnid dfbvideosink and 
even instantiate it. But it will refuse to switch to the READY state and 
is thus unusable. That makes sense since I was not using it in a Direct 
FB context.

I expect hardware decoders will be even worse. For instance vaapidecode 
is likely to be present but may not work if you don't have an Intel 
card. Even if you can instantiate it, it may cause a VP8 pipeline caps 
negotiation to fail if your graphics does not support VP8.

Given that this code is not speed critical I don't think there's much 
point in making it more complex and likely less reliable.


-- 
Francois Gouget 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-gtk v1 3/4] channel-display-gst: improve check for decoder element

[Victor Toso]

From: Victor Toso 

Instead of creating the whole pipeline and check for errors (which
would create and destroy the SpiceGstDecoder), let's use
gst_element_factory_find() function.

Signed-off-by: Victor Toso 
---
 src/channel-display-gst.c | 25 ++---
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 68ebd1f..c0f0a1e 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -484,13 +484,24 @@ VideoDecoder* create_gstreamer_decoder(int codec_type, 
display_stream *stream)
 G_GNUC_INTERNAL
 gboolean gstvideo_has_codec(int codec_type)
 {
-gboolean has_codec = FALSE;
+gint i;
+gchar **elements;
 
-VideoDecoder *decoder = create_gstreamer_decoder(codec_type, NULL);
-if (decoder) {
-has_codec = TRUE;
-decoder->destroy(decoder);
-}
+g_return_val_if_fail(gstvideo_init(), FALSE);
+g_return_val_if_fail(VALID_VIDEO_CODEC_TYPE(codec_type), FALSE);
+
+elements = g_strsplit(gst_opts[codec_type].dec_name, "!", 0);
+for (i = 0; elements[i] != NULL; i++) {
+GstElementFactory *factory;
 
-return has_codec;
+factory = gst_element_factory_find(g_strstrip(elements[i]));
+if (factory == NULL) {
+SPICE_DEBUG("no element %s", elements[i]);
+g_strfreev(elements);
+return FALSE;
+}
+gst_object_unref(factory);
+}
+g_strfreev(elements);
+return TRUE;
 }
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel