[Spice-devel] [PATCH spice-gtk] Update dropping frames debug messages

[Frediano Ziglio]

> 
> Hi,
> 
> 
> On 07/16/2018 10:52 AM, Frediano Ziglio wrote:
> >> Since non-mjpeg decoders are available in spice-gtk late frames
> >> are not always dropped
> > This is confusing, the reason is not simply other decoders but that
> > in some of the other decoders we support dropping data like this causes
> > terrible artifacts. Is also not exactly a problem of the decoder (as a
> > piece of software) but of the streaming format (so independently of the
> > software).
> 
> Hmm, indeed, inside my head it sounds more like - decoders that support
> codecs
> other than mjpeg (non-mjpeg decoders?) are not likely to drop these non-jpeg
> frames .
> 
> Will "Decoded frames are not necessarily dropped, update related debug
> messages
> and comment" be sufficient?
> 

sounds good for me.

> 
> >> ---
> >> In stats late frames are still counted & named as dropped meanwhile i
> >> leave it as is.
> >> ---
> >>   src/channel-display-mjpeg.c | 1 +
> >>   src/channel-display.c       | 3 ++-
> >>   2 files changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> >> index 7b2f775..6f7ded6 100644
> >> --- a/src/channel-display-mjpeg.c
> >> +++ b/src/channel-display-mjpeg.c
> >> @@ -262,6 +262,7 @@ static gboolean mjpeg_decoder_queue_frame(VideoDecoder
> >> *video_decoder,
> >>        * So drop late frames as early as possible to save on processing
> >>        time.
> >>        */
> >>       if (latency < 0) {
> >> +        SPICE_DEBUG("dropping a late MJPEG frame");
> >>           frame->free(frame);
> >>           return TRUE;
> >>       }
> >> diff --git a/src/channel-display.c b/src/channel-display.c
> >> index 44ba043..9a9bc99 100644
> >> --- a/src/channel-display.c
> >> +++ b/src/channel-display.c
> >> @@ -1544,11 +1544,12 @@ static void
> >> display_handle_stream_data(SpiceChannel
> >> *channel, SpiceMsgIn *in)
> >>   
> >>       latency = op->multi_media_time - mmtime;
> >>       if (latency < 0) {
> >> -        CHANNEL_DEBUG(channel, "stream data too late by %u ms (ts: %u,
> >> mmtime: %u), dropping",
> >> +        CHANNEL_DEBUG(channel, "stream data too late by %u ms (ts: %u,
> >> mmtime: %u)",
> >>                         mmtime - op->multi_media_time,
> >>                         op->multi_media_time,
> >>                         mmtime);
> >>           st->arrive_late_time += mmtime - op->multi_media_time;
> >>           st->arrive_late_count++;
> >>   
> >> +        /* Late frames are counted as drops in the stats but aren't
> >> necessarily dropped - depends on codec and decoder */
> > Line too long for style.
> > Would not be clearer to put this comment before the debug message?
> 
> Actually i put this comment here since it is counted in the stats as
> dropped, but it is not actually dropped, this naming may\should be
> changed in several components (or even changing the way stats are
> being collected in other codecs? idk)
> So meanwhile i have just leaved it as is and added this comment
> to make it clear (different patch?)
> 

Same patch is fine. Ok, make sense.

> >
> >>           if (!st->cur_drops_seq_stats.len) {
> >>               st->cur_drops_seq_stats.start_mm_time =
> >>               op->multi_media_time;
> >>           }
> Thanks
> 

Frediano