From e00655c3c2d3f468251ed31908730f841ec1c2dd Mon Sep 17 00:00:00 2001 From: "zihao.jiang" Date: Fri, 10 Apr 2015 02:33:54 +0800 Subject: [PATCH] server/shadow: shadow encoder related enhancement/fix. 1. Export fps related API so that subsystem implementation no longer need to know about details in encoder structure. 2. Discard frameList dictionary. The 'value' in this dictionary is never used and not properly free'ed when client is disconnected. The dictionary was used to calculate 'inflight' frame count. Once an ACK is received from client, an item in the dictionary is removed. We then calculate 'inflight' frame by the count of the items in the dictionary. However, some rdp clients (win7 mstsc) skips frame ACK if it is inactive, ACK of some frame would actually never arrive. We actually don't need the dictionary. We only need to record the latest acknowledged frame id, and the difference between last sent frame id is the inflight frame count. 3. Minor fix in default fps calculation. encoder->frameAck is wrongly used as integer while it's actually bool flag. --- include/freerdp/server/shadow.h | 3 ++ server/shadow/Mac/mac_shadow.c | 3 +- server/shadow/X11/x11_shadow.c | 3 +- server/shadow/shadow_client.c | 21 ++++----- server/shadow/shadow_encoder.c | 83 +++++++++++++++------------------ server/shadow/shadow_encoder.h | 4 +- 6 files changed, 55 insertions(+), 62 deletions(-) diff --git a/include/freerdp/server/shadow.h b/include/freerdp/server/shadow.h index c5994b340..bb024db0e 100644 --- a/include/freerdp/server/shadow.h +++ b/include/freerdp/server/shadow.h @@ -279,6 +279,9 @@ FREERDP_API BOOL shadow_client_post_msg(rdpShadowClient* client, void* context, FREERDP_API int shadow_client_boardcast_msg(rdpShadowServer* server, void* context, UINT32 type, SHADOW_MSG_OUT* msg, void* lParam); FREERDP_API int shadow_client_boardcast_quit(rdpShadowServer* server, int nExitCode); +FREERDP_API int shadow_encoder_preferred_fps(rdpShadowEncoder* encoder); +FREERDP_API UINT32 shadow_encoder_inflight_frames(rdpShadowEncoder* encoder); + #ifdef __cplusplus } #endif diff --git a/server/shadow/Mac/mac_shadow.c b/server/shadow/Mac/mac_shadow.c index d3fbb77b0..519b06542 100644 --- a/server/shadow/Mac/mac_shadow.c +++ b/server/shadow/Mac/mac_shadow.c @@ -29,7 +29,6 @@ #include "../shadow_client.h" #include "../shadow_surface.h" #include "../shadow_capture.h" -#include "../shadow_encoder.h" #include "../shadow_subsystem.h" #include "../shadow_mcevent.h" @@ -377,7 +376,7 @@ void (^mac_capture_stream_handler)(CGDisplayStreamFrameStatus, uint64_t, IOSurfa if (client) { - subsystem->captureFrameRate = client->encoder->fps; + subsystem->captureFrameRate = shadow_encoder_preferred_fps(client->encoder); } } diff --git a/server/shadow/X11/x11_shadow.c b/server/shadow/X11/x11_shadow.c index c297683a8..da64e475a 100644 --- a/server/shadow/X11/x11_shadow.c +++ b/server/shadow/X11/x11_shadow.c @@ -42,7 +42,6 @@ #include "../shadow_screen.h" #include "../shadow_client.h" -#include "../shadow_encoder.h" #include "../shadow_capture.h" #include "../shadow_surface.h" #include "../shadow_subsystem.h" @@ -735,7 +734,7 @@ int x11_shadow_screen_grab(x11ShadowSubsystem* subsystem) if (client) { - subsystem->captureFrameRate = client->encoder->fps; + subsystem->captureFrameRate = shadow_encoder_preferred_fps(client->encoder); } } diff --git a/server/shadow/shadow_client.c b/server/shadow/shadow_client.c index 650101400..8aff1f337 100644 --- a/server/shadow/shadow_client.c +++ b/server/shadow/shadow_client.c @@ -357,17 +357,16 @@ BOOL shadow_client_activate(freerdp_peer* peer) BOOL shadow_client_surface_frame_acknowledge(rdpShadowClient* client, UINT32 frameId) { - SURFACE_FRAME* frame; - wListDictionary* frameList; + /* + * Record the last client acknowledged frame id to + * calculate how much frames are in progress. + * Some rdp clients (win7 mstsc) skips frame ACK if it is + * inactive, we should not expect ACK for each frame. + * So it is OK to calculate inflight frame count according to + * a latest acknowledged frame id. + */ + client->encoder->lastAckframeId = frameId; - frameList = client->encoder->frameList; - frame = (SURFACE_FRAME*) ListDictionary_GetItemValue(frameList, (void*) (size_t) frameId); - - if (frame) - { - ListDictionary_Remove(frameList, (void*) (size_t) frameId); - free(frame); - } return TRUE; } @@ -428,7 +427,7 @@ int shadow_client_send_surface_bits(rdpShadowClient* client, rdpShadowSurface* s } if (encoder->frameAck) - frameId = (UINT32) shadow_encoder_create_frame_id(encoder); + frameId = shadow_encoder_create_frame_id(encoder); if (settings->RemoteFxCodec) { diff --git a/server/shadow/shadow_encoder.c b/server/shadow/shadow_encoder.c index 65171e2a1..17ae041bf 100644 --- a/server/shadow/shadow_encoder.c +++ b/server/shadow/shadow_encoder.c @@ -24,15 +24,37 @@ #include "shadow_encoder.h" -int shadow_encoder_create_frame_id(rdpShadowEncoder* encoder) +int shadow_encoder_preferred_fps(rdpShadowEncoder* encoder) +{ + /* Return preferred fps calculated according to the last + * sent frame id and last client-acknowledged frame id. + */ + return encoder->fps; +} + +UINT32 shadow_encoder_inflight_frames(rdpShadowEncoder* encoder) +{ + /* Return inflight frame count = + * - + * Note: This function is exported so that subsystem could + * implement its own strategy to tune fps. + */ + return encoder->frameId - encoder->lastAckframeId; +} + +UINT32 shadow_encoder_create_frame_id(rdpShadowEncoder* encoder) { UINT32 frameId; int inFlightFrames; - SURFACE_FRAME* frame; - inFlightFrames = ListDictionary_Count(encoder->frameList); + inFlightFrames = shadow_encoder_inflight_frames(encoder); - if (inFlightFrames > encoder->frameAck) + /* + * Calculate preferred fps according to how much frames are + * in-progress. Note that it only works when subsytem implementation + * calls shadow_encoder_preferred_fps and takes the suggestion. + */ + if (inFlightFrames > 1) { encoder->fps = (100 / (inFlightFrames + 1) * encoder->maxFps) / 100; } @@ -47,16 +69,9 @@ int shadow_encoder_create_frame_id(rdpShadowEncoder* encoder) if (encoder->fps < 1) encoder->fps = 1; - frame = (SURFACE_FRAME*) malloc(sizeof(SURFACE_FRAME)); + frameId = ++encoder->frameId; - if (!frame) - return -1; - - frameId = frame->frameId = ++encoder->frameId; - if (!ListDictionary_Add(encoder->frameList, (void*) (size_t) frame->frameId, frame)) - return -1; - - return (int) frame->frameId; + return frameId; } int shadow_encoder_init_grid(rdpShadowEncoder* encoder) @@ -130,16 +145,11 @@ int shadow_encoder_init_rfx(rdpShadowEncoder* encoder) rfx_context_set_pixel_format(encoder->rfx, RDP_PIXEL_FORMAT_B8G8R8A8); - if (!encoder->frameList) - { - encoder->fps = 16; - encoder->maxFps = 32; - encoder->frameId = 0; - encoder->frameList = ListDictionary_New(TRUE); - if (!encoder->frameList) - return -1; - encoder->frameAck = settings->SurfaceFrameMarkerEnabled; - } + encoder->fps = 16; + encoder->maxFps = 32; + encoder->frameId = 0; + encoder->lastAckframeId = 0; + encoder->frameAck = settings->SurfaceFrameMarkerEnabled; encoder->codecs |= FREERDP_CODEC_REMOTEFX; @@ -159,16 +169,11 @@ int shadow_encoder_init_nsc(rdpShadowEncoder* encoder) nsc_context_set_pixel_format(encoder->nsc, RDP_PIXEL_FORMAT_B8G8R8A8); - if (!encoder->frameList) - { - encoder->fps = 16; - encoder->maxFps = 32; - encoder->frameId = 0; - encoder->frameList = ListDictionary_New(TRUE); - if (!encoder->frameList) - return -1; - encoder->frameAck = settings->SurfaceFrameMarkerEnabled; - } + encoder->fps = 16; + encoder->maxFps = 32; + encoder->frameId = 0; + encoder->lastAckframeId = 0; + encoder->frameAck = settings->SurfaceFrameMarkerEnabled; encoder->nsc->ColorLossLevel = settings->NSCodecColorLossLevel; encoder->nsc->ChromaSubsamplingLevel = settings->NSCodecAllowSubsampling ? 1 : 0; @@ -241,12 +246,6 @@ int shadow_encoder_uninit_rfx(rdpShadowEncoder* encoder) encoder->rfx = NULL; } - if (encoder->frameList) - { - ListDictionary_Free(encoder->frameList); - encoder->frameList = NULL; - } - encoder->codecs &= ~FREERDP_CODEC_REMOTEFX; return 1; @@ -260,12 +259,6 @@ int shadow_encoder_uninit_nsc(rdpShadowEncoder* encoder) encoder->nsc = NULL; } - if (encoder->frameList) - { - ListDictionary_Free(encoder->frameList); - encoder->frameList = NULL; - } - encoder->codecs &= ~FREERDP_CODEC_NSCODEC; return 1; diff --git a/server/shadow/shadow_encoder.h b/server/shadow/shadow_encoder.h index 34b8d0ef5..fd7d46a11 100644 --- a/server/shadow/shadow_encoder.h +++ b/server/shadow/shadow_encoder.h @@ -54,7 +54,7 @@ struct rdp_shadow_encoder int maxFps; BOOL frameAck; UINT32 frameId; - wListDictionary* frameList; + UINT32 lastAckframeId; }; #ifdef __cplusplus @@ -63,7 +63,7 @@ extern "C" { int shadow_encoder_reset(rdpShadowEncoder* encoder); int shadow_encoder_prepare(rdpShadowEncoder* encoder, UINT32 codecs); -int shadow_encoder_create_frame_id(rdpShadowEncoder* encoder); +UINT32 shadow_encoder_create_frame_id(rdpShadowEncoder* encoder); rdpShadowEncoder* shadow_encoder_new(rdpShadowClient* client); void shadow_encoder_free(rdpShadowEncoder* encoder);