From 32e64c1e98d9218944a054a5046fc012c15f38aa Mon Sep 17 00:00:00 2001 From: David Fort Date: Thu, 4 Dec 2025 13:14:09 +0100 Subject: [PATCH] rdpecam: fix camera sample grabbing Before this patch we had a behavior where there was a credit of 8 samples that could be sent to the server with no corresponding sample request. So in the right conditions, we were having situations where the server was receiving samples that it has not requested, and so it was dropping them. The visible effect was small artifacts in the camera stream when i-frames where dropped, and more serious ones when the dropped content was containing key frames. This issue has also been reported when xfreerdp connects on g-r-d as #11990. This patch reworks the frame grabbing workflow: when the frame grabbing thread calls the sample callback we check if a sample is already pending, waiting to be sent to the server. If that's the case and the camera's input format supports frame dropping we just refresh the pending frame with the new one. If the input format can't drop frames (like with h264 and mjpg) we wait until the current pending frame is sent. So now frames can be sent either when we receive a sample request from the server, or when the sample callback is invoked. --- channels/rdpecam/client/camera.h | 5 +- channels/rdpecam/client/camera_device_main.c | 185 +++++++++++++++---- 2 files changed, 150 insertions(+), 40 deletions(-) diff --git a/channels/rdpecam/client/camera.h b/channels/rdpecam/client/camera.h index d813389f8..f0f4f9d9a 100644 --- a/channels/rdpecam/client/camera.h +++ b/channels/rdpecam/client/camera.h @@ -101,7 +101,10 @@ typedef struct CAM_MEDIA_TYPE_DESCRIPTION currMediaType; GENERIC_CHANNEL_CALLBACK* hSampleReqChannel; - INT nSampleCredits; + CRITICAL_SECTION lock; + volatile LONG samplesRequested; + wStream* pendingSample; + volatile BOOL haveSample; wStream* sampleRespBuffer; H264_CONTEXT* h264; diff --git a/channels/rdpecam/client/camera_device_main.c b/channels/rdpecam/client/camera_device_main.c index 0f992d151..10a9cb990 100644 --- a/channels/rdpecam/client/camera_device_main.c +++ b/channels/rdpecam/client/camera_device_main.c @@ -19,6 +19,7 @@ #include #include +#include #include "camera.h" @@ -108,30 +109,28 @@ static UINT ecam_dev_send_sample_response(CameraDevice* dev, size_t streamIndex, FALSE /* don't free stream */); } -/** - * Function description - * - * @return 0 on success, otherwise a Win32 error code - */ -static UINT ecam_dev_sample_captured_callback(CameraDevice* dev, int streamIndex, - const BYTE* sample, size_t size) +static BOOL mediaSupportDrops(CAM_MEDIA_FORMAT format) +{ + switch (format) + { + case CAM_MEDIA_FORMAT_H264: + case CAM_MEDIA_FORMAT_MJPG: + return FALSE; + default: + return TRUE; + } +} + +static UINT ecam_dev_send_pending(CameraDevice* dev, int streamIndex, CameraDeviceStream* stream) { BYTE* encodedSample = NULL; size_t encodedSize = 0; - WINPR_ASSERT(dev); - - if (streamIndex >= ECAM_DEVICE_MAX_STREAMS) - return ERROR_INVALID_INDEX; - - CameraDeviceStream* stream = &dev->streams[streamIndex]; - - if (!stream->streaming) - return CHANNEL_RC_OK; - if (streamInputFormat(stream) != streamOutputFormat(stream)) { - if (!ecam_encoder_compress(stream, sample, size, &encodedSample, &encodedSize)) + if (!ecam_encoder_compress(stream, Stream_Buffer(stream->pendingSample), + Stream_Length(stream->pendingSample), &encodedSample, + &encodedSize)) { WLog_DBG(TAG, "Frame drop or error in ecam_encoder_compress"); return CHANNEL_RC_OK; @@ -142,21 +141,87 @@ static UINT ecam_dev_sample_captured_callback(CameraDevice* dev, int streamIndex } else /* passthrough */ { - encodedSample = WINPR_CAST_CONST_PTR_AWAY(sample, BYTE*); - encodedSize = size; + encodedSample = Stream_Buffer(stream->pendingSample); + encodedSize = Stream_Length(stream->pendingSample); } - if (stream->nSampleCredits == 0) - { - WLog_DBG(TAG, "Skip sample: no credits left"); - return CHANNEL_RC_OK; - } - stream->nSampleCredits--; + stream->samplesRequested--; + stream->haveSample = FALSE; return ecam_dev_send_sample_response(dev, WINPR_ASSERTING_INT_CAST(size_t, streamIndex), encodedSample, encodedSize); } +static UINT ecam_dev_sample_captured_callback(CameraDevice* dev, int streamIndex, + const BYTE* sample, size_t size) +{ + WINPR_ASSERT(dev); + + if (streamIndex >= ECAM_DEVICE_MAX_STREAMS) + return ERROR_INVALID_INDEX; + + CameraDeviceStream* stream = &dev->streams[streamIndex]; + + if (!stream->streaming) + return CHANNEL_RC_OK; + + EnterCriticalSection(&stream->lock); + UINT ret = CHANNEL_RC_NO_MEMORY; + + /* If we already have a waiting sample, let's see if the input format support dropping + * frames so that we could just "refresh" the pending sample, otherwise we must wait until + * a frame request flushes it + */ + + if (stream->haveSample && !mediaSupportDrops(stream->formats.inputFormat)) + { + /* we can't drop samples, so we have to wait until the pending sample is + * sent, by a sample request. + * + * When we're here we already have a sample ready to be sent, the delay between 2 frames + * seems like a reasonable wait delay. For instance 60 FPS means a frame every 16ms. + * We also cap that wait delay to not spinloop and not get stuck for too long. + * */ + DWORD waitDelay = (1000 * stream->currMediaType.FrameRateDenominator) / + stream->currMediaType.FrameRateNumerator; + if (waitDelay < 16) + waitDelay = 16; + if (waitDelay > 100) + waitDelay = 100; + + while (stream->haveSample && stream->streaming) + { + LeaveCriticalSection(&stream->lock); + + SleepEx(waitDelay, TRUE); + + EnterCriticalSection(&stream->lock); + } + + if (!stream->streaming) + { + ret = CHANNEL_RC_OK; + goto out; + } + } + + Stream_SetPosition(stream->pendingSample, 0); + if (!Stream_EnsureRemainingCapacity(stream->pendingSample, size)) + goto out; + + Stream_Write(stream->pendingSample, sample, size); + Stream_SealLength(stream->pendingSample); + stream->haveSample = TRUE; + + ret = CHANNEL_RC_OK; + if (stream->samplesRequested) + ret = ecam_dev_send_pending(dev, streamIndex, stream); + +out: + LeaveCriticalSection(&stream->lock); + return ret; +} + static void ecam_dev_stop_stream(CameraDevice* dev, size_t streamIndex) { WINPR_ASSERT(dev); @@ -170,6 +235,8 @@ static void ecam_dev_stop_stream(CameraDevice* dev, size_t streamIndex) { stream->streaming = FALSE; dev->ihal->StopStream(dev->ihal, dev->deviceId, 0); + + DeleteCriticalSection(&stream->lock); } if (stream->sampleRespBuffer) @@ -178,6 +245,12 @@ static void ecam_dev_stop_stream(CameraDevice* dev, size_t streamIndex) stream->sampleRespBuffer = NULL; } + if (stream->pendingSample) + { + Stream_Free(stream->pendingSample, TRUE); + stream->pendingSample = NULL; + } + ecam_encoder_context_free(stream); } @@ -267,7 +340,25 @@ static UINT ecam_dev_process_start_streams_request(CameraDevice* dev, /* replacing outputFormat with inputFormat in mediaType before starting stream */ mediaType.Format = streamInputFormat(stream); - stream->nSampleCredits = 0; + stream->samplesRequested = 0; + stream->haveSample = FALSE; + + if (!InitializeCriticalSectionEx(&stream->lock, 0, 0)) + { + WLog_ERR(TAG, "InitializeCriticalSectionEx failed"); + ecam_dev_stop_stream(dev, streamIndex); + ecam_channel_send_error_response(dev->ecam, hchannel, CAM_ERROR_CODE_OutOfMemory); + return ERROR_INVALID_DATA; + } + + stream->pendingSample = Stream_New(NULL, mediaType.Width * mediaType.Height * 4ull); + if (!stream->pendingSample) + { + WLog_ERR(TAG, "pending stream failed"); + ecam_dev_stop_stream(dev, streamIndex); + ecam_channel_send_error_response(dev->ecam, hchannel, CAM_ERROR_CODE_OutOfMemory); + return ERROR_INVALID_DATA; + } UINT error = dev->ihal->StartStream(dev->ihal, dev, streamIndex, &mediaType, ecam_dev_sample_captured_callback); @@ -352,14 +443,19 @@ static UINT ecam_dev_process_sample_request(CameraDevice* dev, GENERIC_CHANNEL_C CameraDeviceStream* stream = &dev->streams[streamIndex]; + EnterCriticalSection(&stream->lock); + /* need to save channel because responses are asynchronous and coming from capture thread */ if (stream->hSampleReqChannel != hchannel) stream->hSampleReqChannel = hchannel; - /* allow to send that many unsolicited samples */ - stream->nSampleCredits = ECAM_MAX_SAMPLE_CREDITS; + UINT ret = CHANNEL_RC_OK; + stream->samplesRequested++; + if (stream->pendingSample) + ret = ecam_dev_send_pending(dev, streamIndex, stream); - return CHANNEL_RC_OK; + LeaveCriticalSection(&stream->lock); + return ret; } /** @@ -442,8 +538,8 @@ static UINT ecam_dev_process_media_type_list_request(CameraDevice* dev, { UINT error = CHANNEL_RC_OK; BYTE streamIndex = 0; - CAM_MEDIA_TYPE_DESCRIPTION mediaTypes[ECAM_MAX_MEDIA_TYPE_DESCRIPTORS] = { 0 }; - size_t nMediaTypes = ARRAYSIZE(mediaTypes); + CAM_MEDIA_TYPE_DESCRIPTION* mediaTypes = NULL; + size_t nMediaTypes = ECAM_MAX_MEDIA_TYPE_DESCRIPTORS; WINPR_ASSERT(dev); @@ -460,17 +556,24 @@ static UINT ecam_dev_process_media_type_list_request(CameraDevice* dev, } CameraDeviceStream* stream = &dev->streams[streamIndex]; + mediaTypes = + (CAM_MEDIA_TYPE_DESCRIPTION*)calloc(nMediaTypes, sizeof(CAM_MEDIA_TYPE_DESCRIPTION)); + if (!mediaTypes) + { + WLog_ERR(TAG, "calloc failed"); + ecam_channel_send_error_response(dev->ecam, hchannel, CAM_ERROR_CODE_OutOfMemory); + return CHANNEL_RC_NO_MEMORY; + } + INT16 formatIndex = dev->ihal->GetMediaTypeDescriptions(dev->ihal, dev->deviceId, streamIndex, supportedFormats, nSupportedFormats, mediaTypes, &nMediaTypes); - if ((formatIndex < 0) || (nMediaTypes == 0)) + if (formatIndex == -1 || nMediaTypes == 0) { - WLog_ERR(TAG, - "Camera doesn't support any compatible video formats [streamIndex=%" PRIu32 - ", formatIndex=%" PRId16 ", nMediaTypes=%" PRIu32 "]", - streamIndex, formatIndex, nMediaTypes); + WLog_ERR(TAG, "Camera doesn't support any compatible video formats"); ecam_channel_send_error_response(dev->ecam, hchannel, CAM_ERROR_CODE_ItemNotFound); - return ERROR_DEVICE_FEATURE_NOT_SUPPORTED; + error = ERROR_DEVICE_FEATURE_NOT_SUPPORTED; + goto error; } stream->formats = supportedFormats[formatIndex]; @@ -488,7 +591,11 @@ static UINT ecam_dev_process_media_type_list_request(CameraDevice* dev, stream->currMediaType = mediaTypes[0]; } - return ecam_dev_send_media_type_list_response(dev, hchannel, mediaTypes, nMediaTypes); + error = ecam_dev_send_media_type_list_response(dev, hchannel, mediaTypes, nMediaTypes); + +error: + free(mediaTypes); + return error; } /**