From dd07236834cfd09509190db7916c44ed0ce99b3f Mon Sep 17 00:00:00 2001 From: Roland Date: Wed, 18 May 2022 17:24:50 -0600 Subject: [PATCH] Checking for video corruption. Processing video when preparing filname to avoid corrupt videos. --- app/src/components/Admin.js | 2 +- modules/videos/common.py | 31 +++++++++++++++++++++++++++++- modules/videos/downloader.py | 9 ++++----- modules/videos/lib.py | 5 ++++- modules/videos/test/test_common.py | 27 ++++++++++++++++++++++++++ wrolpi/admin.py | 4 ++++ wrolpi/downloader.py | 6 +++++- wrolpi/root_api.py | 7 ++++++- wrolpi/test/test_root_api.py | 8 ++++++-- 9 files changed, 87 insertions(+), 12 deletions(-) diff --git a/app/src/components/Admin.js b/app/src/components/Admin.js index 4e3f32f0..588a20ca 100644 --- a/app/src/components/Admin.js +++ b/app/src/components/Admin.js @@ -462,7 +462,7 @@ class StoppableRow extends React.Component { > Download Error - 
{error}
+ 
{error}
 diff --git a/modules/videos/common.py b/modules/videos/common.py index 4008b77d..1c450f5d 100644 --- a/modules/videos/common.py +++ b/modules/videos/common.py @@ -3,6 +3,7 @@ import os import pathlib import subprocess import tempfile +from decimal import Decimal from functools import partial from pathlib import Path from typing import Union, Tuple, List, Set, Iterable, Optional @@ -254,10 +255,38 @@ def get_video_duration(video_path: Path) -> Optional[int]: logger.warning(f'FFPROBE failed to get duration with stderr: {e.stderr.decode()}') raise stdout = proc.stdout.decode() - duration = int(float(stdout.strip())) + duration = int(Decimal(stdout.strip())) return duration +def check_for_video_corruption(video_path: Path) -> bool: + """Uses ffprobe to check for specific ways a video file can be corrupt.""" + if not isinstance(video_path, Path): + video_path = Path(video_path) + if not video_path.is_file(): + raise FileNotFoundError(f'{video_path} does not exist!') + + cmd = [FFPROBE_BIN, str(video_path)] + try: + proc = subprocess.run(cmd, check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + except subprocess.CalledProcessError as e: + logger.warning(f'FFPROBE failed to check for corruption with stdout: {e.stdout.decode()}') + logger.warning(f'FFPROBE failed to check for corruption with stderr: {e.stderr.decode()}') + return True # video is corrupt. + + messages = ( + 'Invalid NAL unit size', + 'Error splitting the input into NAL units', + ) + stderr = proc.stderr.decode() + corrupt = False + for error in messages: + if error in stderr: + logger.warning(f'Possible video corruption ({error}): {video_path}') + corrupt = True + return corrupt + + def apply_info_json(channel_id: int): """Update view_count for all Videos in a channel using its info_json file. diff --git a/modules/videos/downloader.py b/modules/videos/downloader.py index 9728ed9b..2f1b9cf7 100755 --- a/modules/videos/downloader.py +++ b/modules/videos/downloader.py @@ -45,7 +45,7 @@ PREFERRED_VIDEO_FORMAT = 'best[height=720],22,720p,mp4-480p,mp4-360p,mp4-240p,18 def extract_info(url: str, ydl: YoutubeDL = YDL) -> dict: """Get info about a video. Separated for testing.""" - return ydl.extract_info(url, download=False, process=False) + return ydl.extract_info(url, download=False, process=True) def prepare_filename(entry: dict, ydl: YoutubeDL = YDL) -> str: @@ -254,6 +254,7 @@ class VideoDownloader(Downloader, ABC): options = get_downloader_config().dict() options['outtmpl'] = f'{out_dir}/{options["file_name_format"]}' options['merge_output_format'] = PREFERRED_VIDEO_EXTENSION + logger.debug(f'Download video with options: {options}') logger.debug(f'Downloading {url} to {out_dir}') @@ -265,10 +266,8 @@ class VideoDownloader(Downloader, ABC): # Get the path where the video will be saved. entry = extract_info(url, ydl=ydl) final_filename = pathlib.Path(prepare_filename(entry, ydl=ydl)).absolute() - if final_filename.suffix == '.NA': - # yt-dlp has odd behavior sometimes and ignores our preferred video extension. - final_filename = final_filename.with_suffix('.mp4') - logger.warning(f'Had to replace video extension: {final_filename}') + if final_filename.suffix.lower() != f'.{PREFERRED_VIDEO_EXTENSION}': + raise DownloadError(f'Cannot download video {url} because yt-dlp filename is invalid.') return final_filename, entry diff --git a/modules/videos/lib.py b/modules/videos/lib.py index 885e265f..651b9b86 100644 --- a/modules/videos/lib.py +++ b/modules/videos/lib.py @@ -18,7 +18,7 @@ from wrolpi.vars import PYTEST from .captions import get_captions from .common import generate_video_paths, remove_duplicate_video_paths, apply_info_json, get_video_duration, \ is_valid_poster, convert_image, generate_video_poster, logger, REQUIRED_OPTIONS, ConfigError, \ - get_no_channel_directory + get_no_channel_directory, check_for_video_corruption from .models import Channel, Video logger = logger.getChild(__name__) @@ -181,6 +181,9 @@ def validate_video(video: Video, channel_generate_poster: bool): video_path = video.video_path.path if isinstance(video.video_path, MediaPath) else video.video_path + # Check for corruption, but don't throw away the video even if it is corrupt. Errors will be logged in this call. + check_for_video_corruption(video_path) + if not video.title or not video.upload_date or not video.source_id: # Video is missing things that can be extracted from the video file name. # These are the least trusted, so anything already on the video should be trusted. diff --git a/modules/videos/test/test_common.py b/modules/videos/test/test_common.py index 059c5246..25ee41eb 100644 --- a/modules/videos/test/test_common.py +++ b/modules/videos/test/test_common.py @@ -14,6 +14,7 @@ from wrolpi.dates import local_timezone, now from wrolpi.downloader import Download, DownloadFrequency from wrolpi.test.common import build_test_directories, TestAPI from wrolpi.vars import PROJECT_DIR +from .. import common from ..common import get_matching_directories, convert_image, remove_duplicate_video_paths, \ apply_info_json, get_video_duration, generate_video_poster, is_valid_poster from ..lib import save_channels_config, get_channels_config, import_channels_config @@ -414,3 +415,29 @@ def test_import_channels_config_outdated(test_session, test_directory, channel_f # Videos were favorited by their channel "link". assert vid1.favorite and vid2.favorite and vid3.favorite assert not vid4.favorite + + +def test_check_for_video_corruption(video_file, test_directory): + # The test video is not corrupt. + assert common.check_for_video_corruption(video_file) is False + + # An empty file is corrupt. + empty_file = test_directory / 'empty file.mp4' + empty_file.touch() + assert common.check_for_video_corruption(empty_file) is True + + # A video file must be complete. + truncated_video = test_directory / 'truncated_video.mp4' + with truncated_video.open('wb') as fh: + fh.write(video_file.read_bytes()[:10000]) + assert common.check_for_video_corruption(truncated_video) is True + + # Check for specific ffprobe errors. + with mock.patch('modules.videos.common.subprocess') as mock_subprocess: + # `video_file` is ignored for these calls. + mock_subprocess.run().stderr.decode.return_value = 'Something\nInvalid NAL unit size' + assert common.check_for_video_corruption(video_file) is True + mock_subprocess.run().stderr.decode.return_value = 'Something\nError splitting the input into NAL units' + assert common.check_for_video_corruption(video_file) is True + mock_subprocess.run().stderr.decode.return_value = 'Some stderr is fine' + assert common.check_for_video_corruption(video_file) is False diff --git a/wrolpi/admin.py b/wrolpi/admin.py index c1ea968a..37b5c258 100644 --- a/wrolpi/admin.py +++ b/wrolpi/admin.py @@ -54,6 +54,7 @@ def enable_hotspot(): logger.warning(f'Hotspot status: {status}') if status == HotspotStatus.connected: + logger.info('Hotspot already connected. Restarting hotspot.') disable_hotspot() return enable_hotspot() elif status == HotspotStatus.disconnected: @@ -61,11 +62,13 @@ def enable_hotspot(): cmd = (SUDO, NMCLI, 'device', 'wifi', 'hotspot', 'ifname', 'wlan0', 'ssid', WROLPI_CONFIG.hotspot_ssid, 'password', WROLPI_CONFIG.hotspot_password) subprocess.check_call(cmd) + logger.info('Enabling hotspot successful') return True elif status == HotspotStatus.unavailable: # Radio is not on, turn it on. cmd = (SUDO, NMCLI, 'radio', 'wifi', 'on') subprocess.check_call(cmd) + logger.info('Turned on hotspot radio.') return enable_hotspot() elif status == HotspotStatus.unknown: logger.error('Cannot enable hotspot with unknown status!') @@ -76,6 +79,7 @@ def disable_hotspot(): cmd = (SUDO, NMCLI, 'radio', 'wifi', 'off') try: subprocess.check_call(cmd) + logger.info('Disabling hotspot successful') return True except Exception as e: logger.error('Failed to disable hotspot', exc_info=e) diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py index 3da16b10..55ab3551 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -182,6 +182,7 @@ class Downloader: """ Run a subprocess using the provided arguments. This process can be killed by the Download Manager. """ + logger.debug(f'{self} launching download process with args: {" ".join(cmd)}') proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd, **kwargs) pid = proc.pid logger.debug(f'{self} launched download process {pid=} for {url}') @@ -370,6 +371,9 @@ class DownloadManager: logger.warning(f'Failed to download {url}. Will be tried again later.', exc_info=e) result = DownloadResult(success=False, error=str(traceback.format_exc())) + error_len = len(result.error) if result.error else 0 + logger.debug(f'Got {result.success=} from {downloader} with {error_len=}') + # Long-running, get the download again. with get_db_session(commit=True) as session_: download = self.get_download(session_, id_=download_id) @@ -377,7 +381,7 @@ class DownloadManager: # clear out an outdated location. download.location = result.location or download.location or None # Clear any old errors if the download succeeded. - download.error = result.error if download.error else None + download.error = result.error if result.error else None download.next_download = self.get_next_download(download, session) if try_again is False: diff --git a/wrolpi/root_api.py b/wrolpi/root_api.py index f9172104..cebfde69 100644 --- a/wrolpi/root_api.py +++ b/wrolpi/root_api.py @@ -17,6 +17,7 @@ from sanic_ext import validate from sanic_ext.extensions.openapi import openapi from wrolpi import admin +from wrolpi.admin import HotspotStatus from wrolpi.common import set_sanic_url_parts, logger, get_config, wrol_mode_enabled, Base, get_media_directory, \ wrol_mode_check, native_only, set_wrol_mode from wrolpi.dates import set_timezone @@ -158,12 +159,16 @@ def update_settings(_: Request, body: SettingsRequest): # Remove any keys with None values, then save the config. config = {k: v for k, v in body.__dict__.items() if v is not None} wrolpi_config = get_config() + old_password = wrolpi_config.hotspot_password wrolpi_config.update(config) if body.wrol_mode: download_manager.kill() - if body.hotspot_status is True: + # If the password was changed, we need to restart the hotspot. + password_changed = (new_password := config.get('hotspot_password')) and old_password != new_password + + if body.hotspot_status is True or (password_changed and admin.hotspot_status() == HotspotStatus.connected): # Turn on Hotspot if admin.enable_hotspot() is False: raise HotspotError('Could not turn on hotspot') diff --git a/wrolpi/test/test_root_api.py b/wrolpi/test/test_root_api.py index a50dce75..5d973035 100644 --- a/wrolpi/test/test_root_api.py +++ b/wrolpi/test/test_root_api.py @@ -4,6 +4,7 @@ from itertools import zip_longest from mock import mock +from wrolpi.admin import HotspotStatus from wrolpi.common import get_config from wrolpi.dates import strptime from wrolpi.db import get_db_session @@ -197,8 +198,9 @@ def test_hotspot_settings(test_session, test_client, test_config): assert response.json['code'] == 35 mock_admin.disable_hotspot.assert_called_once() - config = get_config() - assert config.hotspot_on_startup is True + mock_admin.enable_hotspot.reset_mock() + mock_admin.enable_hotspot.return_value = True + mock_admin.hotspot_status.return_value = HotspotStatus.connected # Hotspot password can be changed. mock_admin.disable_hotspot.return_value = True @@ -207,6 +209,8 @@ def test_hotspot_settings(test_session, test_client, test_config): assert response.status_code == HTTPStatus.NO_CONTENT assert config.hotspot_password == 'new password' assert config.hotspot_ssid == 'new ssid' + # Changing the password restarts the hotspot. + mock_admin.enable_hotspot.assert_called_once() @skip_circleci