[Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder
Pavel Grunt
pgrunt at redhat.com
Wed Jul 19 16:01:43 UTC 2017
On Wed, 2017-07-19 at 17:46 +0200, Victor Toso wrote:
> Hi,
>
> On Wed, Jul 19, 2017 at 05:27:43PM +0200, Pavel Grunt wrote:
> > > 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).
>
> With *host* creating the stream or are you testing something else? :)
Only the host (spice-server) creates the stream.
>
> With the following diff, I can test an error in the pipeline which
> should remove the stream, see:
>
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 3f51361..eb6a86b 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -511,6 +511,9 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder
> *video_decoder,
> return TRUE;
> }
>
> + if (video_decoder->codec_type == SPICE_VIDEO_CODEC_TYPE_H264)
> + return FALSE;
> +
> if (spice_mmtime_diff(frame->mm_time, decoder->last_mm_time) < 0) {
> SPICE_DEBUG("new-frame-time < last-frame-time (%u < %u):"
> " resetting stream",
>
> channel-display.c:1241 display-2:0: display_handle_stream_create: id 49
> channel-display.c:1497 display-2:0: video latency: 350
> channel-display.c:1563 display-2:0: destroy_display_stream: id=49 #in-frames=1
> out/in=1.00 #drops-on-receive=0 avg-late-time(ms)=0.00 #drops-on-playback=0
> spice-channel.c:2913 test cap 4 in 0x1052: yes report_invalid_stream: notify
> the server that stream 49 does not exist
> spice-channel.c:2913 test cap 4 in 0x1052: yes report_invalid_stream: notify
> the server that stream 49 does not exist
>
> GSpice-CRITICAL **: display_handle_stream_data: assertion 'st != NULL' failed
> decode-glz.c:352 decode_header: 1920x946, id 51, ref 49
> decode-glz.c:352 decode_header: 1920x946, id 52, ref 50
> decode-glz.c:352 decode_header: 1920x946, id 53, ref 51
> decode-glz.c:352 decode_header: 1920x946, id 54, ref 52
> decode-glz.c:352 decode_header: 1920x946, id 55, ref 53
>
> Just one critical, which is expected because we might have frames to
> decode while we already remove the decoder.
one critical per created stream, in case of youtube it may try to create more
streams
>
> > >
> > > We cannot guarantee that the pipeline will work smooth in all systems,
> > > all possible combinations.
> >
> > Victor, that is a different and unrelated issue
>
> Its not, because your patch is trying to make avdec_h264 work because of
> h264parse and that means you are trying to guarantee h264 to succeed if
> this two elements are present
>
no, I am trying to keep compatibility while not reporting
> >
> > >
> > > 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
>
> It is 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).
>
> Have you checked the commit log of that patch?
Where do you mention testing of those plugins????
>
> > 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
>
> What I'm trying to stress is that we are trying to not check *specific*
> plugins related to decode/parse a stream. We don't know what our users
> will be using and we should not enforce them to use avdec_h264 for
> instance.
I am not enforcing anything in this patch...
> You are doing this in this patch and that's why I'm against
> it.
Where am I enforcing it?
>
> I could be wrong that's why I'm waiting for someone else's feedback. I
> stated that we will not agree on this.
>
> I would take the patch to include parsers if it is agnostic to plugins
> as a workaround but I still don't think is the right way...
No, like in your patch it may lead to incorrect results.
>
> Cheers,
>
>
> >
> > > >
> > > > > 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_el
> > > > > > > > > > > > emen
> > > > > > > > > > > > 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-dev
> > > > > > > > > > > > el
More information about the Spice-devel
mailing list