Skip to content

cli: rewrite progress output #4656

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 17, 2022
Merged

cli: rewrite progress output #4656

merged 1 commit into from
Jul 17, 2022

Conversation

bastimeyer
Copy link
Member

@bastimeyer bastimeyer commented Jul 16, 2022 •
edited
Loading

  • Split streamlink_cli.utils.progress into progress + terminal
    • terminal:
      Responsible for calculating character output widths and for writing
      data to the output text stream
    • progress:
      Responsible for calculating the download speed and for formatting
      the progress output
  • Use a separate thread for the progress output, with the output being
    written in constant time intervals, independent of the data that gets
    fed by the stream iterator of the main thread
  • Output format changes:
    • Remove "prefix" from output format (full path already gets logged)
    • Avoid showing download speeds until a time threshold is reached
    • Use binary values for file size and download speed values
    • Add leading zeros to time units (only ever grow output text size)
  • Don't import from alias module in streamlink_cli.main
  • Split, move and rewrite tests

The download progress on the master branch is currently bound to the main thread and data of the stream iterator in read_stream(). As explained in #4642, this has several issues. Rewriting the progress output is one of the requirements for being able to resolve the linked issue.

The interface of the Progress class is written with the current stream_iterator implementation in read_stream() in mind, which will change according to my plans described in #4642, but that shouldn't be a big issue with how the put() method is implemented.

@bastimeyer bastimeyer added WIP Work in process CLI labels Jul 16, 2022
@bastimeyer bastimeyer force-pushed the cli/threaded-progress branch from 3a3e5da to d8beab6 Compare July 16, 2022 22:52
@bastimeyer
cli: rewrite progress output
ed65152 
- Split `streamlink_cli.utils.progress` into `progress` + `terminal`
  - `terminal`:
    Responsible for calculating character output widths and for writing
    data to the output text stream
  - `progress`:
    Responsible for calculating the download speed and for formatting
    the progress output
- Use a separate thread for the progress output, with the output being
  written in constant time intervals, independent of the data that gets
  fed by the stream iterator of the main thread
- Output format changes:
  - Remove "prefix" from output format (full path already gets logged)
  - Avoid showing download speeds until a time threshold is reached
  - Use binary values for file size and download speed values
  - Add leading zeros to time units (only ever grow output text size)
- Don't import from alias module in `streamlink_cli.main`
- Split, move and rewrite tests
@bastimeyer bastimeyer force-pushed the cli/threaded-progress branch from d8beab6 to ed65152 Compare July 16, 2022 22:57
@bastimeyer
Copy link
Member Author

Added the missing tests.

I decided to not test the threading logic of the Progress thread, as it requires a ridiculous amount of boilerplate code and it also requires adding a workaround for freezegun (in regards to the threading module) which then unfortunately interferes with pytest's test runner outputs. Testing the update() logic is good enough.

@bastimeyer bastimeyer marked this pull request as ready for review July 16, 2022 22:59
@bastimeyer bastimeyer removed the WIP Work in process label Jul 16, 2022
bastimeyer
bastimeyer commented Jul 16, 2022
View reviewed changes
src/streamlink_cli/utils/progress.py
Comment on lines +78 to +80
interval: float = 0.25,
history: int = 20,
threshold: int = 2,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These values could be tweaked.

Four updates per seconds seems reasonable, and so is keeping a download speed history (the result is the average in this time frame) of twenty seconds which ensures that segmented streams with large segment durations (but still less than 20 seconds) don't make the download speed go up and down in between segment downloads and segment queue wait times.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if anyone has feedback after the next release. If so they can be modified but these seem reasonable to me.

@gravyboat
Copy link
Member

Looks great @bastimeyer!

@gravyboat gravyboat merged commit 424aac2 into streamlink:master Jul 17, 2022
@bastimeyer bastimeyer deleted the cli/threaded-progress branch July 17, 2022 19:03
This was referenced Aug 20, 2022
Closed
Merged
@bastimeyer bastimeyer mentioned this pull request Aug 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gravyboat gravyboat gravyboat left review comments

Assignees
No one assigned
Labels
CLI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bastimeyer @gravyboat