[Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder
Victor Toso
victortoso at redhat.com
Wed Jul 19 14:43:35 UTC 2017
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/038634.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.
We cannot guarantee that the pipeline will work smooth in all systems,
all possible combinations.
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.
> > 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.
>
> > 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_elements);
> > > > > > > > > + 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170719/c2811f17/attachment.sig>
More information about the Spice-devel
mailing list