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

DownloadSections option #157

Merged
merged 9 commits into from
Oct 3, 2023
Merged

DownloadSections option #157

merged 9 commits into from
Oct 3, 2023

Conversation

ar2rworld
Copy link
Contributor

--download-sections support

@ar2rworld
Copy link
Contributor Author

ar2rworld commented Aug 21, 2023

but it requires ffmpeg installed

@wader
Copy link
Owner

wader commented Aug 22, 2023

Hey, looks good and i think ffmpeg is more or less required anyway. Wonder if we want some kind if test?

Also fixes #151 i think

@ar2rworld
Copy link
Contributor Author

Hey @wader, check this one out.
And I am not sure if

if ydlResult.Options.DownloadSections != "*0:0-0:5" {
  t.Errorf("failed to setup --download-sections")
}

is required?

@wader
Copy link
Owner

wader commented Aug 24, 2023

Probably good idea to check that the option "round-trips" so leave it i think. I guess one could check the duration of the output but would require ffmpeg etc. ...but i notice now that ffmpeg is not installed in the Dockerfile when running tests, so how does this work? ffmpeg not needed by yt-dlp for this or it behaves differently?

@wader
Copy link
Owner

wader commented Aug 26, 2023

Hi again, took a deeper look and found some issues:

  • I was confused with the ydlResult.Options.DownloadSections != "*0:0-0:5" check, i somehow assumed it was a value from info.json but see now it just our own option :) but maybe it still make sense to check
  • The download for TestDownloadSections seems to fail because ffmpeg is missing, see log example below. But also the format that gets downloaded ydlResult.Info.Formats[0].FormatID is "sb2" which seems to be an image or something? maybe it's better to just leave it empty or "best" etc?
root@44453646f6dd:/Users/wader/src/goutubedl# yt-dlp -o - -f sb2 --download-sections '*0:0-0:5' 'https://www.youtube.com/watch?v=OyuL5biOQ94' > test
[youtube] Extracting URL: https://www.youtube.com/watch?v=OyuL5biOQ94
[youtube] OyuL5biOQ94: Downloading webpage
WARNING: [youtube] unable to extract initial player response; please report this issue on  https://github.com/yt-dlp/yt-dlp/issues?q= , filling out the appropriate issue template. Confirm you are on the latest version using  yt-dlp -U
[youtube] OyuL5biOQ94: Downloading ios player API JSON
[youtube] OyuL5biOQ94: Downloading android player API JSON
[youtube] OyuL5biOQ94: Downloading iframe API JS
[youtube] OyuL5biOQ94: Downloading player c153b631
[youtube] OyuL5biOQ94: Downloading web player API JSON
[youtube] OyuL5biOQ94: Downloading m3u8 information
WARNING: [youtube] unable to extract yt initial data; please report this issue on  https://github.com/yt-dlp/yt-dlp/issues?q= , filling out the appropriate issue template. Confirm you are on the latest version using  yt-dlp -U
WARNING: [youtube] Incomplete data received in embedded initial data; re-fetching using API.
[youtube] OyuL5biOQ94: Downloading initial data API JSON
[info] OyuL5biOQ94: Downloading 1 format(s): sb2
[info] OyuL5biOQ94: Downloading 1 time ranges: 0.0-5.0
ERROR: You have requested downloading the video partially, but ffmpeg is not installed. Aborting

I tried installing ffmpeg and change format then --download-sections seem to work fine. So maybe we should install it and do some checks?

@ar2rworld
Copy link
Contributor Author

added ffmpeg to the Dockerfile
and added exec.Command check for FFmpeg in the test

@wader
Copy link
Owner

wader commented Sep 3, 2023

ci fails on installing ffmpeg, missing && i think. there is some tips how to build and run tests here https://github.com/wader/goutubedl#development

@ar2rworld
Copy link
Contributor Author

thanks that was helpful, i think it is good to go?

if err != nil {
t.Fatal(err)
}
dr.Close()
Copy link
Owner

Choose a reason for hiding this comment

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

Thinking it would be great to somehow check that DownloadSections works, maybe can ffprobe the result?

$ yt-dlp --download-sections '*0:0-0:5'  'https://www.youtube.com/watch?v=OyuL5biOQ94'
...
$ ffprobe -v quiet -show_entries format=duration  Paradise\ -\ Coldplay\ \(Fingerstyle\ Guitar\)\ \[OyuL5biOQ94\].webm
[FORMAT]
duration=5.004000
[/FORMAT]

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for being picky with tests, reason is i really want to be able to trust the tests to catch breakage when auto updating yt-dlp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do,
but this gonna work with the sections that specified as a time range, not then chapter name specified:

yt-dlp --download-sections 'Intro'

Is there a way to get information on the length of the chapter?

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe its in the Info.json but not sure. But just testing time range is fine with me

Copy link
Owner

Choose a reason for hiding this comment

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

Hey, got stuck on checking duration?

@ar2rworld
Copy link
Contributor Author

looks very messy, what do you think?

f, err := os.Create(fileName)
if err != nil {
t.Fatal(err)
}
Copy link
Owner

Choose a reason for hiding this comment

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

might want a defer f.Close() to make sure things are written

@wader
Copy link
Owner

wader commented Oct 3, 2023

yeah a bit messy but maybe ok?

@ar2rworld
Copy link
Contributor Author

how can I improve it?

@wader
Copy link
Owner

wader commented Oct 3, 2023

I guess it could be refactored to some kind of ffprobe/get value function but maybe can wait until another test needs it

@wader wader merged commit 3f0889f into wader:master Oct 3, 2023
1 check passed
@wader
Copy link
Owner

wader commented Oct 3, 2023

Thanks!

@ar2rworld
Copy link
Contributor Author

Thanks for your support)

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