stream_file: make appending:// URLs more useful #15998
Conversation
|
Download the artifacts for this pull request: |
|
Addressed all suggestions. |
|
Also needs |
| #define OPT_BASE_STRUCT struct appending_opts | ||
| const struct m_sub_options stream_appending_conf = { | ||
| .opts = (const struct m_option[]) { | ||
| {"retry-timeout", OPT_DOUBLE(retry_timeout), M_RANGE(0.1, 1.0)}, |
There was a problem hiding this comment.
This range seems a bit tight? any reason for that?
There was a problem hiding this comment.
well because there are multiple retries.
however having both of these options seems needlessly confusing. it would work perfectly fine to have just retry-timeout and then automatically the code automatically re-checks every 0.2s
There was a problem hiding this comment.
It goes against mpv core principles where everything is customization. Though given how small is the range to customize this variable, it makes not much difference in practice, no?
it would work perfectly fine to have just retry-timeout and then automatically the code automatically re-checks every 0.2s
You meant max-retries?
There was a problem hiding this comment.
You meant
max-retries?
well no. then the unit of the option would be 0.2s, which is confusing.
the user should specify which total timeout he wants and then mpv retries as it sees fit (could even be linear backoff).
There was a problem hiding this comment.
Regarding exposing retry_timeout and it's range.
My thought was that a lower value than 0.2 may help allowing uninterrupted playback in cases where a stream is being appended to very often. Think an unsegmented live stream that is being saved as a file, or a live stream with very short segments.
On the upper bound, my thought was allowing for less resource usage if the user knows appending doesn't happen with high frequency, and doesn't mind the delay between retries, which is maybe less convincing, but still a nice to have IMHO.
I think both options should be exposed in any case.
There was a problem hiding this comment.
I will let @sfan5 decide on this one. For me it is fine to have those options exposed.
There was a problem hiding this comment.
if the user is interested in micro-optimizing resource usage he should use a pipe, which does not require polling.
I stand by my suggestion to only have one option.
MoSal
force-pushed
the
appending_opts
branch
2 times, most recently
from
4d53658 to
330f7b8
Compare
|
Made opts live-update, which theoretically adds synchronization overhead. But I think it's fine. |
I would rather avoid this in main file reading function. |
By exposing the two parameters that control the *timeout* as options. `--appending-max-retries` replaces the `MAX_RETRIES` constant. And `--appending-retry-timeout` replaces the `RETRY-TIMEOUT`. The options have the same default values as the removed constants. So nothing should change behavior wise. Signed-off-by: Mohammad AlSaleh <CE.Mohammad.AlSaleh@gmail.com>
Reverted. As for not exposing |
|
What does your use case look like? |
Just an example, playing a live MPEG-TS stream that is being saved to disk with the fastest update/lowest delay possible, but with seek and |
|
I see why a pipe wouldn't work for your use case. In that case adding these options should be fine. |
By exposing the two parameters that control the timeout as options.
--appending-max-retriesreplaces theMAX_RETRIESconstant. And--appending-retry-timeoutreplaces theRETRY-TIMEOUT.The options have the same default values as the removed constants. So
nothing should change behavior wise.