meetecho / simple-whep-client

Simple WHEP Client (based on GStreamer's webrtcbin)
GNU General Public License v3.0
6 stars 2 forks source link

Location parsing appears to duplicate the query string #5

Closed mondain closed 1 year ago

mondain commented 1 year ago

In this section, it seems that the query string is duplicated for follow-on PATCH calls: https://github.com/meetecho/simple-whep-client/blob/main/src/whep-client.c#L453-L482

Where the location in a POST response header of live/whep/resource/stream1?requestId=subscriber1 becomes requestId=subscriber1?requestId=subscriber1 in the PATCH requests. Calls on the request for requestId are then invalid with subscriber1?requestId=subscriber1 instead of the expected value of subscriber1.

This may also be an issue in simple-whip-client, but I don't send a querystring there in my testing.

lminiero commented 1 year ago

Yeah that's probably because of

/* Last part of the path, replace it */
g_free(parts[i]);
parts[i] = g_strdup(location);

where we replace the last part of the path with the full value of location. If location contains a query string component too, maybe libsoup flattens that out as part of the path, and the query string property from the main url "survives". Probably the location part should be parsed too, so that the path and query string bits can be replaced separately (and probably the query string part of the endpoint url must be removed nevertheless).

lminiero commented 1 year ago

That said, the value of location is not a valid address, so I don't think soup_uri_new can be used with its helpers, and I'd really rather not write my own URL parser just for that :stuck_out_tongue_winking_eye:

mondain commented 1 year ago

I hear ya there, no interest in writing my own HTML stuff ever again. If I can make the time, I'll see if I can make some sort of patch here, then again I'd love to get all your simple projects working on Ubuntu 22 as well... :)

lminiero commented 1 year ago

then again I'd love to get all your simple projects working on Ubuntu 22 as well... :)

Is that because of the libsoup version clash I remember being mentioned?

lminiero commented 1 year ago

That said, the value of location is not a valid address, so I don't think soup_uri_new can be used with its helpers, and I'd really rather not write my own URL parser just for that stuck_out_tongue_winking_eye

I think I found an easier fix. The problem here is that we're trying to append the location's query string to an existing query string, but since we only need to use the existing endpoint's url as the basis, we can just get rid of its query string argument entirely: then, if the location has one, it will be the only one remaining. As such, the only thing I needed to do was adding a

soup_uri_set_query(uri, NULL);

after the soup_uri_new call, and in my simple test that did the trick. I'll apply the same fix to both WHIP and WHEP clients.

mondain commented 1 year ago

The libsoup seemed fine in U22, it was just a different version I think. The "problem child" was gstreamer with some of its linkage and dependencies.

lminiero commented 1 year ago

IIRC the problem was GStreamer links to libsoup3, while we link to libsoup2, and since they're used in the same application it breaks.

mondain commented 1 year ago

I didn't run into that, but this and its follow on cascade after you try updating the deps

The following packages have unmet dependencies:
 libopencv-dev : Depends: libilmbase-dev
E: Unable to correct problems, you have held broken packages.

Its easy enough to just load that, but then it conflicts with two other dependencies.. I gave up