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.

diff --git a/include/freerdp/server/shadow.h b/include/freerdp/server/shadow.h
index c5994b3..bb024db 100644 (file)
--- 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 d3fbb77..519b065 100644 (file)
--- 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 c297683..da64e47 100644 (file)
--- 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 6501014..8aff1f3 100644 (file)
--- 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 65171e2..17ae041 100644 (file)
--- 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 = 
+        * <last sent frame id> - <last client-acknowledged frame id>
+        * 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 34b8d0e..fd7d46a 100644 (file)
--- 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);