search

catalinii / minisatip

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

Fix compare tunning if SR=auto in current tunning #1037

Closed lars18th closed 1 year ago

lars18th commented 1 year ago

When comparing new and current tunning values, the SR could be AUTO. Not only for the "new" tunning, but also for the "current" tunning. This fixes the tunning when the "current" adapter tuning is using AUTO mode for SR, and the "new" request is forcing some value. In this edge case, we can consider that the current running value is the one corresponding to the current tunning mode. That's we trust the new request.

lars18th commented 1 year ago

Hi,

This fixes #1011. The only problem that I can see, it's if the new request is enforcing a different SR mode that the real currently used. For example, if some client is receiving a valid stream and it has tunned it using SR=AUTO. And after another client is doing some scaning over the same frequency and it uses a SR that is different from the real one. Only in this case this patch could generate a trouble. So I assume that this very edge case could not be considered. However, if someone understand that we need to protect to it, then it will be necessary to store the real tunning parameters when requesting with the AUTO mode for some options. In this case, if the client requests SR=AUTO, then the adapter will complete the information this information. And therefore this patch will can't generate any side effect, as the adapter->sr value will never be 0 (aka auto).

Please test it and merge. Regards.

Jalle19 commented 1 year ago

Can you add a test case for this to https://github.com/catalinii/minisatip/blob/master/tests/test_dvb.c ?

lars18th commented 1 year ago

Can you add a test case for this to https://github.com/catalinii/minisatip/blob/master/tests/test_dvb.c ?

I'll try.

Jalle19 commented 1 year ago

I just realized that the existing tests aren't for compare_tuning_parameters :facepalm: So it's okay to skip the tests for now, but it would be good to get this function tested since it contains tons of logic and is pretty central to the operation of minisatip.

lars18th commented 1 year ago

Hi @Jalle19 ,

Because I'm not an specialist writing Unit Tests, I prefer if someone write them. In the meantime, please merge this patch. The only edge case not considered is when a new client requests the same transponder with specific SR, and at the same time other user is using it with AUTO and the SR from the new client is not the same. In this case, nothing is interrupted. The new user will be attached to the tuner used by the current user. The problem is that the new client will receive the stream using an incorrect value of the SR in the request. The true is that the request in fact requires to fail because the incorrect tunning parameters. But it's a very edge case... and perhaps it could be a problem in a blind scaning. But I feel it's preferable to overcome this case and merge the patch.

lars18th commented 1 year ago

Hi @catalinii ,

This PR is approved by @Jalle19 . Any comment from you? From my point of view it's ready to merge it.

catalinii commented 1 year ago

I agree with @Jalle19 related to unit tests and in general we should this about this aspect too.

Generally is the responsibility of the one submitting a patch to add unit tests

lars18th commented 1 year ago

I agree with @Jalle19 related to unit tests and in general we should this about this aspect too.

Generally is the responsibility of the one submitting a patch to add unit tests

OK. But I don't have sufficient experience with them. I'll try in the future to implement one.

Regards.