[Spice-devel] [spice-gtk v1 10/11] channel-display-gst: summarize number of frames dropped

[Victor Toso]

Hi,

On Tue, Apr 17, 2018 at 07:38:22AM -0400, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me at victortoso.com>
> > 
> > For example, this has produced 9 lines of debug below instead of 31.
> > 
> > GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 4
> > frames
> > GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 5
> > frames
> > GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 4
> > frames
> > GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 4
> > frames
> > GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 4
> > frames
> > GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 4
> > frames
> > GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 3
> > frames
> > GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 2
> > frames
> > GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 1
> > frames
> > 
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  src/channel-display-gst.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index d2847ec..72b5a16 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -207,6 +207,7 @@ static GstFlowReturn new_sample(GstAppSink *gstappsink,
> > gpointer video_decoder)
> >      GstSample *sample = gst_app_sink_pull_sample(decoder->appsink);
> >      if (sample) {
> >          GstBuffer *buffer = gst_sample_get_buffer(sample);
> > +        guint num_frames_dropped = 0;
> >          g_mutex_lock(&decoder->queues_mutex);
> >  
> >          /* gst_app_sink_pull_sample() sometimes returns the same buffer
> >          twice
> > @@ -235,13 +236,16 @@ static GstFlowReturn new_sample(GstAppSink *gstappsink,
> > gpointer video_decoder)
> >                      /* The GStreamer pipeline dropped the corresponding
> >                       * buffer.
> >                       */
> > -                    SPICE_DEBUG("the GStreamer pipeline dropped a frame");
> > +                    num_frames_dropped++;
> >                      free_gst_frame(gstframe);
> >                  }
> >                  break;
> >              }
> >              l = l->next;
> >          }
> > +        if (num_frames_dropped != 0) {
> > +            SPICE_DEBUG("the GStreamer pipeline dropped %u frames",
> > num_frames_dropped);
> > +        }
> >          if (!l) {
> >              spice_warning("got an unexpected decoded buffer!");
> >              gst_sample_unref(sample);
> 
> I think is more a bug (or lack) of protocol specification!
> 
> The protocol state (comments included):
> 
>     message {
>         StreamDataHeader base;
>         uint32 data_size;
>         uint8 data[data_size] @end @nomarshal;
>     } stream_data;
> 
> now... where is the "frame" ?? The client is expecting the "stream_data" to contain
> a full frame information and calls the stream_data message "frame" and report the
> problem. The server is currently splitting the byte stream into multiple chunks
> which is fine from the protocol specification and the client is complaining.
> I think we should start putting more efforts on protocol specification and
> avoid to having hidden assumption all over!
> 
> I would propose to just drop the SPICE_DEBUG line.

Looks good to me. Could you send the patch?

        toso
-------------- 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/20180418/81ae4dfb/attachment.sig>