Skip to content
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

Lars18th soft pid rev2 #180

Merged
merged 11 commits into from
Jan 10, 2023
Merged

Lars18th soft pid rev2 #180

merged 11 commits into from
Jan 10, 2023

Conversation

Barracuda09
Copy link
Owner

Hi @lars18th

Could you check the updated version of your #176

I only did some refactoring of function names, and changed/moved some code around for decrypt etc.

lars18th and others added 11 commits November 16, 2022 20:12
Another refactoring of the internal pid filtering. In this case for the Child PIPE.
Notes:
- The concrete Device is responsable of reread if more data is pending to read after filtering out.
- The function `StreamThreadBase::readDataFromInputDevice()` can flush incomplete packets if timeout expires.
@lars18th
Copy link
Contributor

Hi @Barracuda09 ,

I'm quite busy, and I need to review it in deep.
Regards.

@Barracuda09 Barracuda09 merged commit 4ffe2c9 into master Jan 10, 2023
@lars18th
Copy link
Contributor

Hi @Barracuda09 ,

Thank you for merging my PR #176 and your changes inside this PR. I'm very satisfied with almost all. However, I've two issues to comment (don't worry, one is trivial, and the other is simple):

  1. The first is the removement of the two lines: SI_LOG_DEBUG("Frontend: @#1, PIDs Table: @#2", _feID, _deviceData.getFilter().getPidCSV());. This debug information is very useful to understand which pids are requested and which ones are selected. Please, could you reintegrate them?
  2. The second question is your added line: std::this_thread::sleep_for(std::chrono::microseconds(5));. Finally I've understand why you're enforcing this. I think you're worried with the case when the producer (the source child process) is overloading the PIPE. Therefore you want to protect from this case not reading at full speed. That is? Then, ok I agree with you. However, my use case is when the producer is reading from the network one stream (in fact, it's a RTP TS from one HDHR tuner). In this case the source NEVER will overload the PIPE, and the artificial added reading delays are generating jitter issues. So, I need to remove it. My suggestion then is to provide one tuner option to tweak the waiting value. Then if the value is 0, no waiting will be performed. And if it's set to 5 (or 10, or 50) microseconds then this will be the used delay. You agree with this balanced solution?

I hope you want to add these two small enhancements. Without them I can't start to implement my "external pid support". The initial target will be the HDHR tuners. My idea is to edit the simple script to start/configure/stop them, and add the pid filtering support to the Streaming input with the new functionality to pass the pid list to the script. Then I'll have the option to update the pid list directly in the tuner instead of configure it for sending the full transport stream and do it internally.

So please, you agree?
Regards.

@Barracuda09
Copy link
Owner Author

Hi @lars18th

Some thoughts on you points:

  1. This is already in the log, I believe:
  Sun Feb 22 02:37:08.7389 1970  6  [              src/input/dvb/delivery/DVBC.cpp:054] Frontend: 1, Start tuning process for DVB-C(2)...
  Sun Feb 22 02:37:08.7391 1970  6  [                   src/input/dvb/Frontend.cpp:789] Frontend: 1, Tuned, waiting on lock...
  Sun Feb 22 02:37:08.8898 1970  6  [                   src/input/dvb/Frontend.cpp:807] Frontend: 1, Not locked yet   (FE status 0x00)...
  Sun Feb 22 02:37:09.1910 1970  6  [                   src/input/dvb/Frontend.cpp:801] Frontend: 1, Tuned and locked (FE status 0x1F)
  Sun Feb 22 02:37:09.1912 1970  6  [                          src/mpegts/Filter.h:138] Frontend: 1, Updating PID filters...
  Sun Feb 22 02:37:09.1914 1970  6  [                   src/input/dvb/Frontend.cpp:497] Frontend: 1, Opened /dev/dvb/adapter0/demux0 using fd: 17
  Sun Feb 22 02:37:09.1930 1970  6  [                   src/input/dvb/Frontend.cpp:503] Frontend: 1, Set DMX buffer size to 18874368 Bytes
  Sun Feb 22 02:37:09.1933 1970  6  [                   src/input/dvb/Frontend.cpp:519] Frontend: 1, Set DMX_SET_SOURCE with (Src: 0 - Offset: 0)
  Sun Feb 22 02:37:09.2140 1970  7  [                          src/mpegts/Filter.h:163] Frontend: 1, Set filter PID: 0000
  Sun Feb 22 02:37:09.2345 1970  7  [                          src/mpegts/Filter.h:163] Frontend: 1, Set filter PID: 0001
  Sun Feb 22 02:37:09.2549 1970  7  [                          src/mpegts/Filter.h:163] Frontend: 1, Set filter PID: 0016
  Sun Feb 22 02:37:09.2753 1970  7  [                          src/mpegts/Filter.h:163] Frontend: 1, Set filter PID: 0017
  Sun Feb 22 02:37:09.2957 1970  7  [                          src/mpegts/Filter.h:163] Frontend: 1, Set filter PID: 0018
  Sun Feb 22 02:37:09.3163 1970  7  [                          src/mpegts/Filter.h:163] Frontend: 1, Set filter PID: 3200
  Sun Feb 22 02:37:09.3367 1970  7  [                          src/mpegts/Filter.h:163] Frontend: 1, Set filter PID: 3201
  Sun Feb 22 02:37:09.3571 1970  7  [                          src/mpegts/Filter.h:163] Frontend: 1, Set filter PID: 3211
  Sun Feb 22 02:37:09.3574 1970  6  [                   src/input/dvb/Frontend.cpp:442] Frontend: 1, Updating frontend (Finished in 669 ms)
  1. This delay is only done if the buffer is not filled with the first read. I don't think it is wise to read directly again after the first read, because there will be probably not that many data to read. So just wait a few micro seconds to give it time to fill up with some data.

@lars18th
Copy link
Contributor

Hi @Barracuda09 ,

Thank you for the response. I comment about the two questions:

  1. This part of the log that you've presented only reflects the state change of one pid filter. And my previous lines indicate the current state of the pidlist in the frontend. I need this information for two reasons: The first is to check if the pid list in the process is the same as the pid list in the remote device (when I'll implement the external pid filtering). And the second is to check if the implementation is done correctly: that's the final pid filter status is the spected one. So, if you think that these lines generate a large log then change the visibility of them. Or almost comment the lines but leave them inside the code. I really need them! So please...
  2. As I've commented the delay is generating troubles. I agree with the idea to limit the input in case it can overload the process. I've not considered this case, and it's a potential cause of errors. That's why it's good to have a mechanism to limit the threat. But in the case that you trust the producer, baucase it has a MAX bandwith limit, then the delay is not necessary. And in my environment the source is comming from DVB tuners, so the maximum is the transponder/mux bitrate. And why the delay is generating troubles? Please think on this case: the socket's network read buffer has received a lot of data because some network glitch has added a small jitter. In this case, you need to read more from the buffer, almost until the process buffer is full. I feel you think that the PIPE buffer is different from a NETWORK buffer. But in the case that the process connected to the PIPE is reading from the network, then in this case both have the same behaviour. Therefore any delay in the network will be translated to the PIPE because the process is not controlling the bitrate of the data transmitted (is a simple FIFO process). Said that, I'm convinced that the best is to introduce a new parameter to adjust the value of this delay. In my case, it's necessary that it will be zero. If not then this module is completly useless. I suggest somethig like ?msys=childpipe&exec="..."&delay=XX

You want now to change the code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants