Forgot to ask if you hit these issues in practice, if so in which
scenario, and what happened before this patch series.
Christophe
On Tue, Nov 15, 2016 at 05:40:50PM +0100, Christophe Fergeau wrote:
> Missing commit log.
>
> On Mon, Oct 31, 2016 at 09:25:37PM +0100, Francois Gouget wrote:
> > Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
> > ---
> > src/channel-display-gst.c | 41 ++++++++++++++++++++++++++++++++++++-----
> > src/channel-display-mjpeg.c | 8 +++++---
> > src/channel-display-priv.h | 4 +++-
> > src/channel-display.c | 7 ++++++-
> > 4 files changed, 50 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index f52299f..5fb0b77 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -280,6 +280,28 @@ static void free_pipeline(SpiceGstDecoder *decoder)
> > decoder->pipeline = NULL;
> > }
> >
> > +static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer video_decoder)
> > +{
> > + SpiceGstDecoder *decoder = video_decoder;
> > +
> > + if (GST_MESSAGE_TYPE(msg) == GST_MESSAGE_ERROR) {
> > + GError *err = NULL;
> > + gchar *debug_info = NULL;
> > + gst_message_parse_error(msg, &err, &debug_info);
> > + spice_warning("GStreamer error from element %s: %s",
> > + GST_OBJECT_NAME(msg->src), err->message);
> > + if (debug_info) {
> > + SPICE_DEBUG("debug information: %s", debug_info);
> > + g_free(debug_info);
> > + }
> > + g_clear_error(&err);
> > +
> > + /* We won't be able to process any more frame anyway */
> > + free_pipeline(decoder);
> > + }
> > + return TRUE;
> > +}
> > +
> > static gboolean create_pipeline(SpiceGstDecoder *decoder)
> > {
> > gchar *desc;
> > @@ -324,6 +346,8 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
> >
> > appsink_cbs.new_sample = new_sample;
> > gst_app_sink_set_callbacks(decoder->appsink, &appsink_cbs, decoder, NULL);
> > + gst_bus_add_watch(gst_pipeline_get_bus(GST_PIPELINE(decoder->pipeline)),
> > + handle_pipeline_message, decoder);
> >
> > decoder->clock = gst_pipeline_get_clock(GST_PIPELINE(decoder->pipeline));
> >
> > @@ -390,9 +414,9 @@ static void release_buffer_data(gpointer data)
> > spice_msg_in_unref(frame_msg);
> > }
> >
> > -static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> > - SpiceMsgIn *frame_msg,
> > - int32_t latency)
> > +static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> > + SpiceMsgIn *frame_msg,
> > + int32_t latency)
> > {
> > SpiceGstDecoder *decoder = (SpiceGstDecoder*)video_decoder;
> >
> > @@ -400,7 +424,7 @@ static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> > uint32_t size = spice_msg_in_frame_data(frame_msg, &data);
> > if (size == 0) {
> > SPICE_DEBUG("got an empty frame buffer!");
> > - return;
> > + return TRUE;
> > }
> >
> > SpiceStreamDataHeader *frame_op = spice_msg_in_parsed(frame_msg);
> > @@ -419,7 +443,13 @@ static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> > * saves CPU so do it.
> > */
> > SPICE_DEBUG("dropping a late MJPEG frame");
> > - return;
> > + return TRUE;
> > + }
> > +
> > + if (decoder->pipeline == NULL) {
> > + /* An error occurred, causing the GStreamer pipeline to be freed */
> > + spice_warning("An error occurred, stopping the video stream");
> > + return FALSE;
> > }
> >
> > /* ref() the frame_msg for the buffer */
> > @@ -440,6 +470,7 @@ static void spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> > SPICE_DEBUG("GStreamer error: unable to push frame of size %u", size);
> > stream_dropped_frame_on_playback(decoder->base.stream);
> > }
> > + return TRUE;
> > }
> >
> > static gboolean gstvideo_init(void)
> > diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> > index 4976d53..67746c3 100644
> > --- a/src/channel-display-mjpeg.c
> > +++ b/src/channel-display-mjpeg.c
> > @@ -235,8 +235,9 @@ static void mjpeg_decoder_drop_queue(MJpegDecoder *decoder)
> >
> > /* ---------- VideoDecoder's public API ---------- */
> >
> > -static void mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
> > - SpiceMsgIn *frame_msg, int32_t latency)
> > +static gboolean mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
> > + SpiceMsgIn *frame_msg,
> > + int32_t latency)
> > {
> > MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
> > SpiceMsgIn *last_msg;
> > @@ -262,12 +263,13 @@ static void mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
> > * So drop late frames as early as possible to save on processing time.
> > */
> > if (latency < 0) {
> > - return;
> > + return TRUE;
> > }
> >
> > spice_msg_in_ref(frame_msg);
> > g_queue_push_tail(decoder->msgq, frame_msg);
> > mjpeg_decoder_schedule(decoder);
> > + return TRUE;
> > }
> >
> > static void mjpeg_decoder_reschedule(VideoDecoder *video_decoder)
> > diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> > index aa57f95..b9c08a3 100644
> > --- a/src/channel-display-priv.h
> > +++ b/src/channel-display-priv.h
> > @@ -50,8 +50,10 @@ struct VideoDecoder {
> > * @frame_msg: The Spice message containing the compressed frame.
> > * @latency: How long in milliseconds until the frame should be
> > * displayed. Negative values mean the frame is late.
> > + * @return: False if the decoder can no longer decode frames,
> > + * True otherwise.
> > */
> > - void (*queue_frame)(VideoDecoder *decoder, SpiceMsgIn *frame_msg, int32_t latency);
> > + gboolean (*queue_frame)(VideoDecoder *decoder, SpiceMsgIn *frame_msg, int32_t latency);
> >
> > /* The format of the encoded video. */
> > int codec_type;
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index 6cbbdd3..3d88cb0 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -1423,7 +1423,12 @@ static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn *in)
> > * decoding and best decide if/when to drop them when they are late,
> > * taking into account the impact on later frames.
> > */
> > - st->video_decoder->queue_frame(st->video_decoder, in, latency);
> > + if (!st->video_decoder->queue_frame(st->video_decoder, in, latency)) {
> > + destroy_stream(channel, op->id);
> > + /* Trigger the error reporting code */
> > + get_stream_by_id(channel, op->id);
>
> I'd prefer if this called a helper with a more descriptive name, which
> would make the comment not useful.
>
> Seems fine otherwise.
>
> Christophe
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel