[Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder
Pavel Grunt
pgrunt at redhat.com
Wed Jul 19 15:27:43 UTC 2017
On Wed, 2017-07-19 at 16:43 +0200, Victor Toso wrote:
> Hi,
>
> On Wed, Jul 19, 2017 at 04:25:19PM +0200, Pavel Grunt wrote:
> > On Wed, 2017-07-19 at 16:15 +0200, Victor Toso wrote:
> > > Hi,
> > >
> > > On Wed, Jul 19, 2017 at 04:01:21PM +0200, Pavel Grunt wrote:
> > > > On Wed, 2017-07-19 at 15:52 +0200, Victor Toso wrote:
> > > > > On Wed, Jul 19, 2017 at 03:46:36PM +0200, Pavel Grunt wrote:
> > > > > > On Wed, 2017-07-19 at 15:40 +0200, Victor Toso wrote:
> > > > > > > On Wed, Jul 19, 2017 at 03:30:49PM +0200, Pavel Grunt wrote:
> > > > > > > > On Wed, 2017-07-19 at 15:23 +0200, Victor Toso wrote:
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > On Wed, Jul 19, 2017 at 02:59:00PM +0200, Pavel Grunt wrote:
> > > > > > > > > > GStreamer's avdec_h264 needs h264parse to be able to process
> > > > > > > > > > H264
> > > > > > > > > > video
> > > > > > > > > > streams. However the check for elements through GstRegistry
> > > > > > > > > > does
> > > > > > > > > > not
> > > > > > > > > > take into account the possible pipeline of elements (like
> > > > > > > > > > "h264parse
> > > > > > > > > > !
> > > > > > > > > > avdec_h264").
> > > > > > > > > >
> > > > > > > > > > Fix that by checking for the elements by their name.
> > > > > > > > >
> > > > > > > > > As I mentioned before, given the extra complexity that this is
> > > > > > > > > requiring, I'm not convinced that it is a good fix because it
> > > > > > > > > relies
> > > > > > > > > on
> > > > > > > > > the fact that we know the elements that we need (hard coded).
> > > > > > > > >
> > > > > > > > > 1-) An user might have a different subset of plugins, like the
> > > > > > > > > fluendo
> > > > > > > > > ones or a different 3rd party ones. Do that work when we
> > > > > > > > > check
> > > > > > > > > for
> > > > > > > > > h264parse avdec_h264? I don't think so as it should be
> > > > > > > > > different
> > > > > > > > > plugins (one that people pay to have in their system)
> > > > > > > >
> > > > > > > > This checks for presence of all the requirred plugins, for both
> > > > > > > > h264parse and avdec_h264. the user must have both to detect it.
> > > > > > >
> > > > > > > I can have h264 stream decoded without avdec_h264 -> Not required
> > > > > > >
> > > > > > > I _might_ _still_ have failures due lack of plugins on runtime
> > > > > > > even
> > > > > > > with
> > > > > > > h264parse -> Not enough
> > > > > > >
> > > > > > > > It fixes the "regression" caused by the GstRegistry patch by
> > > > > > > > checking
> > > > > > > > for
> > > > > > > > the
> > > > > > > > elements like it was done before (but without parsing the full
> > > > > > > > pipeline)
> > > > > > >
> > > > > > > It does fix for this case. I would prefer to be agnostic about
> > > > > > > plugins,
> > > > > > > really.
> > > > > > >
> > > > > > > > >
> > > > > > > > > 2-) If playbin just needs an extra plugin, we don't check it
> > > > > > > > > and
> > > > > > > > > that
> > > > > > > > > means the pipeline will fail even if the caps were
> > > > > > > > > enabled.
> > > > > > > >
> > > > > > > > It is about the plugin combinations already tested in the past.
> > > > > > > > We
> > > > > > > > have
> > > > > > > > their
> > > > > > > > name (and their usage in a pipeline) in the header file
> > > > > > >
> > > > > > > That's correct but it doesn't mean that with playbin it'll be
> > > > > > > always
> > > > > > > correct as we are trying to increase the possibilities (i.e not
> > > > > > > depend
> > > > > > > solely on avdec_h264)
> > > > > > >
> > > > > > > I don't think we will agree because you want to check for
> > > > > > > h264parse
> > > > > > > and
> > > > > > > that's one thing I want to avoid.
> > > > > > >
> > > > > >
> > > > > > I do the check for the plugins defined in the header, if you remove
> > > > > > h264parse
> > > > > > from the header, it won't check for it
> > > > >
> > > > > Which would mean that [0] is much simpler, no?
> > > > >
> > > > > [0] https://lists.freedesktop.org/archives/spice-devel/2017-July/03863
> > > > > 4.ht
> > > > > ml
> > > > >
> > > >
> > > > It is simple, because it is incorrect. Remove h264parse and vaapi
> > > > plugins
> > > > and
> > > > keep avdec_h264, then try to connect to h264 streaming vm...
> > >
> > > The pipeline will fail and that's fine.
> >
> > yeah, but server will be sending a stream in h264 because the client
> > incorrectly
> > reported h264 cap. and client will render just a black screen, imho very bad
> > UX.
>
> Stream decoding will fail, server will know about it and sever should
> fallback to image compression or a different video codec format for
> streaming.
millions of criticals in the debug log and some resources wasted (Server will
keep trying to create streams - it does not know whether it is a temporary issue
or permanent).
>
> We cannot guarantee that the pipeline will work smooth in all systems,
> all possible combinations.
Victor, that is a different and unrelated issue
>
> IMHO, we should improve debug log to provide the information of what is
> lacking on GStreamer point of view instead of trying to guarantee that
> it will work.
unrelated
>
> > > You shouldn't do that in the
> > > first place.
> > >
> > > My point is to check for decoders only. If you want to check for
> > > parsers, this patch of yours will do it solely for hard coded elements.
> >
> > This patch is just try to keep working what used to work in the
> > previous release and prevent breaking it for the next release (which
> > seems to happen soon).
>
> My patch should work as well and possibly for other plugins like
> avdec_h264 that don't show up due bad filtering.
>
Have you tested that? They are not working (I'm using playbin).
We do have the check for the same elements in configure, I really don't
understand what is wrong about doing the same check in runtime (considerring
TODO and keeping the compatibility with the last release).
Pavel
> >
> > > You would need to do something nicer using GStreamer API
> >
> > That is the reason for TODO
> >
> > > to be agnostic
> > > of specific parser elements.
> > >
> > > And yet, we could miss something else now or in one year.
> > >
> > > Yes, my take is to check for decoders and avoid this workarounds.
> > >
> > > >
> > > >
> > > > > >
> > > > > >
> > > > > > > > >
> > > > > > > > > I do think the right approach is as I mentioned before, fix
> > > > > > > > > the
> > > > > > > > > filtering to show all decoders for h264 and that's it. If the
> > > > > > > > > pipeline
> > > > > > > > > fails, that's fine because it is *hard* to *ensure* that it'll
> > > > > > > > > work
> > > > > > > > > anyway without an actual video stream.
> > > > > > > > >
> > > > > > > > > > ---
> > > > > > > > > > src/channel-display-gst.c | 25 +++++++++++++++++++++++++
> > > > > > > > > > 1 file changed, 25 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/src/channel-display-gst.c b/src/channel-
> > > > > > > > > > display-
> > > > > > > > > > gst.c
> > > > > > > > > > index 5042fc8..a9a044a 100644
> > > > > > > > > > --- a/src/channel-display-gst.c
> > > > > > > > > > +++ b/src/channel-display-gst.c
> > > > > > > > > > @@ -666,6 +666,31 @@ gboolean gstvideo_has_codec(int
> > > > > > > > > > codec_type)
> > > > > > > > > > codec_decoders =
> > > > > > > > > > gst_element_factory_list_filter(all_decoders,
> > > > > > > > > > caps,
> > > > > > > > > > GST_PAD_SINK, TRUE);
> > > > > > > > > > gst_caps_unref(caps);
> > > > > > > > > >
> > > > > > > > > > + /* Check for the presence of decoding elements that
> > > > > > > > > > could
> > > > > > > > > > be
> > > > > > > > > > filter
> > > > > > > > > > out.
> > > > > > > > > > + * TODO: Improve filtering to avoid this...
> > > > > > > > > > + */
> > > > > > > > > > + {
> > > > > > > > > > + GList *decoder_elements = NULL;
> > > > > > > > > > + gchar **decoders_by_names;
> > > > > > > > > > + guint i = 0;
> > > > > > > > > > + decoders_by_names =
> > > > > > > > > > g_strsplit(gst_opts[codec_type].dec_name,
> > > > > > > > > > "!",
> > > > > > > > > > -1);
> > > > > > > > > > + for (i = 0; decoders_by_names[i] != NULL; i++) {
> > > > > > > > > > + GstElementFactory *element =
> > > > > > > > > > gst_element_factory_find(g_strstrip(decoders_by_names[i]));
> > > > > > > > > > + if (element == NULL) {
> > > > > > > > > > + gst_plugin_feature_list_free(decoder_elemen
> > > > > > > > > > ts);
> > > > > > > > > > + decoder_elements = NULL;
> > > > > > > > > > + break;
> > > > > > > > > > + }
> > > > > > > > > > + if (g_list_find(codec_decoders, element)) {
> > > > > > > > > > + gst_object_unref(element);
> > > > > > > > > > + } else {
> > > > > > > > > > + decoder_elements =
> > > > > > > > > > g_list_append(decoder_elements,
> > > > > > > > > > element);
> > > > > > > > > > + }
> > > > > > > > > > + }
> > > > > > > > > > + codec_decoders = g_list_concat(codec_decoders,
> > > > > > > > > > decoder_elements);
> > > > > > > > > > + g_strfreev(decoders_by_names);
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > if (codec_decoders == NULL) {
> > > > > > > > > > spice_debug("From %u decoders, none can handle
> > > > > > > > > > '%s'",
> > > > > > > > > > g_list_length(all_decoders),
> > > > > > > > > > gst_opts[codec_type].dec_caps);
> > > > > > > > > > --
> > > > > > > > > > 2.13.3
> > > > > > > > > >
> > > > > > > > > > _______________________________________________
> > > > > > > > > > Spice-devel mailing list
> > > > > > > > > > Spice-devel at lists.freedesktop.org
> > > > > > > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list