Victor, it is faster to write NACK On Thu, 2017-07-20 at 07:26 +0200, Victor Toso wrote: > A new day, a new thread > > 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. > > By reading your commit log, your are fixing a single use case by > checking element's names that are hard coded to our codebase. > > The reason for the following two patches were to remove the usage of > hard coded names to check and play the stream. > > Shortlog: display-gst: Use Playbin for GStreamer 1.9.0 onwards > Commit : ed697dd7fa165632bd0cbdb34c971c8001c433fa > Victor Toso on Mon, 10 Jul 2017 17:34:23 +0200 > > Shortlog: display-gst: check GstRegistry for decoding elements > Commit : c9209aef946b3ad517e7794d2e5648caf5ee2bf9 > Victor Toso on Mon, 10 Jul 2017 17:33:10 +0200 > > So, this patch is going backwards in regard to one of the objectives of > playbin's introducing besides the fact that it does not guarantee a > solution for 3rd party plugins that might be filtered out. if going backwards means to not break compatibility with the previous release then yes, it goes backwards. > > So, Let's try to be clear about what's going on: > > The regression introduced by c9209aef946b3 is that avdec_h264 is filter > out in gstvideo_has_codec() and for that reason, for systems that do not > have any other decoder, our caps for decoding h264 will be disabled, > which is a regression. > > To fix the issue in a way that follows the objective of current code, we > should simply fix the filter as I've suggested at [0]. By the way, I > went to the GStreamer folks about this filtering issue and I understood > that my code in regard to the filtering is wrong due the assumption of > what is a subset. That's a bug alright. > > https://lists.freedesktop.org/archives/spice-devel/2017-July/038634.html > > Pavel, you saying that patch is incorrect is wrong. Victor, I cannot say it is correct when it does something incorrectly... > If you want, you can > say _incomplete_, but that's a fix. > > Incomplete you may say, because you think it is important to check for > h264parse element. I'm not against that Aren't you? > but I do think we, again, need > to approach #gstreamer folks and check with them that we want a more > robust check for enabling or not a streaming given a video-codec. If we > go this way and checking for parses seem important, we should do it in a > more generic fashion. > > Again, I would go with my patch that fixes an issue and see from now on > how to make our check more robust and I would not be surprise if there > is no way to guarantee 100% that checking every single element in a > pipeline would in fact decode a stream and that's because we don't have > the parameters for the stream itself embedded in our protocol besides > other issues that may occur. Victor, it is checking for the elements used for GST < 1.9 (consider el7, etc...) > > So, yes, if you remove h264parse from registry, we will say that the > stream can be decoded with avdec_h264 while it can't. I think that's a > weak argument Victor, you have used the same argument for my oneliner https://lists.freedesktop.org/archives/spice-devel/2017-July/038629.html > in favor of your patch and as I'm saying a few times now, > we should consider failure an option, fallback to image compression or > different streams with different video-codec types.... then you should remove the configure checks as well... (but it wasn't an intention of your playbin series) Have a nice day, Pavel > if the fallback > does not work, it is a bug even with your patch. > > Cheers, > > > > --- > > 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@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel