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...
>
>
> > >
> > > >
> > > > 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...
<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,
>
> >
> > 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 :)
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_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@xxxxxxxxxxxxxxxxxxxxx
> > > > > > > > > > > > > https://lists.freedesktop.org/mailman/listinfo/spice-dev
> > > > > > > > > > > > > el
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel