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.

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

[core] Warn user when FFmpeg is missing #9805

Merged
merged 8 commits into from
May 4, 2024
Merged

[core] Warn user when FFmpeg is missing #9805

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ad4fd27
[core] Warn user when ffmpeg is missing
seproDev Apr 27, 2024
1b57e3b
Use `self._get_formats`
seproDev May 1, 2024
810f85f
Pull format selection in to separate method
seproDev May 1, 2024
6078a87
Fix warning when piping to stdout
seproDev May 2, 2024
e9d01a9
Clean up `_default_format_spec` function
seproDev May 2, 2024
9e305e7
Update wording
seproDev May 3, 2024
80b7cd3
Actually don't check the same format twice
seproDev May 4, 2024
f230093
Don't check broken formats twice
seproDev May 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 19 additions & 6 deletions yt_dlp/YoutubeDL.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2136,6 +2136,8 @@ def _filter(f):

def _check_formats(self, formats):
for f in formats:
if f.get('__working'):
yield f
self.to_screen('[info] Testing format %s' % f['format_id'])
path = self.get_output_path('temp')
if not self._ensure_dir_exists(f'{path}/'):
Expand All @@ -2153,16 +2155,32 @@ def _check_formats(self, formats):
except OSError:
self.report_warning('Unable to delete temporary file "%s"' % temp_file.name)
if success:
f['__working'] = True
yield f
else:
self.to_screen('[info] Unable to download format %s. Skipping...' % f['format_id'])

def _select_formats(self, formats, selector):
return list(selector({
'formats': formats,
'has_merged_format': any('none' not in (f.get('acodec'), f.get('vcodec')) for f in formats),
'incomplete_formats': (all(f.get('vcodec') == 'none' for f in formats) # No formats with video
or all(f.get('acodec') == 'none' for f in formats)), # OR, No formats with audio
}))

def _default_format_spec(self, info_dict, download=True):

def can_merge():
merger = FFmpegMergerPP(self)
return merger.available and merger.can_merge()

def evaluate_formats(format_spec):
formats = self._get_formats(info_dict)
return self._select_formats(formats, self.build_format_selector(format_spec))
if not can_merge() and evaluate_formats('best/bestvideo+bestaudio') != evaluate_formats('bestvideo*+bestaudio/best'):
pukkandan marked this conversation as resolved.
Show resolved Hide resolved
self.report_warning('ffmpeg not found. The downloaded format is not the highest available quality. '
'Installing ffmpeg is strongly recommended: https://github.com/yt-dlp/yt-dlp#dependencies')

prefer_best = (
not self.params.get('simulate')
and download
Copy link
Member

@bashonly bashonly May 2, 2024 •
edited

Choose a reason for hiding this comment

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

This could trigger an unnecessary and misleading warning if the user is passing -o - and doesn't have ffmpeg installed. Since the default format selection becomes best/bestvideo+bestaudio when piping to stdout, the warning would be incorrect: installing ffmpeg will not change the quality of what's piped. And I think it's fair to assume that a user who is piping to stdout knows what they're doing and does not need any warning here.

Maybe we could do this?

	to_stdout = self.params['outtmpl']['default'] == '-'

        def evaluate_formats(format_spec):
            formats = self._get_formats(info_dict)
            return self._select_formats(formats, self.build_format_selector(format_spec))
        if not can_merge() and not to_stdout and evaluate_formats('b/bv+ba') != evaluate_formats('bv*+ba/b'):
            self.report_warning('ffmpeg not found. The downloaded format is not the highest available quality. '
                                'Installing ffmpeg is strongly recommended: https://github.com/yt-dlp/yt-dlp#dependencies')

        prefer_best = (
            not self.params.get('simulate')
            and download
            and (
                not can_merge()
                or info_dict.get('is_live') and not self.params.get('live_from_start')
                or to_stdout)))

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we also check for is_live etc then?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think we need to as well

Copy link
Member

@bashonly bashonly May 2, 2024 •
edited

Choose a reason for hiding this comment

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

Side note: I think having simulate result in a different default format spec should be considered a bug. A dry-run should be no different than the real thing, right?

It looks like we can trace this logic back to ytdl-org/youtube-dl@0017d9a

Back then, upstream's default format spec was only best if ffmpeg was not available. So a simulated run would result in a "requested formats not available" error if ffmpeg was not available and there was no combined video+audio format available. This simulate check seems to be added so that you could print json without having to manually pass -f bv+ba or -f bv etc in this scenario -- see the linked upstream PR

(sepro and I started digging into this when I was questioning if simulate should result in a warning or not)

Copy link
Member

Choose a reason for hiding this comment

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

(sepro and I started digging into this when I was questioning if simulate should result in a warning or not)

And if we don't change the default format spec behavior with simulate, then currently the warning is wrongly raised since we aren't preferring best

Copy link
Member

Choose a reason for hiding this comment

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

I don't remember anyone having complained, and I am hesitant to change long standing behavior without reason

And if we don't change the default format spec behavior with simulate, then currently the warning is wrongly raised since we aren't preferring best

Edited #9805 (comment) to address this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, consider this me complaining lol
As bash mentioned, I think a simulate/dry run option is only really useful if you actually simulate what will happen. If a user wants to see what formats will be selected when downloading, the result would be wrong without FFmpeg installed.
This behaviour is currently also not documented.

Copy link
Member

Choose a reason for hiding this comment

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

I was bitten by this undocumented behavior the other day when I was troubleshooting for someone on discord. I also would like to see it fixed or at the very least documented

Copy link
Member

@pukkandan pukkandan May 2, 2024 •
edited

Choose a reason for hiding this comment

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

Still, I don't want to make this change in this PR. Could you open a new issue/PR? We should discuss with dirkf as well.

Expand Down Expand Up @@ -2928,12 +2946,7 @@ def is_wellformed(f):
self.write_debug(f'Default format spec: {req_format}')
format_selector = self.build_format_selector(req_format)

formats_to_download = list(format_selector({
'formats': formats,
'has_merged_format': any('none' not in (f.get('acodec'), f.get('vcodec')) for f in formats),
'incomplete_formats': (all(f.get('vcodec') == 'none' for f in formats) # No formats with video
or all(f.get('acodec') == 'none' for f in formats)), # OR, No formats with audio
}))
formats_to_download = self._select_formats(formats, format_selector)
if interactive_format_selection and not formats_to_download:
self.report_error('Requested format is not available', tb=False, is_error=False)
continue
Expand Down