Il giorno gio 16 mar 2023 alle ore 06:05 Vivek Kasireddy
<vivek.kasireddy@xxxxxxxxx> 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@xxxxxxxxxx>
> Cc: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> Cc: Dongwon Kim <dongwon.kim@xxxxxxxxx>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> ---
> 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