catalinii / minisatip

minisatip is an SATIP server for linux using local DVB-S2, DVB-C, DVB-T or ATSC cards
https://minisatip.org
324 stars 78 forks source link

[BUG]: "satipc" does not work properly when the tuner is in use. #1118

Open lars18th opened 1 year ago

lars18th commented 1 year ago

Hi @catalinii ,

When using the satipc module if you "share" the source adapter then you will be in troubles when the tuner is in use.

Here a simple tcpdump log with the problem:

18:48:29.137741 IP 192.168.1.51.56302 > 192.168.1.82.rtsp: Flags [P.], seq 3586096098:3586096289, ack 2841071189, win 501, length 191: RTSP: PLAY rtsp://192.168.1.82:554/?freq=... RTSP/1.0
18:48:29.138591 IP 192.168.1.82.rtsp > 192.168.1.51.56302: Flags [P.], seq 1:127, ack 191, win 473, length 126: RTSP: RTSP/1.0 404 Not Found
18:48:29.138756 IP 192.168.1.37.54071 > 192.168.1.82.rtsp: Flags [.], ack 2298112, win 21, length 0
18:48:29.138823 IP 192.168.1.82.rtsp > 192.168.1.37.54071: Flags [P.], seq 2298112:2299324, ack 1, win 490, length 1212: RTSP
18:48:29.140357 IP 192.168.1.51.56302 > 192.168.1.82.rtsp: Flags [.], ack 127, win 501, length 0
18:48:29.140519 IP 192.168.1.51.56302 > 192.168.1.82.rtsp: Flags [P.], seq 191:274, ack 127, win 501, length 83: RTSP: DESCRIBE rtsp://192.168.1.82:554/ RTSP/1.0

As you can see the problem is this:

However, in the minisatip server using the satipc module I've more adapters. And the problem is that instead of searching for another (next) free adapter it will continue reopening (aka restarting) the same adapter.

Any idea to fix this?

Jalle19 commented 1 year ago

Doesn't this only happen if fe= is specified in the URL?

lars18th commented 1 year ago

Hi @catalinii ,

The minisatip LOG when the problem appears:

[07/06 19:28:42.648]: satipc: commit for adapter 3 freq 11347000, pids to add 15, pids to delete 0, expect_reply 0, force_commit 0 want_tune 1 do_tune 1, force_pids 0, sent_transport 0, sleep 0, closed_rtsp 0
[07/06 19:28:42.648]: satipc_http_request (adapter 3): sending to sock 12
[07/06 19:28:42.648]: Reply (handle 11) [192.168.1.82:53387] content_len:0, sock 15
[07/06 19:28:42.656]: satipc_reply (adapter 3): receiving from handle 9, sock 12
[07/06 19:28:42.656]: marking device 0 as error, rc = 404
[07/06 19:28:42.656]: satipc 0 wp 0 qp 0, expect_reply 1, want_tune 0, force_commit 0, want commit 0
[07/06 19:28:42.656]: satipc_http_request (adapter 3): sending to sock 12
[07/06 19:28:42.657]: satipc_reply (adapter 3): receiving from handle 9, sock 12
[07/06 19:28:42.657]: satipc: Received signal status from the server for adapter 3, stength=240 status=1 quality=15
[07/06 19:28:42.657]: satipc 0 wp 0 qp 0, expect_reply 1, want_tune 0, force_commit 0, want commit 0
[07/06 19:28:54.679]: select_and_execute[15]: Close on socket 11 (sid:0) from 192.168.1.82:53387 - type http (3) errno 0
[07/06 19:28:54.679]: Requested sid close 0 timeout 30000 type 1, sock 15, handle 11, timeout 0
lars18th commented 1 year ago

Doesn't this only happen if fe= is specified in the URL?

No. It appears with and without the fe= parameter. But without it the correct behaviour is to close the adpater and search for another one. Try yourself to do the test:

lars18th commented 1 year ago

More or less the bug has the root cause in these lines: https://github.com/catalinii/minisatip/blob/8d33e939b820fef824c84a92dd6affcd455bbf55/src/satipc.c#L243-L244

The error of the adapter it's set, but the code will never checks it.

Jalle19 commented 1 year ago

I'm pretty sure this works for me. I have one minisatip instance proxying two Digibit R1 tuners.

Jalle19 commented 1 year ago

Maybe I'm misunderstanding the issue

catalinii commented 1 year ago

Can u check how next branch is behaving? There are some refactorings done for the code ro be managed easier there. The master branch will get only important fixes and will not get any new features

lars18th commented 1 year ago

Can u check how next branch is behaving? There are some refactorings done for the code ro be managed easier there.

I'll check it. However, reading the code of satipc.c...

    if (rc == 404) {
        sip->state = SATIP_STATE_OPTIONS;
    }
[...]
        case SATIP_STATE_OPTIONS:
            sip->state = SATIP_STATE_SETUP;

So, I feel the behaviour is the same: re-request when receiving the 404 error. And the correct behaviour is to close the adapter and try the next.

The master branch will get only important fixes and will not get any new features

Then, please commit my PR #1117 with the next branch. This new functionality is very useful. 😉

Jalle19 commented 1 year ago

Shouldn't the server be choosing a free adapter? Why does the client have to do that?

lars18th commented 1 year ago

Hi @Jalle19 ,

Shouldn't the server be choosing a free adapter? Why does the client have to do that?

The scenario is this:

Client 1 ------------------------> +--> minisatip A (1 tuner)
                                   |
Client 2 ---> minisatip C (satipc) +--> minisatip B (1 tuner)

And the problem appears in the "minisatip C". This is a central server that has configured with 2 adapters (with the satipc module). The problem here is that when the client 1 is using the minisatip A (that only has 1 tuner), then the adapter 1 in the minisatip C is BUSY. But the current implementation marks it as FREE. Therefore if the Client 2 requests some channel then the minisatip C will selects the adapter 1... and this is BUSY because the minisatip A is in use by the client 1. And then the current code enters in a loop re-requesting to SETUP to the minisatip A, instead of try with the minisatip B that it's free.

So the current implementation of the satipc module requires to be changed. The code at time doesn't care about BUSY adapters. And this functionality requires to be incorporated. So please @catalinii any idea about how to do it?

Regards.

Jalle19 commented 1 year ago

Yeah I think I understand the problem now.

lars18th commented 1 year ago

Yeah I think I understand the problem now.

Nice! So to fix it, I feel @catalinii needs to help us to indicate how to close an adapter from inside the submodule (satipc in this case) and reopen another one from the upper layer. My suggestion is that the get_free_adapter(...) function will check for a new adapter parameter (ad->busy) ; and the satipc module will set/cleans this flag.

What you think @catalinii ?

Jalle19 commented 4 days ago

It seems to be that in @lars18th scenario, "minisatip A" that has one physical tuner should return error code 503 with the body No-More: frontends. Currently we just return 404:

        a_id = get_free_adapter(&sid->tp);
        LOG("Got adapter %d on sid %d socket %d", a_id, sid->sid, s->id);
        if (a_id < 0)
            return -404;

This would just be the first step, next would be to handle this return code correctly. Currently the path taken is the same - state gets reset to SATIP_STATE_OPTIONS.

lars18th commented 4 days ago

Hi @Jalle19 ,

Yes, the "remote SAT>IP server" (the one with the physical tuner) requires to return the correct response. And then, the "local SAT>IP server" (the one using the satipc module) needs to process it in acordance.