Add local unix socket support #86

sundermann · 2022-07-20T13:17:54Z

Local unix socket support was added in HyperHDR in awawa-dev/HyperHDR@1100093

Local unix sockets are automatically used when the hostname is set to "/tmp/hyperhdr-domain" which is hardcoded in HyperHDR. On my TV local sockets are around 20-30% faster.

TBSniller · 2022-09-25T18:20:53Z

Hey there,

thanks again for contribution!
We have mergend unicapture branch to main. Could you please update your PR to also merge it into main?

Thanks!

src/settings.c

src/service.c

tuxuser

See comments

tuxuser · 2022-09-28T21:40:01Z

src/service.c

+    char* address = service->settings->address;
+    if (service->settings->unix_socket) {
+        address = "127.0.0.1";
+    }


Please move this piece of code / overwrite to main.c, after parse_options was called.

Basically overwriting address in global variable settings by doing settings.address = "127.0.0.1

That won't work in case of a reconnect.

Can you explain in more detail in which case it would break?

unix_socket is just a boolean value so where would the unix socket address be stored if it's already overriden just after parsing options. I can only think of making this work by changing unix_socket to be a char* containing the socket address.

Why does the target address/path for the socket need to be stored?

On a later call to hyperion_client it will receive the flag unix_socket again and know that it has to do the scanning for path /tmp/hyperhdr-domain again - to see if its still available.

The logic stays the same.
The overwrite to address 127.0.0.1 is really just used to enable communication to the JSON RPC, when unix domain socket is used.

Yes, I misunderstood your idea at first thinking you wanted to compare the hostname against a list of known sockets. However in fact you want to scan if a specific local socket is available. I agree the second idea can work but still I don't like the idea of scanning against a hardcoded list because the socket address could change or hyperion.ng decides to implement local sockets as well.

Another issue is that the -a parameter is still mandatory even though in case of local sockets -a is not mandatory and overwritten making it a bit unintuitive to use.

agreed, so the flow is:

Set /tmp/hyperhdr-domain in the IP address field

Set whatever in port or leave it the default by not providing it

Result: unix domain socket is used for flatbuffers, Tcp socket for json rpc.

Sounds good!

Yes, that is how it is supposed to work.

tuxuser · 2022-09-28T21:41:10Z

src/service.c

+    char* address = service->settings->address;
+    if (service->settings->unix_socket) {
+        address = "127.0.0.1";
+    }


See comment above

tuxuser · 2022-09-28T21:43:23Z

src/hyperion_client.c

 {
    _origin = origin;
    _priority = priority;
    _connected = false;
    _registered = false;
    sockfd = 0;
-    struct sockaddr_in serv_addr;


Add checking for /tmp/hyperhdr-domain here. Iterate over constant array of possible filelocations, in case Hyperion.NG implements local unix socket support too.

If such unix socket handle is found, use it as parameter to _connect_unix_socket.

Does this mean we should remove the command line option?

No, the cmdline is fine and useful in this case. Just when it knows that a "local unix domain socket" is desired, it should check and use such, if available.

Case 1: No unix domain socket desired

Supplied IP and Port is used as-is

Case 2: Unix domain socket is desired

Hardcode ddress to 127.0.0.1 for HTTP RPC JSON communication to the daemon

Before connecting the socket for flatbuffer data, check if known unix domain sockets exist and use them - otherwise fall back to 127.0.0.1.

I'm not convinced this is the best option. This tightly couples hyperion-webos to a list of unix sockets which are provided by external software and might change in future. As you pointed out if hyperion.ng were to add a local socket this means that hyperion-webos needs changes as well.

sundermann · 2022-10-02T21:58:18Z

Anything missing on this PR?

tuxuser · 2022-10-02T22:02:15Z

Testing it currently

tuxuser · 2022-10-02T22:10:45Z

Yup, works fine. Please cleanly rebase against main and correct the linting, then we should be good to go. cheers

Local unix socket support was added in HyperHDR in awawa-dev/HyperHDR@1100093 Adds the --unix-socket / -l switch in which case a connection over a local unix socket on the specified address is established. On my TV unix sockets are around 20-30% faster.

TBSniller · 2022-10-02T22:32:49Z

Thanks!

sundermann force-pushed the local-socket branch from 49824cd to c37bd9f Compare July 30, 2022 16:21

TBSniller mentioned this pull request Sep 25, 2022

Add setting for local unix socket TBSniller/piccap#46

Closed

asturel reviewed Sep 27, 2022

View reviewed changes

src/settings.c Show resolved Hide resolved

asturel reviewed Sep 27, 2022

View reviewed changes

src/service.c Outdated Show resolved Hide resolved

sundermann force-pushed the local-socket branch 3 times, most recently from 7b86104 to d9ac6ba Compare September 27, 2022 20:49

sundermann changed the base branch from unicapture to main September 27, 2022 20:51

sundermann force-pushed the local-socket branch from d9ac6ba to bd81cca Compare September 28, 2022 08:37

tuxuser requested changes Sep 28, 2022

View reviewed changes

sundermann force-pushed the local-socket branch 3 times, most recently from 047d0be to df27e8b Compare September 28, 2022 23:07

sundermann force-pushed the local-socket branch 2 times, most recently from a816306 to f813209 Compare October 2, 2022 22:22

Add unix socket support

a94be23

Local unix socket support was added in HyperHDR in awawa-dev/HyperHDR@1100093 Adds the --unix-socket / -l switch in which case a connection over a local unix socket on the specified address is established. On my TV unix sockets are around 20-30% faster.

sundermann force-pushed the local-socket branch from f813209 to a94be23 Compare October 2, 2022 22:28

TBSniller merged commit 19eb7ce into webosbrew:main Oct 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add local unix socket support #86

Add local unix socket support #86

sundermann commented Jul 20, 2022

TBSniller commented Sep 25, 2022

tuxuser left a comment

tuxuser Sep 28, 2022

sundermann Sep 28, 2022

tuxuser Sep 28, 2022

sundermann Sep 28, 2022

tuxuser Sep 28, 2022

sundermann Sep 28, 2022

sundermann Sep 28, 2022

tuxuser Sep 28, 2022

sundermann Sep 28, 2022

tuxuser Sep 28, 2022 •

edited

Loading

tuxuser Sep 28, 2022

sundermann Sep 28, 2022

tuxuser Sep 28, 2022

sundermann Sep 28, 2022

sundermann commented Oct 2, 2022

tuxuser commented Oct 2, 2022

tuxuser commented Oct 2, 2022

TBSniller commented Oct 2, 2022

Add local unix socket support #86

Add local unix socket support #86

Conversation

sundermann commented Jul 20, 2022

TBSniller commented Sep 25, 2022

tuxuser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuxuser Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sundermann commented Oct 2, 2022

tuxuser commented Oct 2, 2022

tuxuser commented Oct 2, 2022

TBSniller commented Oct 2, 2022

tuxuser Sep 28, 2022 •

edited

Loading