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

Generate cover [a.k.a. thumbnail/poster] for video manually uploaded #196

Merged
merged 13 commits into from
Jun 21, 2024

Conversation

deldesir
Copy link
Collaborator

@deldesir deldesir commented Jun 20, 2024

@deldesir deldesir added the enhancement New feature or request label Jun 20, 2024
@deldesir deldesir requested a review from holta June 20, 2024 03:02
@deldesir deldesir self-assigned this Jun 20, 2024
@deldesir deldesir changed the title Generate cover for video manually updated Generate cover for video manually uploaded Jun 20, 2024
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the same video
image

Copy link
Collaborator Author

@deldesir deldesir left a comment

Choose a reason for hiding this comment

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

Safe enough @codewiz?

@@ -326,8 +327,11 @@ def generate_video_cover(tmp_file_path):
ffmpeg_args = [
ffmpeg_executable,
'-i', tmp_file_path,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Defensive programming reminder: Sanitize the file path to prevent command injection

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll use shlex for now. Should be safe on POSIX compliant shells. (Done in f8c39f2)

Copy link
Member

Choose a reason for hiding this comment

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

I added a general warning that shlex.quote() is not bulletproof:

3a965f8

(Though it's probably good enough for path sanitizing!)

@deldesir deldesir added the bug Something isn't working label Jun 20, 2024
@holta
Copy link
Member

holta commented Jun 20, 2024

@holta holta changed the title Generate cover for video manually uploaded Generate cover [a.k.a. thumbnail/poster] for video manually uploaded Jun 20, 2024
@holta
Copy link
Member

holta commented Jun 20, 2024

"2025 request"

While probably not needed in 2024, the "Upload" (Upload to IIAB) button should eventually flag overweight videos that cause congestion for everyone using a school's IIAB WiFi hotspot.

▶️ We should try to warn and educate teachers/parents/students on how to compress their own locally-created overweight videos, BEFORE they're uploaded to IIAB.
▶️ Smartphone videos can usually be compressed 10X or even 20X. This works because smartphones record video at very high resolution (a.k.a. "HD" high definition, which is rarely needed during playback).
▶️ Almost any teacher worldwide can send a just-recorded video to themselves via WhatsApp, in order to force this compression to happen without much hassle.

ASIDE: Occasionally overweight videos arrive from the Internet too — e.g. when the video author/publisher has not bothered to compress their own work for easy/broad distribution...

cps/uploader.py Outdated
def generate_video_cover(tmp_file_path):
ffmpeg_executable = os.getenv('FFMPEG_PATH', 'ffmpeg')
ffmpeg_output_file = os.path.splitext(tmp_file_path)[0] + '.cover.jpg'
if not ffmpeg_executable:
Copy link

Choose a reason for hiding this comment

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

Because getenv() returns "ffmpeg" as default, this check will never succeed.

Either remove this check, or change the default to "".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point

cps/uploader.py Outdated
Comment on lines 335 to 337
sanitized_input_path = sanitize_path(tmp_file_path)
output_file_path = os.path.splitext(tmp_file_path)[0] + '.cover.jpg'
sanitized_output_path = sanitize_path(output_file_path)
Copy link

Choose a reason for hiding this comment

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

Sanitizing this path is unnecessary because it's not controlled by the user.

The path is constructed at line 301:

tmp_file_path = os.path.join(tmp_dir, md5)

Copy link

Choose a reason for hiding this comment

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

Also, I think process.Popen invokes the program directly, avoiding all problems related to shell expansion.

Copy link
Member

Choose a reason for hiding this comment

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

The path is constructed at line 301:

You mean line 391, ok:

tmp_file_path = os.path.join(tmp_dir, md5)

Copy link
Member

Choose a reason for hiding this comment

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

Also, I think process.Popen invokes the program directly, avoiding all problems related to shell expansion.

Clarify a specific recommendation?

Copy link
Member

Choose a reason for hiding this comment

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

Presumably @codewiz is talking about:

return subprocess.Popen(exc_command, shell=False, stdout=sout, stderr=serr, universal_newlines=newlines, env=env) # nosec

@deldesir please go ahead and revise (i.e. tighten up) using Bernie's recommendations, Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@holta, I did not use Calibre-Web's subprocess wrapper here because I thought it's more clear to just call it and use run from it.

Copy link
Member

Choose a reason for hiding this comment

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

@holta, I did not use Calibre-Web's subprocess wrapper here because I thought it's more clear to just call it and use run from it.

Ah, interesting!

@codewiz is this clean/solid enough, in your opinion?

Copy link
Collaborator Author

@deldesir deldesir left a comment

Choose a reason for hiding this comment

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

Recommendations applied in 2bdd296

Copy link
Member

@holta holta left a comment

Choose a reason for hiding this comment

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

Add explanatory comments back

Awesome! Allowing for easier tweaking of ffmpeg in future, if anybody chooses!

cps/uploader.py Outdated Show resolved Hide resolved
deldesir and others added 3 commits June 21, 2024 12:01
Co-authored-by: Bernie Innocenti <bernie@codewiz.org>
@holta holta merged commit 121feb5 into iiab:master Jun 21, 2024
@holta
Copy link
Member

holta commented Jun 21, 2024

  1. @deldesir can you tell why @nzola's fresh VM install is not working? (http://box/books shows "502 Bad Gateway")

    IMG-20240621-WA0002~2

    iiab-diagnostics: https://dpaste.com/FKWVETW67

  2. @deldesir can you eliminate the ffmpeg-related SyntaxWarning: invalid escape sequence going forward? As seen in his logs below:

    [2024-06-21 12:08:26,370] WARN {py.warnings:110} /usr/local/calibre-web-py3/lib/python3.12/site-packages/flask_limiter/extension.py:337: UserWarning: Using the in-memory storage for tracking rate limits as no storage was explicitly specified. This is not recommended for production use. See: https://flask-limiter.readthedocs.io#configuring-a-storage-backend for documentation about configuring the storage backend.
    warnings.warn(

    [2024-06-21 12:08:26,688] WARN {py.warnings:110} /usr/local/calibre-web-py3/cps/uploader.py:338: SyntaxWarning: invalid escape sequence ','
    '-vf', 'fps=1,thumbnail,select=gt(scene,0.1),scale=-1:720', # apply filters to avoid black frames and scale

    [2024-06-21 12:08:26,822] WARN {py.warnings:110} /usr/local/calibre-web-py3/cps/metadata_provider/lubimyczytac.py:140: SyntaxWarning: invalid escape sequence '?'
    characters_to_remove = "?()/"

@holta
Copy link
Member

holta commented Jun 21, 2024

FYI @nzola a fresh install of IIAB Calibre-Web onto Ubuntu 24.04 (VM) just worked for me.

As confirmed by iiab-diagnostics: https://dpaste.com/9DWAB47VX

@nzola
Copy link

nzola commented Jun 21, 2024

FYI @nzola a fresh install of IIAB Calibre-Web onto Ubuntu 24.04 (VM) just worked for me.

As confirmed by iiab-diagnostics: https://dpaste.com/9DWAB47VX

ubuntu@box:~$ pastebinit -b dpaste.com /var/log/nginx/error.log
https://dpaste.com/9JANY2EA6
ubuntu@box:~$ journalctl -u nginx | pastebinit -b dpaste.com
https://dpaste.com/3S2KYHC2A
ubuntu@box:~$ journalctl -u calibre-web | pastebinit -b dpaste.com
https://dpaste.com/3RASF2AMN
ubuntu@box:~$

@holta
Copy link
Member

holta commented Jun 21, 2024

@nzola you tried two erroneous URLs:

  • http://box/books (wrong box, it shows NGINX 1.22.1, which is not possible with an Ubuntu 24.04 VM, so the "502 Bad Gateway" error is irrelevant!) ❌
  • http://172.23.47.39/home showing "403 Forbidden" ❌

You need to try:

@nzola
Copy link

nzola commented Jun 22, 2024

@nzola you tried two erroneous URLs:

  • http://box/books (wrong box, it shows NGINX 1.22.1, which is not possible with an Ubuntu 24.04 VM, so the "502 Bad Gateway" error is irrelevant!) ❌
  • http://172.23.47.39/home showing "403 Forbidden" ❌

You need to try:

My vm/iiab is working now.
I turned [off] the box Lokole/iiab downstairs and now I can browse with http://172.23.47.39/books as well with http://box/books.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants