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

[core] Warn user when FFmpeg is missing #9805

Merged
merged 8 commits into from May 4, 2024

Conversation

seproDev
Copy link
Collaborator

@seproDev seproDev commented Apr 27, 2024

IMPORTANT: PRs without the template will be CLOSED

Description of your pull request and other information

Warns the user when the selected format on the default format selection differs due to FFmpeg missing.

As this is part of the _default_format_spec function we don't need to check if the user has passed any custom format selection.
To suppress this warning, the user can pass -f best/bestvideo+bestaudio.

Fіxes #9843

Template

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

@seproDev seproDev added the enhancement New feature or request label Apr 27, 2024
yt_dlp/YoutubeDL.py Outdated Show resolved Hide resolved
yt_dlp/YoutubeDL.py Outdated Show resolved Hide resolved
yt_dlp/YoutubeDL.py Outdated Show resolved Hide resolved
seproDev and others added 2 commits May 1, 2024 23:26
Co-authored-by: pukkandan <pukkandan.ytdlp@gmail.com>
Copy link
Member

@pukkandan pukkandan left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd like a second set of eyes on this
cc @bashonly @Grub4K

Comment on lines 2177 to 2186
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'):
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

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

@pukkandan
Copy link
Member

pukkandan commented May 2, 2024

Might as well clean up the function then? Something like:

    def _default_format_spec(self, info_dict, download=True):
        download = download and not self.params.get('simulate')
        prefer_best = download and (
            self.params['outtmpl']['default'] == '-'
            or info_dict.get('is_live') and not self.params.get('live_from_start'))

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

        if not prefer_best and download and not can_merge():
            prefer_best = True
            formats = self._get_formats(info_dict)
            evaluate_formats = lambda spec: self._select_formats(formats, self.build_format_selector(spec))
            if 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')

        compat = (self.params.get('allow_multiple_audio_streams')
                  or 'format-spec' in self.params['compat_opts'])

        return ('best/bestvideo+bestaudio' if prefer_best
                else 'bestvideo+bestaudio/best' if compat
                else 'bestvideo*+bestaudio/best')

Note: can_merge is a function to avoid initializing ffmpeg if not needed

Comment on lines 2187 to 2189
prefer_best = (
not self.params.get('simulate')
and download
Copy link
Member

@bashonly bashonly May 2, 2024

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

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.

Co-Authored-By: pukkandan <pukkandan.ytdlp@gmail.com>
yt_dlp/YoutubeDL.py Outdated Show resolved Hide resolved
seproDev and others added 3 commits May 4, 2024 00:51
Co-authored-by: bashonly <88596187+bashonly@users.noreply.github.com>
@seproDev seproDev merged commit 96da952 into yt-dlp:master May 4, 2024
15 checks passed
@seproDev seproDev deleted the no-ffmpeg-warning branch May 4, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants