[Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder
Victor Toso
victortoso at redhat.com
Wed Jul 19 15:46:51 UTC 2017
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? :)
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.
> >
> > 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
>
> >
> > 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?
> 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. You are doing this in this patch and that's why I'm against
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...
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_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 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/c98f5e69/attachment.sig>
More information about the Spice-devel
mailing list