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@xxxxxxxxxxxxxxxxxxxxx > > > > > > > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel