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

--embed-thumbnail crashes the program if the file has the wrong type #10500

Open
Profpatsch opened this issue Aug 30, 2016 · 8 comments
Open

--embed-thumbnail crashes the program if the file has the wrong type #10500

Profpatsch opened this issue Aug 30, 2016 · 8 comments
Labels

Comments

@Profpatsch
Copy link

@Profpatsch Profpatsch commented Aug 30, 2016

  • I've verified and I assure that I'm running youtube-dl 2016.08.28

Before submitting an issue make sure you have:

  • At least skimmed through README and most notably FAQ and BUGS sections
  • Searched the bugtracker for similar issues including closed ones

What is the purpose of your issue?

  • Bug report (encountered problems with youtube-dl)

The following sections concretize particular purposed issues, you can erase any section (the contents between triple ---) not applicable to your issue


If the purpose of this issue is a bug report, site support request or you are not completely sure provide the full verbose output as follows:

[info] Writing video description to: Nujabes - Modal Soul (Full Album)-WrO9PTpuSSs.description
[info] Writing video annotations to: Nujabes - Modal Soul (Full Album)-WrO9PTpuSSs.annotations.xml
[info] Writing video description metadata as JSON to: Nujabes - Modal Soul (Full Album)-WrO9PTpuSSs.info.json
[youtube] WrO9PTpuSSs: Downloading thumbnail ...
[youtube] WrO9PTpuSSs: Writing thumbnail to: Nujabes - Modal Soul (Full Album)-WrO9PTpuSSs.jpg
[download] Destination: Nujabes - Modal Soul (Full Album)-WrO9PTpuSSs.f247.webm
[download] 100% of 167.34MiB in 11:38
[download] Destination: Nujabes - Modal Soul (Full Album)-WrO9PTpuSSs.f251.webm
[download] 100% of 60.15MiB in 04:41
[ffmpeg] Merging formats into "Nujabes - Modal Soul (Full Album)-WrO9PTpuSSs.webm"
Deleting original file Nujabes - Modal Soul (Full Album)-WrO9PTpuSSs.f247.webm (pass -k to keep)
Deleting original file Nujabes - Modal Soul (Full Album)-WrO9PTpuSSs.f251.webm (pass -k to keep)
[ffmpeg] Adding metadata to 'Nujabes - Modal Soul (Full Album)-WrO9PTpuSSs.webm'
[ffmpeg] There aren't any subtitles to embed
ERROR: Only mp3 and m4a/mp4 are supported for thumbnail embedding for now.

Description of your issue, suggested solution and other information

The program crashes with --embed-thumbnails enabled when the file is not mp3/m4a/mp4.
I wanted to backup a long playlist over night, but it crashed after video 9.

Invocation:

youtube-dl --yes-playlist --write-description --write-info-json --write-annotations --write-thumbnail --all-subs --add-metadata --embed-subs --embed-thumbnail <playlist>

I wrote a small, dirty patch that prevents the crash:

diff --git a/youtube_dl/postprocessor/embedthumbnail.py b/youtube_dl/postprocessor/embedthumbnail.py
index 3bad5a2..cfc0e07 100644
--- a/youtube_dl/postprocessor/embedthumbnail.py
+++ b/youtube_dl/postprocessor/embedthumbnail.py
@@ -87,6 +87,8 @@ class EmbedThumbnailPP(FFmpegPostProcessor):
                 os.remove(encodeFilename(filename))
                 os.rename(encodeFilename(temp_filename), encodeFilename(filename))
         else:
-            raise EmbedThumbnailPPError('Only mp3 and m4a/mp4 are supported for thumbnail embedding for now.')
+            # should be correctly caught outside, but the calling system is confusing
+            # raise EmbedThumbnailPPError('Only mp3 and m4a/mp4 are supported for thumbnail embedding for now.')
+            self._downloader.report_warning('Only mp3 and m4a/mp4 are supported for thumbnail embedding for now.')

         return [], info

The nice way would be to catch the exception outside of the class and handle it correctly, but I couldn’t find out where it is called.
It should also be clarified how the semantics should be: does --embed-thumbnail guarantee that a thumbnail exists in the output? can it embed if there is a thumbnail (then the error should not exist)? does it try its best and skip if it doesn’t work?

@Tatsh
Copy link
Contributor

@Tatsh Tatsh commented Feb 21, 2017

You should make a PR.

--embed-thumbnail can fail for a lot of reasons and it especially does not work on non-YouTube sites. A workaround to skip this being fatal would be fine. Either it simply does by default or there's a new flag: --non-fatal-embed-thumbnail (only this flag and not requiring --embed-thumbnail just to keep things simple).

@Profpatsch
Copy link
Author

@Profpatsch Profpatsch commented Feb 21, 2017

You should make a PR.

As I said, the code is too confusing for me to make a nice PR that isn’t as hacky as the one above.

@532910
Copy link

@532910 532910 commented Sep 6, 2020

Is any update there through years? Why this is just ignored, thought the fix is trivial?

I got the same issue trying to use --all-formats --embed-thumbnail

@532910
Copy link

@532910 532910 commented Sep 7, 2020

look like --ignore-errors solves this issue
youtube-dl --embed-thumbnail --all-formats --ignore-errors downloads all tracks ignoring errors
@Profpatsch, could you confirm and close?

@Tatsh
Copy link
Contributor

@Tatsh Tatsh commented Sep 7, 2020

Ignoring errors is not a solution

@532910
Copy link

@532910 532910 commented Sep 7, 2020

any explanation?

@Tatsh
Copy link
Contributor

@Tatsh Tatsh commented Sep 7, 2020

All your command does is ignore any error. It would ignore errors you may want to catch and stop for. For a single video argument for this issue ignoring the error may be okay, but it's not okay for playlists or multiple video URLs as arguments.

@532910
Copy link

@532910 532910 commented Sep 7, 2020

This is exactly what I want for playlists.--ignore-errors
Moreover, there are other errors (geo restriction for example) that crash and are mitigated with --ignore-errors.
I believe each error should have a personal switch, like --ignore-errors=all --ignore-errors=thumbnail --ignore-errors=geo,thumbnail --ignore-errors=all,-geo, a new issue should be opened and this one be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.