[Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder
Pavel Grunt
pgrunt at redhat.com
Wed Jul 19 16:22:00 UTC 2017
On Wed, 2017-07-19 at 18:13 +0200, Victor Toso wrote:
> On Wed, Jul 19, 2017 at 06:01:43PM +0200, Pavel Grunt wrote:
> > 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
>
> Did not understand.. You are trying to fix a regression but only for
> avdec_h264 case while our code is more generic now...
Yes, I am trying to fix the regression and avoid future regressions just before
the release
>
> >
> >
> > > >
> > > > >
> > > > > 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????
>
> Maybe we are talking about something different. I'm talking about
> testing my patch, which shows avdec_h264 with a simple line to fix the
> filtering...
>
By *they are not working* I am talking about "the other plugins" - does nvdec
work on your system, avdec_mjpeg? on my system it also shows openh264dec. I
tested these to decode streams created by spice-server
> <quote>
> In my system, our debug shows:
> .. gstvideo_debug_available_decoders: From 228 video decoder elements,
> - 4 can handle caps image/jpeg: jpegdec, nvdec, avdec_mjpeg, vaapijpegdec
> - 3 can handle caps video/x-vp8: vaapidecodebin, vp8dec, avdec_vp8
> - 4 can handle caps video/x-h264: vaapidecodebin, avdec_h264, nvdec,
> vaapih264dec
> - 3 can handle caps video/x-vp9: vaapidecodebin, vp9dec, avdec_vp9
> </quote>
>
> >
> >
> > >
> > > > 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...
>
> We were not using the elements name but your patch does use them.
> We only use that for gstreamer 1.9.0 and below and i hope to remove that
> after the release instead of depending on it.
>
> >
> > > You are doing this in this patch and that's why I'm against
> > > it.
> >
> > Where am I enforcing it?
>
> ...
> g_strsplit(gst_opts[codec_type].dec_name,
It does not enforce usage of the plugin at all. It *may* enable reporting the
capability to the server if there are elements specified in
*gst_opts[codec_type].dec_name* pipeline
>
> >
> > >
> > > 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.
>
> It is not incorrect Pavel and that's starting to hurt :)
sorry, it is incorrect to report a capability if you cannot handle it.
>
> I'm saying the pipeline can fail for lacking of elements/plugins and
> that's by design at this point.
>
> Cheers,
>
> >
> > >
> > > 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_name
> > > > > > > > > > > > > > s[i]
> > > > > > > > > > > > > > ));
> > > > > > > > > > > > > > + if (element == NULL) {
> > > > > > > > > > > > > > + gst_plugin_feature_list_free(decode
> > > > > > > > > > > > > > r_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