Checking for video corruption. Processing video when preparing filname to avoid corrupt videos.

app/src/components/Admin.js

@@ -462,7 +462,7 @@ class StoppableRow extends React.Component {
                 >
                     <Modal.Header>Download Error</Modal.Header>
                     <Modal.Content>
-                        <pre>{error}</pre>
+                        <pre style={{overflowX: 'scroll'}}>{error}</pre>
                     </Modal.Content>
                     <Modal.Actions>
                         <Button onClick={() => this.setState({errorModalOpen: false})}>Close</Button>

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.
 

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
 
 

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.

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

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)

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:

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')

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