Hi, On Thu, Jul 20, 2017 at 10:03:37AM +0200, Pavel Grunt wrote: > Victor, > > it is faster to write NACK You don't want feedback and rationale on the NACK? I wish you had discussed with me on my proposal as well as I don't agree that's wrong. > > 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 <quote> Wouldn't that make it possible to find a parser but not a decoder and evaluate the _has_codec() to true? </quote> Because, for sure, we know that we need decoders. Having the decoder and not a parser might produce a failure, that's one thing that I'm considering a okay-failure which should have proper fallback, etc. Having a false positive without decoders, that's bad (and you agreed). > > > 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 Yes, there is room for improvement there! I do think we should check at build time elements that we require (mainly from -plugins-base and plugins-good). We do check for decoders but that's only evaluate to a warning. Cheers, > > > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel