-
Notifications
You must be signed in to change notification settings - Fork 14.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add server side handling for [MS-RDPECAM] #8010
Conversation
Can one of the admins verify this patch? |
channels/camera-device-enumerator/server/camera_device_enumerator_main.c
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM nothing show stopper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM,
some style issues and on missing event? (or did I miss that?)
channels/camera-device-enumerator/server/camera_device_enumerator_main.c
Outdated
Show resolved
Hide resolved
@pnowack oh, and all the |
@pnowack another thing, maybe unify this whole thing in a single |
Not specific to this PR, but the reading of data is done like in some other dynamic channels, and we have something rotten here. Dynamic channels are supposed to always receive fragmentation headers (so when you read, you receive fragheaders+data) and the frag header allows you to reassemble a complete PDU, but our FreeRDP VCM implementation doesn't respect that. Unfortunately fixing that has the potential to break lots of external projects that have dynamic channel implementations (yes Ogon I'm pointing at you). I will open a discussion on this. We also have to talk about our |
8b3c60c
to
d9b36e3
Compare
Done.
I hope I did that correctly in the last push
I am not really sure, what to do here.
If fixing this would break external projects, then it would better be done for FreeRDP3 in my opinion. |
@pnowack yeah my general comment on dynamic channels was not meant for your PR, sorry for the noise. |
@freerdp-bot can you test that please buddy ? |
Refer to this link for build results (access rights to CI server needed): |
@freerdp-bot test |
Refer to this link for build results (access rights to CI server needed): |
@freerdp-bot test |
Refer to this link for build results (access rights to CI server needed): |
@freerdp-bot test |
Refer to this link for build results (access rights to CI server needed): |
@freerdp-bot test |
Refer to this link for build results (access rights to CI server needed): |
@freerdp-bot test |
@freerdp-bot be so kind, please test this again |
@freerdp-bot test |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
@freerdp-bot test |
@freerdp-bot test |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
This is a forward port of https://github.com/pnowack/FreeRDP/commits/rdpecam. Due to lack of time not all messages are tested. The ones, that are tested are tested with g-r-d with https://gitlab.gnome.org/pnowack/gnome-remote-desktop/-/commits/rdpecam.
Successfully tested messages:**
Enumerator messages:
SelectVersionRequest
SelectVersionResponse
DeviceAddedNotification
Device messages:
SuccessResponse
ErrorResponse
ActivateDeviceRequest
DeactivateDeviceRequest
StreamListRequest
StreamListResponse
MediaTypeListRequest
MediaTypeListResponse
CurrentMediaTypeRequest
CurrentMediaTypeResponse
StartStreamsRequest
StopStreamsRequest
SampleRequest
SampleResponse
PropertyListRequest
PropertyListResponse
PropertyValueRequest
PropertyValueResponse
SetPropertyValueRequest
Messages that weren't tested (yet), due to lack of time (they should work though):
Enumerator messages:
DeviceRemovedNotification
(never received it)Device messages:
SampleErrorResponse
(never received it)Would be good if you can take a more precise look at the untested messages.
Some messages only contain the shared message header. However, when sending such messages, the whole PDU can be provided or
NULL
.The reason for this design is for forward compatibility. In case that in future versions one of these messages is extended with extra arguments, when using newer protocol versions, we don't need to add new APIs in form of
ActivateDeviceRequest2
or similar.For the device channel both the protocol version and virtual channel name need to be specified in the camera device server context by the API user.
The protocol version set in the context struct is used, when writing the shared message header, when sending any messages.
Server implementations should check for invalid virtual channel names, like already existing ones, or channels, that are known for different purposes (graphics pipeline, echo, etc.).