[Spice-devel] [PATCH v1 3/5] dcc-send: Encode and send gl_draw stream data to the remote client

[Frediano Ziglio]

Il giorno gio 16 mar 2023 alle ore 06:05 Vivek Kasireddy
<vivek.kasireddy at intel.com> ha scritto:
>
> For remote (or non-gl) clients, if a valid gl_draw stream exists,
> then we first extract the dmabuf fd associated with the scanout and
> share it with the encoder along with other key parameters such as
> stride, width and height. Once the encoder finishes creating an
> encoded buffer (using the dmabuf fd as input), we then send it
> over to the client. Also, as soon as the encoder notifies that
> it is no longer using the dmabuf fd, we send a gl_draw_done async
> to the application.
>
> Cc: Gerd Hoffmann <kraxel at redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau at redhat.com>
> Cc: Dongwon Kim <dongwon.kim at intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
> ---
>  server/dcc-send.cpp    | 89 +++++++++++++++++++++++++++++++++++++++++-
>  server/video-encoder.h | 13 ++++++
>  2 files changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/server/dcc-send.cpp b/server/dcc-send.cpp
> index 2c40a231..94cbe7e7 100644
> --- a/server/dcc-send.cpp
> +++ b/server/dcc-send.cpp
> @@ -1730,6 +1730,82 @@ static bool red_marshall_stream_data(DisplayChannelClient *dcc,
>      return TRUE;
>  }
>
> +static void red_gst_mem_free_cb(void *opaque)

I won't use "gst" here, this is for a generic encoder.

> +{
> +    auto dcc = static_cast<DisplayChannelClient *>(opaque);
> +    DisplayChannel *display = DCC_TO_DC(dcc);
> +
> +    dcc->priv->gl_draw_ongoing = FALSE;

minor, false

> +    display_channel_gl_draw_done(display);

This looks like a free() which is not freeing the object, other
callbacks in SPICE free the object.

> +}
> +
> +static void red_marshall_gl_draw_stream(DisplayChannelClient *dcc,
> +                                        SpiceMarshaller *base_marshaller)
> +{
> +    DisplayChannel *display = DCC_TO_DC(dcc);
> +    QXLInstance* qxl = display->priv->qxl;
> +    VideoEncoderDmabufData *dmabuf_data = nullptr;
> +
> +    VideoStream *stream = dcc->priv->gl_draw_stream;
> +    SpiceMsgDisplayGlScanoutUnix *scanout = red_qxl_get_gl_scanout(qxl);
> +    if (scanout != nullptr) {

"if (scanout)" suffice... not strong about.

> +        dmabuf_data = g_new0(VideoEncoderDmabufData, 1);

Use new/delete instead?

> +        dmabuf_data->drm_dma_buf_fd = scanout->drm_dma_buf_fd;
> +        dmabuf_data->width = stream->width;
> +        dmabuf_data->height = stream->height;
> +        dmabuf_data->stride = scanout->stride;
> +        dmabuf_data->opaque = dcc;

Why an opaque pointer? Better a proper typed pointer.

> +        dmabuf_data->notify_mem_free = red_gst_mem_free_cb;
> +    }
> +    red_qxl_put_gl_scanout(qxl, scanout);
> +    if (!dmabuf_data) {
> +        spice_warning("scanout is not valid");
> +        return;
> +    }
> +
> +    int stream_id = display_channel_get_video_stream_id(display, stream);
> +    VideoStreamAgent *agent = &dcc->priv->stream_agents[stream_id];
> +    uint32_t frame_mm_time = reds_get_mm_time();
> +    VideoBuffer *outbuf;
> +    VideoEncodeResults ret;
> +
> +    ret = !agent->video_encoder || !agent->video_encoder->encode_dmabuf ?
> +          VIDEO_ENCODER_FRAME_UNSUPPORTED :
> +          agent->video_encoder->encode_dmabuf(agent->video_encoder,
> +                                              frame_mm_time,
> +                                              dmabuf_data, &outbuf);
> +    switch (ret) {
> +    case VIDEO_ENCODER_FRAME_ENCODE_DONE:
> +        break;
> +    case VIDEO_ENCODER_FRAME_DROP:
> +#ifdef STREAM_STATS
> +        agent->stats.num_drops_fps++;
> +#endif
> +    case VIDEO_ENCODER_FRAME_UNSUPPORTED:
> +    default:
> +        spice_warning("bad return value (%d) from VideoEncoder::encode_dmabuf", ret);
> +        display_channel_gl_draw_done(display);

I don't know why but not setting dcc->priv->gl_draw_ongoing here smells bad.

> +        g_free(dmabuf_data);
> +        return;
> +    }
> +
> +    SpiceMsgDisplayStreamData stream_data;
> +
> +    dcc->init_send_data(SPICE_MSG_DISPLAY_STREAM_DATA);
> +    stream_data.base.id = stream_id;
> +    stream_data.base.multi_media_time = frame_mm_time;
> +    stream_data.data_size = outbuf->size;
> +    spice_marshall_msg_display_stream_data(base_marshaller, &stream_data);
> +
> +    spice_marshaller_add_by_ref_full(base_marshaller, outbuf->data, outbuf->size,
> +                                     &red_release_video_encoder_buffer, outbuf);
> +#ifdef STREAM_STATS
> +    agent->stats.num_frames_sent++;
> +    agent->stats.size_sent += outbuf->size;
> +    agent->stats.end = frame_mm_time;
> +#endif
> +}
> +
>  static inline void marshall_inval_palette(RedChannelClient *rcc,
>                                            SpiceMarshaller *base_marshaller,
>                                            RedCachePipeItem *cache_item)
> @@ -2126,6 +2202,8 @@ static void marshall_stream_start(DisplayChannelClient *dcc,
>      if (stream->current) {
>          RedDrawable *red_drawable = stream->current->red_drawable.get();
>          stream_create.clip = red_drawable->clip;
> +    } else if (stream == dcc->priv->gl_draw_stream){
> +        stream_create.clip.type = SPICE_CLIP_TYPE_NONE;

Is this necessary?

>      } else {
>          stream_create.clip.type = SPICE_CLIP_TYPE_RECTS;
>          clip_rects.num_rects = 0;
> @@ -2279,10 +2357,17 @@ static void marshall_gl_draw(RedChannelClient *rcc,
>                               SpiceMarshaller *m,
>                               RedPipeItem *item)
>  {
> +    DisplayChannelClient *dcc = DISPLAY_CHANNEL_CLIENT(rcc);

See fixups branch

>      auto p = static_cast<RedGlDrawItem*>(item);
>
> -    rcc->init_send_data(SPICE_MSG_DISPLAY_GL_DRAW);
> -    spice_marshall_msg_display_gl_draw(m, &p->draw);
> +    if (dcc_is_gl_client(dcc)) {
> +        rcc->init_send_data(SPICE_MSG_DISPLAY_GL_DRAW);
> +        spice_marshall_msg_display_gl_draw(m, &p->draw);
> +    } else if (dcc->priv->gl_draw_stream) {
> +        red_marshall_gl_draw_stream(dcc, m);
> +    } else {
> +        spice_warning("nothing to send to the client");
> +    }
>  }
>
>
> diff --git a/server/video-encoder.h b/server/video-encoder.h
> index d5bc0a68..8eb71ca1 100644
> --- a/server/video-encoder.h
> +++ b/server/video-encoder.h
> @@ -56,6 +56,15 @@ typedef struct VideoEncoderStats {
>      double avg_quality;
>  } VideoEncoderStats;
>
> +typedef struct VideoEncoderDmabufData {
> +    int32_t drm_dma_buf_fd;

See fixups branch

> +    uint32_t width;
> +    uint32_t height;
> +    uint32_t stride;
> +    void *opaque;

I would avoid the opaque and extend in dcc-send.

> +    void (*notify_mem_free)(void *opaque);

Here I would use a "void (*free)(struct VideoEncoderDmabufData*)".

> +} VideoEncoderDmabufData;
> +
>  typedef struct VideoEncoder VideoEncoder;
>  struct VideoEncoder {
>      /* Releases the video encoder's resources */
> @@ -84,6 +93,10 @@ struct VideoEncoder {
>                                         const SpiceRect *src, int top_down,
>                                         gpointer bitmap_opaque, VideoBuffer** outbuf);
>
> +    VideoEncodeResults (*encode_dmabuf)(VideoEncoder *encoder, uint32_t frame_mm_time,
> +                                        VideoEncoderDmabufData *dmabuf_data,
> +                                        VideoBuffer** outbuf);
> +
>      /*
>       * Bit rate control methods.
>       */

Frediano