Codecov Report

Merging #920 into master will increase coverage by <.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #920      +/-   ##
==========================================
+ Coverage   51.27%   51.28%   +<.01%     
==========================================
  Files         234      234              
  Lines       14271    14284      +13     
==========================================
+ Hits         7318     7326       +8     
- Misses       6953     6958       +5

@grazzolini Sorry for the delay on getting to this. I'm fine with this enhancement, @beardypig, @bastimeyer any concerns?

@bastimeyer
I don't have any issues in renaming the option to something else. "recording" is just in the sense you're recording something live or something like that. Having said that, I'm out of ideas on what to call it.

I use this option for recording some live events I want to review later. As for an example, I can write one, but --record only works with -o, so it would need to be there in the example. Regarding tests, I think I can write those as well, no problem. Let us just settle on a name for the option.

@grazzolini
Yeah, it's not easy finding a short and precise parameter name.

# output is a file (a simple "record" command)
streamlink -o file url quality
# output is stdout
streamlink -O url quality | tee file
# output is the player and a file (the name "record" doesn't make sense here unfortunately)
streamlink -o file --record url quality

If the implementation was like this, the name "record" could be kept:

# output is the player and a file (watch and record at the same time)
streamlink --record file url quality
streamlink -r file url quality

I think this is a better idea and much easier to understand by the end user. But how should streamlink behave in this case if both -o and -r are being used at the same time?

@bastimeyer
How about make the usage of both at the same time impossible, like what I did with -O? This would surely make the record option more palatable and easier to understand.

There's a reason why I prefer the recording to be done at streamlink itself. tee buffers it's input and that cause buffering issues on the player. You can mess with the cache parameter, but even then there's some type of double buffering. And tee does not always respect stdbuf.

make the usage of both at the same time impossible

Yes, this is ok.

There's a reason why I prefer the recording to be done at streamlink itself.

I've just listed the -o and -O parameters as an example to make the parameter naming scheme more visible. I was not arguing that tee is a better way of recording streams.

Regarding the shorthand parameter name, it should be -r, so that it's clear that it's a replacement of the -o parameter ("record-and-play"). -R could be added as an equivalent to the -O param in the sense of "pipe-and-play".

Can we implement the changes requested by @bastimeyer? I would like to get this merged.

@gravyboat
I think I should be able to hack something tomorrow. It makes sense to add both -r and -R options. I'll also try to write some tests for these as well.

@grazzolini Sounds good, any other feedback @bastimeyer?

• if you use -R but not -o it will not record anything,
  there should be a error- or infomessage
  that -o is missing or/and no download started

• if -R and -o is set there should be an infomessage that the download started,

  if you use -o without -R there will be
  [download][123.ts] Written 4.8 MB (14s @ 330.3 KB/s

  but with -R you won't know if it started
  there is only a debug message, but you don't use it normally
  [cli][debug] Writing stream to output

Not important but this can confuse people who don't know how hls streams work

• if you record a vod and close it, the output file will be ahead of the player time.
  on twitch ~40sec
• if you record a livestream and close it, the output file will be ahead of the player time.
  on twitch ~10sec

grazzolini wants to merge 97 commits into streamlink:master from grazzolini:record
Commits 97
Files changed 144
+2,303 -1,257

You need to properly rebase your branch in order to fix this PR. This is impossible to review on Github in the current state and I don't want to diff your branch locally and comment here then...

# set up proper remote repos
git remote add upstream https://github.com/streamlink/streamlink.git
git remote add github https://github.com/grazzolini/streamlink.git

# I guess your local branch is called record, too
git checkout record

# fix the local master branch, just in case
git fetch upstream
git branch --force master upstream/master

# fix the local record branch and only pick the necessary commits
git rebase --interactive master

# force push in order to upgrade the PR
git push --force github record

@bastimeyer

I have rebased the record branch according to streamlink master. I have been, in the past few months, replacing these files after I update streamlink, so I can continue to use this option. I had not time to write proper tests for it, but it would be great to add them.

@bastimeyer Any further thoughts on this one?

No further thoughts, as the points from all my previous comments still stand...
The comment from Feb 8th was about the messed up branch and not related to the actual changes of this PR.

@grazzolini you still interested in getting this merged in? 👍

Hi @beardypig, given that I have been manually patching files for adding this, yes, I am. I have started writing some tests a couple of months ago, but there were some new streamlink versions in the meantime, so I ended up no updating things.

@grazzolini Looks like you did a push on this a couple days ago. Are you still working on it?

@gravyboat, I occasionally do a rebase from master to keep this branch passing all the checks. I did make some changes to this branch though, because there were changes, specially to argparser.py. But I didn't introduce anything new.

Added in #2152