From 088efb97e4370aa668e0da7073e7a0c57e1a0df4 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Mon, 22 Apr 2024 15:50:54 +0200 Subject: [PATCH 01/23] Initial setup --- constants/base.py | 10 ++ .../templates/extensions/components/card.html | 6 +- .../extensions/components/galleria.html | 22 ++--- files/admin.py | 41 +++++++- files/models.py | 27 ++++- files/signals.py | 31 +++++- files/static/files/admin/file.css | 14 +++ files/tasks.py | 31 ++++++ files/templates/files/admin/thumbnails.html | 14 +++ files/utils.py | 98 +++++++++++++++++++ 10 files changed, 270 insertions(+), 24 deletions(-) create mode 100644 files/static/files/admin/file.css create mode 100644 files/templates/files/admin/thumbnails.html diff --git a/constants/base.py b/constants/base.py index 2a2a81f4..3156e685 100644 --- a/constants/base.py +++ b/constants/base.py @@ -100,3 +100,13 @@ ABUSE_TYPE = Choices( ('ABUSE_USER', ABUSE_TYPE_USER, "User"), ('ABUSE_RATING', ABUSE_TYPE_RATING, "Rating"), ) + +# **N.B.**: thumbnail sizes are not intended to be changed on the fly: +# thumbnails of existing images must exist in MEDIA_ROOT before +# the code expecting the new dimensions can be deployed! +THUMBNAIL_SIZE_DEFAULT = (1920, 1080) +THUMBNAIL_SIZE_M = (1280, 720) +THUMBNAIL_SIZE_S = (640, 360) +THUMBNAIL_SIZES = [THUMBNAIL_SIZE_DEFAULT, THUMBNAIL_SIZE_M, THUMBNAIL_SIZE_S] +THUMBNAIL_FORMAT = 'PNG' +THUMBNAIL_QUALITY = 83 diff --git a/extensions/templates/extensions/components/card.html b/extensions/templates/extensions/components/card.html index 61082b1e..9affb6fa 100644 --- a/extensions/templates/extensions/components/card.html +++ b/extensions/templates/extensions/components/card.html @@ -1,13 +1,13 @@ {% load common filters %} -{% with latest=extension.latest_version %} +{% with latest=extension.latest_version thumbnail_url=extension.previews.listed.first.thumbnail_url %}
{% if blur %} -
+
{% endif %} -
+
diff --git a/extensions/templates/extensions/components/galleria.html b/extensions/templates/extensions/components/galleria.html index b6427931..7862b678 100644 --- a/extensions/templates/extensions/components/galleria.html +++ b/extensions/templates/extensions/components/galleria.html @@ -3,19 +3,17 @@ {% if previews %} {% else %} diff --git a/files/admin.py b/files/admin.py index b794f70a..e7417cb3 100644 --- a/files/admin.py +++ b/files/admin.py @@ -1,10 +1,17 @@ +import logging + +from django.conf import settings from django.contrib import admin +from django.template.loader import render_to_string import background_task.admin import background_task.models from .models import File, FileValidation +from constants.base import THUMBNAIL_SIZE_DEFAULT import files.signals +logger = logging.getLogger(__name__) + def scan_selected_files(self, request, queryset): """Scan selected files.""" @@ -12,6 +19,12 @@ def scan_selected_files(self, request, queryset): files.signals.schedule_scan(instance) +def make_thumbnails(self, request, queryset): + """Make thumbnails for selected files.""" + for instance in queryset: + files.tasks.make_thumbnails.task_function(file_id=instance.pk) + + class FileValidationInlineAdmin(admin.StackedInline): model = FileValidation readonly_fields = ('date_created', 'date_modified', 'is_ok', 'results') @@ -27,6 +40,24 @@ class FileValidationInlineAdmin(admin.StackedInline): @admin.register(File) class FileAdmin(admin.ModelAdmin): + class Media: + css = {'all': ('files/admin/file.css',)} + + def thumbnails(self, obj): + try: + context = { + 'MEDIA_URL': settings.MEDIA_URL, + 'THUMBNAIL_SIZE_DEFAULT': THUMBNAIL_SIZE_DEFAULT, + 'thumbnail': obj.thumbnail, + 'thumbnails': obj.thumbnails, + } + return render_to_string('files/admin/thumbnails.html', context) + except Exception: + # Make sure any exception happening here is always logged + # (e.g. admin eats exception in ModelAdmin properties, making it hard to debug) + logger.exception('Failed to render thumbnails') + raise + view_on_site = False save_on_top = True @@ -48,6 +79,9 @@ class FileAdmin(admin.ModelAdmin): 'date_approved', 'date_status_changed', 'size_bytes', + 'thumbnail', + 'thumbnails', + 'type', 'user', 'original_hash', 'original_name', @@ -67,9 +101,8 @@ class FileAdmin(admin.ModelAdmin): { 'fields': ( 'id', - ('source', 'thumbnail'), - ('original_name', 'content_type'), - 'type', + ('source', 'thumbnail', 'thumbnails'), + ('type', 'content_type', 'original_name'), 'status', ) }, @@ -99,7 +132,7 @@ class FileAdmin(admin.ModelAdmin): ) inlines = [FileValidationInlineAdmin] - actions = [scan_selected_files] + actions = [scan_selected_files, make_thumbnails] def is_ok(self, obj): return obj.validation.is_ok if hasattr(obj, 'validation') else None diff --git a/files/models.py b/files/models.py index f3c73324..9976bdc8 100644 --- a/files/models.py +++ b/files/models.py @@ -6,10 +6,12 @@ from django.contrib.auth import get_user_model from django.db import models from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin -from files.utils import get_sha256, guess_mimetype_from_ext +from files.utils import get_sha256, guess_mimetype_from_ext, get_base_path from constants.base import ( FILE_STATUS_CHOICES, FILE_TYPE_CHOICES, + THUMBNAIL_FORMAT, + THUMBNAIL_SIZES, ) import utils @@ -43,13 +45,13 @@ def file_upload_to(instance, filename): def thumbnail_upload_to(instance, filename): prefix = 'thumbnails/' _hash = instance.hash.split(':')[-1] - extension = Path(filename).suffix + extension = f'.{THUMBNAIL_FORMAT.lower()}' path = Path(prefix, _hash[:2], _hash).with_suffix(extension) return path class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): - track_changes_to_fields = {'status', 'size_bytes', 'hash'} + track_changes_to_fields = {'status', 'size_bytes', 'hash', 'thumbnail'} TYPES = FILE_TYPE_CHOICES STATUSES = FILE_STATUS_CHOICES @@ -63,7 +65,8 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): null=True, blank=True, max_length=256, - help_text='Image thumbnail in case file is a video', + help_text='Image thumbnail', + editable=False, ) content_type = models.CharField(max_length=256, null=True, blank=True) type = models.PositiveSmallIntegerField( @@ -203,6 +206,22 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): def get_submit_url(self) -> str: return self.extension.get_draft_url() + @property + def thumbnails(self) -> dict: + """Return a dictionary with relative URLs of a set of predefined thumbnails.""" + base_path = get_base_path(thumbnail_upload_to(self, '')) + extension = f'.{THUMBNAIL_FORMAT.lower()}' + return {size: f'{base_path}_{size[0]}x{size[1]}{extension}' for size in THUMBNAIL_SIZES} + + @property + def thumbnail_url(self) -> str: + """Log absence of the thumbnail file instead of exploding somewhere in the templates.""" + try: + return self.thumbnail.url + except ValueError: + log.exception(f'File pk={self.pk} is missing thumbnail(s)') + return '' + class FileValidation(CreatedModifiedMixin, TrackChangesMixin, models.Model): track_changes_to_fields = {'is_ok', 'results'} diff --git a/files/signals.py b/files/signals.py index fceb3086..313253c3 100644 --- a/files/signals.py +++ b/files/signals.py @@ -1,10 +1,11 @@ import logging -from django.db.models.signals import pre_save, post_save, pre_delete +from django.db.models.signals import pre_save, post_save, pre_delete, post_delete from django.dispatch import receiver import files.models import files.tasks +import files.utils logger = logging.getLogger(__name__) @@ -35,7 +36,35 @@ def _scan_new_file( schedule_scan(instance) +def schedule_thumbnails(file: files.models.File) -> None: + """Schedule thumbnail generation for a given file.""" + args = {'pk': file.pk, 'type': file.get_type_display()} + logger.info('Scheduling thumbnail generation for file pk=%s type=%s', args) + verbose_name = f'make thumbnails for "{file.source.name}"' + files.tasks.make_thumbnails(file_id=file.pk, creator=file, verbose_name=verbose_name) + + +@receiver(post_save, sender=files.models.FileValidation) +def _schedule_thumbnails( + sender: object, instance: files.models.FileValidation, created: bool, **kwargs: object +) -> None: + if not created: + return + + file = instance.file + if instance.is_ok and (file.is_image or file.is_video): + # Generate thumbnails if initial scan found no issues + schedule_thumbnails(file) + + @receiver(pre_delete, sender=files.models.File) @receiver(pre_delete, sender=files.models.FileValidation) def _log_deletion(sender: object, instance: files.models.File, **kwargs: object) -> None: instance.record_deletion() + + +@receiver(post_delete, sender=files.models.File) +def delete_orphaned_files(sender: object, instance: files.models.File, **kwargs: object) -> None: + """Delete source and thumbnail files from storage when File record is deleted.""" + files.utils.delete_source_file(instance.source.name) + files.utils.delete_thumbnail_file(instance.thumbnail.name) diff --git a/files/static/files/admin/file.css b/files/static/files/admin/file.css new file mode 100644 index 00000000..f44f732b --- /dev/null +++ b/files/static/files/admin/file.css @@ -0,0 +1,14 @@ +.file-thumbnail { + display: inline-block; + border: grey solid 1px; + margin-left: 0.5rem; +} +.file-thumbnail-size { + position: absolute; + background: rgba(255, 255, 255, 0.5); + padding-right: 0.5rem; + padding-left: 0.5rem; +} +.file-thumbnails .icon-alert { + color: red; +} diff --git a/files/tasks.py b/files/tasks.py index 76c570b0..8123804b 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -5,6 +5,7 @@ from background_task import background from background_task.tasks import TaskSchedule from django.conf import settings +from constants.base import THUMBNAIL_FORMAT, THUMBNAIL_SIZE_DEFAULT, THUMBNAIL_QUALITY import files.models import files.utils @@ -27,3 +28,33 @@ def clamdscan(file_id: int): file_validation.results = scan_result file_validation.is_ok = is_ok file_validation.save(update_fields={'results', 'is_ok', 'date_modified'}) + + +@background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) +def make_thumbnails(file_id: int) -> None: + """Generate thumbnails for a given file.""" + file = files.models.File.objects.get(pk=file_id) + abs_path = os.path.join(settings.MEDIA_ROOT, file.source.path) + + # TODO: For an image, source of the thumbnail is file.source itself + if file.is_image: + image_field = file.thumbnail + if not image_field: + # Use the source image if max-size thumbnail is not yet available + image_field.name = file.source.name + # base_path = files.utils.get_base_path(image_field.name) + thumbnails_paths = file.thumbnails + files.utils.make_thumbnails( + abs_path, + thumbnails_paths, + output_format=THUMBNAIL_FORMAT, + quality=THUMBNAIL_QUALITY, + optimize=True, + progressive=True, + ) + image_field.name = thumbnails_paths[THUMBNAIL_SIZE_DEFAULT] + file.save(update_fields={'thumbnail'}) + # TODO: For a video, source of the thumbnail is some frame fetched with ffpeg + elif file.is_video: + raise NotImplementedError + # TODO: file.thumbnail field is updated diff --git a/files/templates/files/admin/thumbnails.html b/files/templates/files/admin/thumbnails.html new file mode 100644 index 00000000..61291b17 --- /dev/null +++ b/files/templates/files/admin/thumbnails.html @@ -0,0 +1,14 @@ +
+{% for size, thumb_url in thumbnails.items %} +
+ {{ size.0 }}x{{ size.1 }}px + +
+{% endfor %} +{% if thumbnail.width != THUMBNAIL_SIZE_DEFAULT.0 or thumbnail.height != THUMBNAIL_SIZE_DEFAULT.1 %} +

+ ⚠ Expected {{ THUMBNAIL_SIZE_DEFAULT.0 }}x{{ THUMBNAIL_SIZE_DEFAULT.1 }}px, + got {{ thumbnail.width }}x{{ thumbnail.height }}px: resolution of the source file might be too low? +

+{% endif %} +
diff --git a/files/utils.py b/files/utils.py index f88ffef6..8739380f 100644 --- a/files/utils.py +++ b/files/utils.py @@ -5,14 +5,19 @@ import logging import mimetypes import os import os.path +import tempfile import toml import typing import zipfile +from PIL import Image +from django.core.files.storage import default_storage from lxml import etree import clamd import magic +from constants.base import THUMBNAIL_FORMAT, THUMBNAIL_SIZES + logger = logging.getLogger(__name__) MODULE_DIR = Path(__file__).resolve().parent THEME_SCHEMA = [] @@ -172,3 +177,96 @@ def run_clamdscan(abs_path: str) -> tuple: result = clamd_socket.instream(f)['stream'] logger.info('File at path=%s scanned: %s', abs_path, result) return result + + +def get_base_path(file_path: str) -> str: + """Return the given path without its extension.""" + base_path, _ = os.path.splitext(file_path) + return base_path + + +def _resize(image: Image, size: tuple, output, output_format: str = 'PNG', **output_params): + """Resize a models.ImageField to a given size and write it into output file.""" + source_image = image.convert('RGBA' if output_format == 'PNG' else 'RGB') + source_image.thumbnail(size, Image.LANCZOS) + source_image.save(output, output_format, **output_params) + + +def make_thumbnails(image_field, output_paths: dict, output_format: str = 'PNG', **output_params): + """ + Generate thumbnail files for given models.ImageField and list of dimensions. + + Currently only intended to be used manually from shell, e.g.: + + from files.models import File + import files.utils + files_wo_thumbs = File.objects.filter(type__in=(File.TYPES.IMAGE, File.TYPES.VIDEO)) + for b in badges: + # TODO: thumbnail should be set to THUMBNAIL_SIZE_DEFAULT of the source for IMAGE + base_path = files.utils.get_base_path(b.thumbnail.path) + files.utils.make_thumbnails(b.thumbnail, base_path, THUMBNAIL_SIZES) + + """ + thumbnail_ext = output_format.lower() + if thumbnail_ext == 'jpeg': + thumbnail_ext = 'jpg' + thumbnails = {} + for (w, h), output_path in output_paths.items(): + # output_path = f'{base_path}_{w}x{h}.{thumbnail_ext}' + with tempfile.TemporaryFile() as f: + size = (w, h) + logger.info('Resizing %s to %s (%s)', image_field, size, output_format) + image = Image.open(image_field) + _resize(image, size, f, output_format=output_format, **output_params) + logger.info('Saving a new thumbnail to %s', output_path) + if default_storage.exists(output_path): + logger.warning('%s exists, overwriting', output_path) + default_storage.delete(output_path) + default_storage.save(output_path, f) + thumbnails[size] = output_path + return thumbnails + + +def get_thumbnails_paths(file_name: str) -> dict: + # Should we check actual existence of the file? + # if not storage.exists(file_name): + # return {} + base_name, _ = os.path.splitext(file_name) + thumbnail_ext = THUMBNAIL_FORMAT.lower() + if thumbnail_ext == 'jpeg': + thumbnail_ext = 'jpg' + return { + size_in_px: f'{base_name}_{size_in_px}x{size_in_px}.{thumbnail_ext}' + for size_in_px in THUMBNAIL_SIZES + } + + +def _delete_file_in_storage(file_name: str) -> None: + if not file_name: + return + + if not default_storage.exists(file_name): + logger.warning("%s doesn't exist in storage, nothing to delete", file_name) + else: + logger.info('Deleting %s from storage', file_name) + default_storage.delete(file_name) + + +def delete_thumbnail_file(file_name: str) -> None: + _delete_file_in_storage(file_name) + + # Also delete associated thumbnails + thumbnail_names = get_thumbnails_paths(file_name).values() + for thumbnail_name in thumbnail_names: + if not default_storage.exists(thumbnail_name): + continue + logger.info( + 'Deleting thumbnail %s from storage because %s was also deleted', + thumbnail_name, + file_name, + ) + default_storage.delete(thumbnail_name) + + +def delete_source_file(file_name: str) -> None: + _delete_file_in_storage(file_name) -- 2.30.2 From 028a3f0a378fe7b95b0fa40dc1587b70603d4d2f Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Mon, 22 Apr 2024 19:01:41 +0200 Subject: [PATCH 02/23] Save thumbnails as metadata --- constants/base.py | 3 +-- extensions/tests/test_delete.py | 6 +++++- files/admin.py | 16 +++++++++++++--- files/models.py | 4 ++-- files/tasks.py | 21 +++++++++++++++------ files/tests/test_models.py | 4 +++- 6 files changed, 39 insertions(+), 15 deletions(-) diff --git a/constants/base.py b/constants/base.py index 3156e685..5b993b19 100644 --- a/constants/base.py +++ b/constants/base.py @@ -105,8 +105,7 @@ ABUSE_TYPE = Choices( # thumbnails of existing images must exist in MEDIA_ROOT before # the code expecting the new dimensions can be deployed! THUMBNAIL_SIZE_DEFAULT = (1920, 1080) -THUMBNAIL_SIZE_M = (1280, 720) THUMBNAIL_SIZE_S = (640, 360) -THUMBNAIL_SIZES = [THUMBNAIL_SIZE_DEFAULT, THUMBNAIL_SIZE_M, THUMBNAIL_SIZE_S] +THUMBNAIL_SIZES = [THUMBNAIL_SIZE_DEFAULT, THUMBNAIL_SIZE_S] THUMBNAIL_FORMAT = 'PNG' THUMBNAIL_QUALITY = 83 diff --git a/extensions/tests/test_delete.py b/extensions/tests/test_delete.py index c4c54f2c..bb8d8beb 100644 --- a/extensions/tests/test_delete.py +++ b/extensions/tests/test_delete.py @@ -1,7 +1,8 @@ +from pathlib import Path import json from django.contrib.admin.models import LogEntry, DELETION -from django.test import TestCase # , TransactionTestCase +from django.test import TestCase, override_settings from common.tests.factories.extensions import create_approved_version, create_version from common.tests.factories.files import FileFactory @@ -10,7 +11,10 @@ import extensions.models import files.models import reviewers.models +TEST_MEDIA_DIR = Path(__file__).resolve().parent / 'media' + +@override_settings(MEDIA_ROOT=TEST_MEDIA_DIR) class DeleteTest(TestCase): fixtures = ['dev', 'licenses'] diff --git a/files/admin.py b/files/admin.py index e7417cb3..d01f0bc0 100644 --- a/files/admin.py +++ b/files/admin.py @@ -21,7 +21,7 @@ def scan_selected_files(self, request, queryset): def make_thumbnails(self, request, queryset): """Make thumbnails for selected files.""" - for instance in queryset: + for instance in queryset.filter(type__in=(File.TYPES.IMAGE, File.TYPE.VIDEO)): files.tasks.make_thumbnails.task_function(file_id=instance.pk) @@ -44,6 +44,8 @@ class FileAdmin(admin.ModelAdmin): css = {'all': ('files/admin/file.css',)} def thumbnails(self, obj): + if not obj or not (obj.is_image or obj.is_video): + return '' try: context = { 'MEDIA_URL': settings.MEDIA_URL, @@ -58,6 +60,14 @@ class FileAdmin(admin.ModelAdmin): logger.exception('Failed to render thumbnails') raise + def get_form(self, request, obj=None, **kwargs): + # Only override if the obj exisits + if obj: + if obj.is_image or obj.is_video: + help_text = 'Additional information about the file, e.g. existing thumbnails.' + kwargs.update({'help_texts': {'metadata': help_text}}) + return super().get_form(request, obj, **kwargs) + view_on_site = False save_on_top = True @@ -79,8 +89,8 @@ class FileAdmin(admin.ModelAdmin): 'date_approved', 'date_status_changed', 'size_bytes', - 'thumbnail', 'thumbnails', + 'thumbnail', 'type', 'user', 'original_hash', @@ -101,7 +111,7 @@ class FileAdmin(admin.ModelAdmin): { 'fields': ( 'id', - ('source', 'thumbnail', 'thumbnails'), + ('source', 'thumbnails', 'thumbnail'), ('type', 'content_type', 'original_name'), 'status', ) diff --git a/files/models.py b/files/models.py index 9976bdc8..85dddb5d 100644 --- a/files/models.py +++ b/files/models.py @@ -51,7 +51,7 @@ def thumbnail_upload_to(instance, filename): class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): - track_changes_to_fields = {'status', 'size_bytes', 'hash', 'thumbnail'} + track_changes_to_fields = {'status', 'size_bytes', 'hash', 'thumbnail', 'metadata'} TYPES = FILE_TYPE_CHOICES STATUSES = FILE_STATUS_CHOICES @@ -100,7 +100,7 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): default=dict, blank=True, # TODO add link to the manifest file user manual page. - help_text=('Meta information that was parsed from the `manifest file.'), + help_text=('Meta information that was parsed from the manifest file.'), ) objects = FileManager() diff --git a/files/tasks.py b/files/tasks.py index 8123804b..8266026e 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -38,11 +38,10 @@ def make_thumbnails(file_id: int) -> None: # TODO: For an image, source of the thumbnail is file.source itself if file.is_image: - image_field = file.thumbnail - if not image_field: + thumbnail_field = file.thumbnail + if not thumbnail_field: # Use the source image if max-size thumbnail is not yet available - image_field.name = file.source.name - # base_path = files.utils.get_base_path(image_field.name) + thumbnail_field.name = file.source.name thumbnails_paths = file.thumbnails files.utils.make_thumbnails( abs_path, @@ -52,8 +51,18 @@ def make_thumbnails(file_id: int) -> None: optimize=True, progressive=True, ) - image_field.name = thumbnails_paths[THUMBNAIL_SIZE_DEFAULT] - file.save(update_fields={'thumbnail'}) + thumbnail_default_path = thumbnails_paths[THUMBNAIL_SIZE_DEFAULT] + # Reverse to make JSON-serialisable + thumbnail_metadata = {path: size for size, path in thumbnails_paths.items()} + update_fields = {} + if thumbnail_default_path != thumbnail_field.name: + thumbnail_field.name = thumbnail_default_path + update_fields.add('thumbnail') + if file.metadata.get('thumbnails') != thumbnail_metadata: + file.metadata.update({'thumbnails': thumbnail_metadata}) + update_fields.add('metadata') + if update_fields: + file.save(update_fields=update_fields) # TODO: For a video, source of the thumbnail is some frame fetched with ffpeg elif file.is_video: raise NotImplementedError diff --git a/files/tests/test_models.py b/files/tests/test_models.py index cb92e646..732f2ec9 100644 --- a/files/tests/test_models.py +++ b/files/tests/test_models.py @@ -42,9 +42,11 @@ class FileTest(TestCase): 'new_state': {'status': 'Approved'}, 'object': '', 'old_state': { - 'status': 2, 'hash': 'foobar', + 'metadata': {}, 'size_bytes': 7149, + 'status': 2, + 'thumbnail': '', }, } }, -- 2.30.2 From 6a6b1daecff3ba59c8437b29a52b8d6278922b3a Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Tue, 23 Apr 2024 13:37:18 +0200 Subject: [PATCH 03/23] Use small thumbnail in cards --- constants/base.py | 7 +- .../templates/extensions/components/card.html | 6 +- .../extensions/components/galleria.html | 6 +- files/admin.py | 9 +-- files/migrations/0008_alter_file_thumbnail.py | 19 +++++ files/models.py | 41 +++++----- files/signals.py | 5 +- files/tasks.py | 72 ++++++++++-------- files/templates/files/admin/thumbnails.html | 12 +-- files/utils.py | 76 +++++++++++-------- requirements.txt | 1 + .../reviewers/extensions_review_detail.html | 14 ++-- 12 files changed, 152 insertions(+), 116 deletions(-) create mode 100644 files/migrations/0008_alter_file_thumbnail.py diff --git a/constants/base.py b/constants/base.py index 5b993b19..af06d22b 100644 --- a/constants/base.py +++ b/constants/base.py @@ -103,9 +103,10 @@ ABUSE_TYPE = Choices( # **N.B.**: thumbnail sizes are not intended to be changed on the fly: # thumbnails of existing images must exist in MEDIA_ROOT before -# the code expecting the new dimensions can be deployed! -THUMBNAIL_SIZE_DEFAULT = (1920, 1080) +# the code expecting thumbnails of new dimensions can be deployed! +THUMBNAIL_SIZE_L = (1920, 1080) THUMBNAIL_SIZE_S = (640, 360) -THUMBNAIL_SIZES = [THUMBNAIL_SIZE_DEFAULT, THUMBNAIL_SIZE_S] +THUMBNAIL_SIZES = [THUMBNAIL_SIZE_L, THUMBNAIL_SIZE_S] +THUMBNAIL_META = {'l': THUMBNAIL_SIZE_L, 's': THUMBNAIL_SIZE_S} THUMBNAIL_FORMAT = 'PNG' THUMBNAIL_QUALITY = 83 diff --git a/extensions/templates/extensions/components/card.html b/extensions/templates/extensions/components/card.html index 9affb6fa..689ae3e9 100644 --- a/extensions/templates/extensions/components/card.html +++ b/extensions/templates/extensions/components/card.html @@ -1,13 +1,13 @@ {% load common filters %} -{% with latest=extension.latest_version thumbnail_url=extension.previews.listed.first.thumbnail_url %} +{% with latest=extension.latest_version thumbnail_s_url=extension.previews.listed.first.thumbnail_s_url %}
{% if blur %} -
+
{% endif %} -
+
diff --git a/extensions/templates/extensions/components/galleria.html b/extensions/templates/extensions/components/galleria.html index 7862b678..b94b5d02 100644 --- a/extensions/templates/extensions/components/galleria.html +++ b/extensions/templates/extensions/components/galleria.html @@ -3,15 +3,15 @@ {% if previews %}
{% for preview in previews %} - {% with thumbnail_url=preview.thumbnail_url %} + {% with thumbnail_l_url=preview.thumbnail_l_url %} - {{ preview.extension_preview.first.caption }} + {{ preview.extension_preview.first.caption }} {% endwith %} {% endfor %} diff --git a/files/admin.py b/files/admin.py index d01f0bc0..54b9d6bb 100644 --- a/files/admin.py +++ b/files/admin.py @@ -7,7 +7,7 @@ import background_task.admin import background_task.models from .models import File, FileValidation -from constants.base import THUMBNAIL_SIZE_DEFAULT +from constants.base import THUMBNAIL_SIZE_L import files.signals logger = logging.getLogger(__name__) @@ -21,7 +21,7 @@ def scan_selected_files(self, request, queryset): def make_thumbnails(self, request, queryset): """Make thumbnails for selected files.""" - for instance in queryset.filter(type__in=(File.TYPES.IMAGE, File.TYPE.VIDEO)): + for instance in queryset.filter(type__in=(File.TYPES.IMAGE, File.TYPES.VIDEO)): files.tasks.make_thumbnails.task_function(file_id=instance.pk) @@ -48,10 +48,9 @@ class FileAdmin(admin.ModelAdmin): return '' try: context = { + 'file': obj, 'MEDIA_URL': settings.MEDIA_URL, - 'THUMBNAIL_SIZE_DEFAULT': THUMBNAIL_SIZE_DEFAULT, - 'thumbnail': obj.thumbnail, - 'thumbnails': obj.thumbnails, + 'THUMBNAIL_SIZE_L': THUMBNAIL_SIZE_L, } return render_to_string('files/admin/thumbnails.html', context) except Exception: diff --git a/files/migrations/0008_alter_file_thumbnail.py b/files/migrations/0008_alter_file_thumbnail.py new file mode 100644 index 00000000..f05a4598 --- /dev/null +++ b/files/migrations/0008_alter_file_thumbnail.py @@ -0,0 +1,19 @@ +# Generated by Django 4.2.11 on 2024-04-23 10:31 + +from django.db import migrations, models +import files.models + + +class Migration(migrations.Migration): + + dependencies = [ + ('files', '0007_alter_file_status'), + ] + + operations = [ + migrations.AlterField( + model_name='file', + name='thumbnail', + field=models.ImageField(blank=True, editable=False, help_text='Thumbnail generated from uploaded image or video source file', max_length=256, null=True, upload_to=files.models.thumbnail_upload_to), + ), + ] diff --git a/files/models.py b/files/models.py index 85dddb5d..3d8e7a76 100644 --- a/files/models.py +++ b/files/models.py @@ -6,13 +6,8 @@ from django.contrib.auth import get_user_model from django.db import models from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin -from files.utils import get_sha256, guess_mimetype_from_ext, get_base_path -from constants.base import ( - FILE_STATUS_CHOICES, - FILE_TYPE_CHOICES, - THUMBNAIL_FORMAT, - THUMBNAIL_SIZES, -) +from files.utils import get_sha256, guess_mimetype_from_ext, get_thumbnail_upload_to +from constants.base import FILE_STATUS_CHOICES, FILE_TYPE_CHOICES import utils User = get_user_model() @@ -43,11 +38,7 @@ def file_upload_to(instance, filename): def thumbnail_upload_to(instance, filename): - prefix = 'thumbnails/' - _hash = instance.hash.split(':')[-1] - extension = f'.{THUMBNAIL_FORMAT.lower()}' - path = Path(prefix, _hash[:2], _hash).with_suffix(extension) - return path + return get_thumbnail_upload_to(instance.hash) class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): @@ -65,7 +56,7 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): null=True, blank=True, max_length=256, - help_text='Image thumbnail', + help_text='Thumbnail generated from uploaded image or video source file', editable=False, ) content_type = models.CharField(max_length=256, null=True, blank=True) @@ -100,7 +91,7 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): default=dict, blank=True, # TODO add link to the manifest file user manual page. - help_text=('Meta information that was parsed from the manifest file.'), + help_text=('Meta information that was parsed from the `manifest file.'), ) objects = FileManager() @@ -207,20 +198,22 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): return self.extension.get_draft_url() @property - def thumbnails(self) -> dict: - """Return a dictionary with relative URLs of a set of predefined thumbnails.""" - base_path = get_base_path(thumbnail_upload_to(self, '')) - extension = f'.{THUMBNAIL_FORMAT.lower()}' - return {size: f'{base_path}_{size[0]}x{size[1]}{extension}' for size in THUMBNAIL_SIZES} - - @property - def thumbnail_url(self) -> str: + def thumbnail_l_url(self) -> str: """Log absence of the thumbnail file instead of exploding somewhere in the templates.""" try: return self.thumbnail.url except ValueError: - log.exception(f'File pk={self.pk} is missing thumbnail(s)') - return '' + log.exception(f'File pk={self.pk} is missing a large thumbnail') + return self.source.url + + @property + def thumbnail_s_url(self) -> str: + """Log absence of the thumbnail file instead of exploding somewhere in the templates.""" + try: + return self.metadata['thumbnails']['s']['path'] + except KeyError: + log.exception(f'File pk={self.pk} is missing a small thumbnail') + return self.source.url class FileValidation(CreatedModifiedMixin, TrackChangesMixin, models.Model): diff --git a/files/signals.py b/files/signals.py index 313253c3..907e9f59 100644 --- a/files/signals.py +++ b/files/signals.py @@ -66,5 +66,6 @@ def _log_deletion(sender: object, instance: files.models.File, **kwargs: object) @receiver(post_delete, sender=files.models.File) def delete_orphaned_files(sender: object, instance: files.models.File, **kwargs: object) -> None: """Delete source and thumbnail files from storage when File record is deleted.""" - files.utils.delete_source_file(instance.source.name) - files.utils.delete_thumbnail_file(instance.thumbnail.name) + files.utils.delete_file_in_storage(instance.source.name) + files.utils.delete_file_in_storage(instance.thumbnail.name) + files.utils.delete_thumbnails(instance.metadata) diff --git a/files/tasks.py b/files/tasks.py index 8266026e..d798334d 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -5,7 +5,7 @@ from background_task import background from background_task.tasks import TaskSchedule from django.conf import settings -from constants.base import THUMBNAIL_FORMAT, THUMBNAIL_SIZE_DEFAULT, THUMBNAIL_QUALITY +from constants.base import THUMBNAIL_FORMAT, THUMBNAIL_SIZE_L, THUMBNAIL_QUALITY, THUMBNAIL_META import files.models import files.utils @@ -34,36 +34,42 @@ def clamdscan(file_id: int): def make_thumbnails(file_id: int) -> None: """Generate thumbnails for a given file.""" file = files.models.File.objects.get(pk=file_id) - abs_path = os.path.join(settings.MEDIA_ROOT, file.source.path) + args = {'pk': file_id, 'type': file.get_type_display()} + if not file.is_image and not file.is_video: + logger.warning('File pk=%(pk)s is neither an image nor a video ("%(type)s")', args) + return - # TODO: For an image, source of the thumbnail is file.source itself - if file.is_image: - thumbnail_field = file.thumbnail - if not thumbnail_field: - # Use the source image if max-size thumbnail is not yet available - thumbnail_field.name = file.source.name - thumbnails_paths = file.thumbnails - files.utils.make_thumbnails( - abs_path, - thumbnails_paths, - output_format=THUMBNAIL_FORMAT, - quality=THUMBNAIL_QUALITY, - optimize=True, - progressive=True, - ) - thumbnail_default_path = thumbnails_paths[THUMBNAIL_SIZE_DEFAULT] - # Reverse to make JSON-serialisable - thumbnail_metadata = {path: size for size, path in thumbnails_paths.items()} - update_fields = {} - if thumbnail_default_path != thumbnail_field.name: - thumbnail_field.name = thumbnail_default_path - update_fields.add('thumbnail') - if file.metadata.get('thumbnails') != thumbnail_metadata: - file.metadata.update({'thumbnails': thumbnail_metadata}) - update_fields.add('metadata') - if update_fields: - file.save(update_fields=update_fields) - # TODO: For a video, source of the thumbnail is some frame fetched with ffpeg - elif file.is_video: - raise NotImplementedError - # TODO: file.thumbnail field is updated + assert file.validation.is_ok, f'File pk={file_id} is flagged' + + source_path = os.path.join(settings.MEDIA_ROOT, file.source.path) + thumbnail_field = file.thumbnail + thumbnails_paths = file.thumbnails + thumbnail_default_path = thumbnails_paths[THUMBNAIL_SIZE_L] + + # For a video, source of the thumbnail is some frame fetched with ffpeg + if file.is_video: + output_path = os.path.join(settings.MEDIA_ROOT, thumbnail_default_path) + files.utils.extract_frame(source_path, output_path) + source_path = output_path + + files.utils.make_thumbnails( + source_path, + thumbnails_paths, + output_format=THUMBNAIL_FORMAT, + quality=THUMBNAIL_QUALITY, + optimize=True, + progressive=True, + ) + + update_fields = set() + if thumbnail_default_path != thumbnail_field.name: + thumbnail_field.name = thumbnail_default_path + update_fields.add('thumbnail') + thumbnail_metadata = { + key: {'size': size, 'path': thumbnails_paths[size]} for key, size in THUMBNAIL_META.items() + } + if file.metadata.get('thumbnails') != thumbnail_metadata: + file.metadata.update({'thumbnails': thumbnail_metadata}) + update_fields.add('metadata') + if update_fields: + file.save(update_fields=update_fields) diff --git a/files/templates/files/admin/thumbnails.html b/files/templates/files/admin/thumbnails.html index 61291b17..e377e6e2 100644 --- a/files/templates/files/admin/thumbnails.html +++ b/files/templates/files/admin/thumbnails.html @@ -1,14 +1,14 @@
-{% for size, thumb_url in thumbnails.items %} +{% for key, thumb in file.metadata.thumbnails.items %}
- {{ size.0 }}x{{ size.1 }}px - + {{ thumb.size.0 }}x{{ thumb.size.1 }}px +
{% endfor %} -{% if thumbnail.width != THUMBNAIL_SIZE_DEFAULT.0 or thumbnail.height != THUMBNAIL_SIZE_DEFAULT.1 %} +{% if file.thumbnail.width != THUMBNAIL_SIZE_L.0 or file.thumbnail.height != THUMBNAIL_SIZE_L.1 %}

- ⚠ Expected {{ THUMBNAIL_SIZE_DEFAULT.0 }}x{{ THUMBNAIL_SIZE_DEFAULT.1 }}px, - got {{ thumbnail.width }}x{{ thumbnail.height }}px: resolution of the source file might be too low? + ⚠ Expected {{ THUMBNAIL_SIZE_L.0 }}x{{ THUMBNAIL_SIZE_L.1 }}px, + got {{ file.thumbnail.width }}x{{ file.thumbnail.height }}px: resolution of the source file might be too low?

{% endif %}
diff --git a/files/utils.py b/files/utils.py index 8739380f..7ff60393 100644 --- a/files/utils.py +++ b/files/utils.py @@ -12,6 +12,7 @@ import zipfile from PIL import Image from django.core.files.storage import default_storage +from ffmpeg import FFmpeg, FFmpegFileNotFound, FFmpegInvalidCommand, FFmpegError from lxml import etree import clamd import magic @@ -202,7 +203,7 @@ def make_thumbnails(image_field, output_paths: dict, output_format: str = 'PNG', import files.utils files_wo_thumbs = File.objects.filter(type__in=(File.TYPES.IMAGE, File.TYPES.VIDEO)) for b in badges: - # TODO: thumbnail should be set to THUMBNAIL_SIZE_DEFAULT of the source for IMAGE + # TODO: thumbnail should be set to THUMBNAIL_SIZE_L of the source for IMAGE base_path = files.utils.get_base_path(b.thumbnail.path) files.utils.make_thumbnails(b.thumbnail, base_path, THUMBNAIL_SIZES) @@ -227,21 +228,44 @@ def make_thumbnails(image_field, output_paths: dict, output_format: str = 'PNG', return thumbnails -def get_thumbnails_paths(file_name: str) -> dict: - # Should we check actual existence of the file? - # if not storage.exists(file_name): - # return {} - base_name, _ = os.path.splitext(file_name) - thumbnail_ext = THUMBNAIL_FORMAT.lower() - if thumbnail_ext == 'jpeg': - thumbnail_ext = 'jpg' - return { - size_in_px: f'{base_name}_{size_in_px}x{size_in_px}.{thumbnail_ext}' - for size_in_px in THUMBNAIL_SIZES - } +def get_thumbnail_upload_to(file_hash: str): + prefix = 'thumbnails/' + _hash = file_hash.split(':')[-1] + extension = f'.{THUMBNAIL_FORMAT.lower()}' + path = Path(prefix, _hash[:2], _hash).with_suffix(extension) + return path -def _delete_file_in_storage(file_name: str) -> None: +def get_thumbnails_paths(file_hash: str) -> dict: + """Return a dictionary of paths, derived from hash and predefined thumbnails dimensions.""" + base_path = get_base_path(get_thumbnail_upload_to(file_hash)) + extension = f'.{THUMBNAIL_FORMAT.lower()}' + return {size: f'{base_path}_{size[0]}x{size[1]}{extension}' for size in THUMBNAIL_SIZES} + + +def extract_frame(source_path: str, output_path: str): + """Extract a single frame of a video at a given path, write it to the given output path.""" + try: + ffmpeg = ( + FFmpeg() + .option('y') + .input(source_path) + .output( + output_path, + {'ss': '00:00:00.01', 'frames:v': 1, 'update': 'true'}, + ) + ) + output_dir = os.path.dirname(output_path) + if not os.path.isdir(output_dir): + os.mkdir(output_dir) + ffmpeg.execute() + except (FFmpegError, FFmpegFileNotFound, FFmpegInvalidCommand) as e: + logger.exception(f'Failed to extract a frame: {e.message}, {" ".join(ffmpeg.arguments)}') + raise + + +def delete_file_in_storage(file_name: str) -> None: + """Delete file from disk or whatever other default storage.""" if not file_name: return @@ -252,21 +276,11 @@ def _delete_file_in_storage(file_name: str) -> None: default_storage.delete(file_name) -def delete_thumbnail_file(file_name: str) -> None: - _delete_file_in_storage(file_name) - - # Also delete associated thumbnails - thumbnail_names = get_thumbnails_paths(file_name).values() - for thumbnail_name in thumbnail_names: - if not default_storage.exists(thumbnail_name): +def delete_thumbnails(file_metadata: dict) -> None: + """Read thumbnail paths from given metadata and delete them from storage.""" + thumbnails = file_metadata.get('thumbnails', {}) + for _, thumb in thumbnails.items: + path = thumb.get('path', '') + if not path: continue - logger.info( - 'Deleting thumbnail %s from storage because %s was also deleted', - thumbnail_name, - file_name, - ) - default_storage.delete(thumbnail_name) - - -def delete_source_file(file_name: str) -> None: - _delete_file_in_storage(file_name) + delete_file_in_storage(path) diff --git a/requirements.txt b/requirements.txt index 1f02303a..fdf11ec8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -40,6 +40,7 @@ mistune==2.0.4 multidict==6.0.2 oauthlib==3.2.0 Pillow==9.2.0 +python-ffmpeg==2.0.12 python-magic==0.4.27 requests==2.28.1 requests-oauthlib==1.3.1 diff --git a/reviewers/templates/reviewers/extensions_review_detail.html b/reviewers/templates/reviewers/extensions_review_detail.html index e2598645..b870e048 100644 --- a/reviewers/templates/reviewers/extensions_review_detail.html +++ b/reviewers/templates/reviewers/extensions_review_detail.html @@ -76,12 +76,14 @@

Previews Pending Approval

{% for preview in pending_previews %} -
- - {{ preview.caption }} - - {% include "common/components/status.html" with object=preview.file class="d-block" %} -
+ {% with thumbnail_l_url=preview.file.thumbnail_l_url %} +
+ + {{ preview.caption }} + + {% include "common/components/status.html" with object=preview.file class="d-block" %} +
+ {% endwith %} {% endfor %}
-- 2.30.2 From adbc385197606d74f346cf99da2d217f7f6f2c89 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Tue, 23 Apr 2024 14:54:24 +0200 Subject: [PATCH 04/23] Typo --- files/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/files/admin.py b/files/admin.py index 54b9d6bb..66206ac6 100644 --- a/files/admin.py +++ b/files/admin.py @@ -55,7 +55,7 @@ class FileAdmin(admin.ModelAdmin): return render_to_string('files/admin/thumbnails.html', context) except Exception: # Make sure any exception happening here is always logged - # (e.g. admin eats exception in ModelAdmin properties, making it hard to debug) + # (e.g. admin eats exceptions in ModelAdmin properties, making it hard to debug) logger.exception('Failed to render thumbnails') raise -- 2.30.2 From 82adaae8daf3122bcd8125af009dc1edcc110cbb Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Tue, 23 Apr 2024 17:03:08 +0200 Subject: [PATCH 05/23] Clean up --- constants/base.py | 1 - files/admin.py | 17 ++-- files/signals.py | 2 +- files/tasks.py | 25 ++---- files/templates/files/admin/thumbnails.html | 2 +- files/utils.py | 89 ++++++++++----------- 6 files changed, 62 insertions(+), 74 deletions(-) diff --git a/constants/base.py b/constants/base.py index af06d22b..3edb2acf 100644 --- a/constants/base.py +++ b/constants/base.py @@ -107,6 +107,5 @@ ABUSE_TYPE = Choices( THUMBNAIL_SIZE_L = (1920, 1080) THUMBNAIL_SIZE_S = (640, 360) THUMBNAIL_SIZES = [THUMBNAIL_SIZE_L, THUMBNAIL_SIZE_S] -THUMBNAIL_META = {'l': THUMBNAIL_SIZE_L, 's': THUMBNAIL_SIZE_S} THUMBNAIL_FORMAT = 'PNG' THUMBNAIL_QUALITY = 83 diff --git a/files/admin.py b/files/admin.py index 66206ac6..91049166 100644 --- a/files/admin.py +++ b/files/admin.py @@ -19,6 +19,12 @@ def scan_selected_files(self, request, queryset): files.signals.schedule_scan(instance) +def schedule_thumbnails(self, request, queryset): + """Schedule thumbnails generationg for selected files.""" + for instance in queryset.filter(type__in=(File.TYPES.IMAGE, File.TYPES.VIDEO)): + files.tasks.make_thumbnails(file_id=instance.pk) + + def make_thumbnails(self, request, queryset): """Make thumbnails for selected files.""" for instance in queryset.filter(type__in=(File.TYPES.IMAGE, File.TYPES.VIDEO)): @@ -60,11 +66,10 @@ class FileAdmin(admin.ModelAdmin): raise def get_form(self, request, obj=None, **kwargs): - # Only override if the obj exisits - if obj: - if obj.is_image or obj.is_video: - help_text = 'Additional information about the file, e.g. existing thumbnails.' - kwargs.update({'help_texts': {'metadata': help_text}}) + """Override metadata help text for depending on type.""" + if obj and (obj.is_image or obj.is_video): + help_text = 'Additional information about the file, e.g. existing thumbnails.' + kwargs.update({'help_texts': {'metadata': help_text}}) return super().get_form(request, obj, **kwargs) view_on_site = False @@ -141,7 +146,7 @@ class FileAdmin(admin.ModelAdmin): ) inlines = [FileValidationInlineAdmin] - actions = [scan_selected_files, make_thumbnails] + actions = [scan_selected_files, schedule_thumbnails, make_thumbnails] def is_ok(self, obj): return obj.validation.is_ok if hasattr(obj, 'validation') else None diff --git a/files/signals.py b/files/signals.py index 907e9f59..6846b613 100644 --- a/files/signals.py +++ b/files/signals.py @@ -39,7 +39,7 @@ def _scan_new_file( def schedule_thumbnails(file: files.models.File) -> None: """Schedule thumbnail generation for a given file.""" args = {'pk': file.pk, 'type': file.get_type_display()} - logger.info('Scheduling thumbnail generation for file pk=%s type=%s', args) + logger.info('Scheduling thumbnail generation for file pk=%(pk)s type=%(type)s', args) verbose_name = f'make thumbnails for "{file.source.name}"' files.tasks.make_thumbnails(file_id=file.pk, creator=file, verbose_name=verbose_name) diff --git a/files/tasks.py b/files/tasks.py index d798334d..b0a18872 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -5,7 +5,7 @@ from background_task import background from background_task.tasks import TaskSchedule from django.conf import settings -from constants.base import THUMBNAIL_FORMAT, THUMBNAIL_SIZE_L, THUMBNAIL_QUALITY, THUMBNAIL_META +from constants.base import THUMBNAIL_SIZE_L import files.models import files.utils @@ -43,33 +43,22 @@ def make_thumbnails(file_id: int) -> None: source_path = os.path.join(settings.MEDIA_ROOT, file.source.path) thumbnail_field = file.thumbnail - thumbnails_paths = file.thumbnails - thumbnail_default_path = thumbnails_paths[THUMBNAIL_SIZE_L] - # For a video, source of the thumbnail is some frame fetched with ffpeg if file.is_video: - output_path = os.path.join(settings.MEDIA_ROOT, thumbnail_default_path) + # For a video, source of the thumbnail is some frame fetched with ffpeg + output_path = os.path.join(settings.MEDIA_ROOT, thumbnail_field.upload_to(file, 'x.png')) files.utils.extract_frame(source_path, output_path) source_path = output_path - files.utils.make_thumbnails( - source_path, - thumbnails_paths, - output_format=THUMBNAIL_FORMAT, - quality=THUMBNAIL_QUALITY, - optimize=True, - progressive=True, - ) + thumbnails = files.utils.make_thumbnails(source_path, file.hash) + thumbnail_default_path = next(_['path'] for _ in thumbnails if _['size'] == THUMBNAIL_SIZE_L) update_fields = set() if thumbnail_default_path != thumbnail_field.name: thumbnail_field.name = thumbnail_default_path update_fields.add('thumbnail') - thumbnail_metadata = { - key: {'size': size, 'path': thumbnails_paths[size]} for key, size in THUMBNAIL_META.items() - } - if file.metadata.get('thumbnails') != thumbnail_metadata: - file.metadata.update({'thumbnails': thumbnail_metadata}) + if file.metadata.get('thumbnails') != thumbnails: + file.metadata.update({'thumbnails': thumbnails}) update_fields.add('metadata') if update_fields: file.save(update_fields=update_fields) diff --git a/files/templates/files/admin/thumbnails.html b/files/templates/files/admin/thumbnails.html index e377e6e2..39d4ed94 100644 --- a/files/templates/files/admin/thumbnails.html +++ b/files/templates/files/admin/thumbnails.html @@ -1,5 +1,5 @@
-{% for key, thumb in file.metadata.thumbnails.items %} +{% for thumb in file.metadata.thumbnails %}
{{ thumb.size.0 }}x{{ thumb.size.1 }}px diff --git a/files/utils.py b/files/utils.py index e7bd85bb..c6e7c86e 100644 --- a/files/utils.py +++ b/files/utils.py @@ -17,7 +17,7 @@ from lxml import etree import clamd import magic -from constants.base import THUMBNAIL_FORMAT, THUMBNAIL_SIZES +from constants.base import THUMBNAIL_FORMAT, THUMBNAIL_SIZES, THUMBNAIL_QUALITY logger = logging.getLogger(__name__) MODULE_DIR = Path(__file__).resolve().parent @@ -195,7 +195,7 @@ def delete_file_in_storage(file_name: str) -> None: def delete_thumbnails(file_metadata: dict) -> None: """Read thumbnail paths from given metadata and delete them from storage.""" thumbnails = file_metadata.get('thumbnails', {}) - for _, thumb in thumbnails.items: + for _, thumb in thumbnails.items(): path = thumb.get('path', '') if not path: continue @@ -208,47 +208,6 @@ def get_base_path(file_path: str) -> str: return base_path -def _resize(image: Image, size: tuple, output, output_format: str = 'PNG', **output_params): - """Resize a models.ImageField to a given size and write it into output file.""" - source_image = image.convert('RGBA' if output_format == 'PNG' else 'RGB') - source_image.thumbnail(size, Image.LANCZOS) - source_image.save(output, output_format, **output_params) - - -def make_thumbnails(image_field, output_paths: dict, output_format: str = 'PNG', **output_params): - """Generate thumbnail files for given models.ImageField and list of dimensions. - - Currently only intended to be used manually from shell, e.g.: - - from files.models import File - import files.utils - files_wo_thumbs = File.objects.filter(type__in=(File.TYPES.IMAGE, File.TYPES.VIDEO)) - for b in badges: - # TODO: thumbnail should be set to THUMBNAIL_SIZE_L of the source for IMAGE - base_path = files.utils.get_base_path(b.thumbnail.path) - files.utils.make_thumbnails(b.thumbnail, base_path, THUMBNAIL_SIZES) - - """ - thumbnail_ext = output_format.lower() - if thumbnail_ext == 'jpeg': - thumbnail_ext = 'jpg' - thumbnails = {} - for (w, h), output_path in output_paths.items(): - # output_path = f'{base_path}_{w}x{h}.{thumbnail_ext}' - with tempfile.TemporaryFile() as f: - size = (w, h) - logger.info('Resizing %s to %s (%s)', image_field, size, output_format) - image = Image.open(image_field) - _resize(image, size, f, output_format=output_format, **output_params) - logger.info('Saving a new thumbnail to %s', output_path) - if default_storage.exists(output_path): - logger.warning('%s exists, overwriting', output_path) - default_storage.delete(output_path) - default_storage.save(output_path, f) - thumbnails[size] = output_path - return thumbnails - - def get_thumbnail_upload_to(file_hash: str): prefix = 'thumbnails/' _hash = file_hash.split(':')[-1] @@ -257,11 +216,47 @@ def get_thumbnail_upload_to(file_hash: str): return path -def get_thumbnails_paths(file_hash: str) -> dict: - """Return a dictionary of paths, derived from hash and predefined thumbnails dimensions.""" +def _resize(image: Image, size: tuple, output, output_format: str = 'PNG', **output_params): + """Resize a models.ImageField to a given size and write it into output file.""" + source_image = image.convert('RGBA' if output_format == 'PNG' else 'RGB') + source_image.thumbnail(size, Image.LANCZOS) + source_image.save(output, output_format, **output_params) + + +def make_thumbnails(file_path: str, file_hash: str, output_format: str = THUMBNAIL_FORMAT) -> list: + """Generate thumbnail files for given file and a predefined list of dimensions. + + Resulting thumbnail paths a derived from the given file hash and thumbnail sizes. + Return a list of sizes and output paths of generated thumbnail images. + """ + thumbnail_ext = output_format.lower() + if thumbnail_ext == 'jpeg': + thumbnail_ext = 'jpg' base_path = get_base_path(get_thumbnail_upload_to(file_hash)) - extension = f'.{THUMBNAIL_FORMAT.lower()}' - return {size: f'{base_path}_{size[0]}x{size[1]}{extension}' for size in THUMBNAIL_SIZES} + thumbnails = [] + for w, h in THUMBNAIL_SIZES: + output_path = f'{base_path}_{w}x{h}.{thumbnail_ext}' + size = (w, h) + with tempfile.TemporaryFile() as f: + logger.info('Resizing %s to %s (%s)', file_path, size, output_format) + image = Image.open(file_path) + _resize( + image, + size, + f, + output_format=THUMBNAIL_FORMAT, + quality=THUMBNAIL_QUALITY, + optimize=True, + progressive=True, + ) + logger.info('Saving a new thumbnail to %s', output_path) + # Overwrite files instead of allowing storage generate a deduplicating suffix + if default_storage.exists(output_path): + logger.warning('%s exists, overwriting', output_path) + default_storage.delete(output_path) + default_storage.save(output_path, f) + thumbnails.append({'size': size, 'path': output_path}) + return thumbnails def extract_frame(source_path: str, output_path: str): -- 2.30.2 From 33587ba7205af8ef8feb282a06ee01ff2f7c19a7 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Tue, 23 Apr 2024 17:34:48 +0200 Subject: [PATCH 06/23] More clean up --- constants/base.py | 2 +- files/models.py | 19 ++++++++++++++----- files/tasks.py | 5 ++--- files/templates/files/admin/thumbnails.html | 4 +++- files/utils.py | 8 ++++---- 5 files changed, 24 insertions(+), 14 deletions(-) diff --git a/constants/base.py b/constants/base.py index 3edb2acf..e3f8cd67 100644 --- a/constants/base.py +++ b/constants/base.py @@ -106,6 +106,6 @@ ABUSE_TYPE = Choices( # the code expecting thumbnails of new dimensions can be deployed! THUMBNAIL_SIZE_L = (1920, 1080) THUMBNAIL_SIZE_S = (640, 360) -THUMBNAIL_SIZES = [THUMBNAIL_SIZE_L, THUMBNAIL_SIZE_S] +THUMBNAIL_SIZES = {'l': THUMBNAIL_SIZE_L, 's': THUMBNAIL_SIZE_S} THUMBNAIL_FORMAT = 'PNG' THUMBNAIL_QUALITY = 83 diff --git a/files/models.py b/files/models.py index 3d8e7a76..11b6a310 100644 --- a/files/models.py +++ b/files/models.py @@ -199,7 +199,11 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): @property def thumbnail_l_url(self) -> str: - """Log absence of the thumbnail file instead of exploding somewhere in the templates.""" + """Return absolute path portion of the URL of the large thumbnail of this file. + + Fall back to the source file, if no thumbnail is stored. + Log absence of the thumbnail file instead of exploding somewhere in the templates. + """ try: return self.thumbnail.url except ValueError: @@ -208,11 +212,16 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): @property def thumbnail_s_url(self) -> str: - """Log absence of the thumbnail file instead of exploding somewhere in the templates.""" + """Return absolute path portion of the URL of the small thumbnail of this file. + + Fall back to the source file, if no thumbnail is stored. + Log absence of the thumbnail file instead of exploding somewhere in the templates. + """ try: - return self.metadata['thumbnails']['s']['path'] - except KeyError: - log.exception(f'File pk={self.pk} is missing a small thumbnail') + s = self.metadata['thumbnails']['s']['path'] + return self.thumbnail.storage.url(s) + except (KeyError, TypeError): + log.exception(f'File pk={self.pk} is missing a small thumbnail: {self.metadata}') return self.source.url diff --git a/files/tasks.py b/files/tasks.py index b0a18872..12321125 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -5,7 +5,6 @@ from background_task import background from background_task.tasks import TaskSchedule from django.conf import settings -from constants.base import THUMBNAIL_SIZE_L import files.models import files.utils @@ -32,7 +31,7 @@ def clamdscan(file_id: int): @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) def make_thumbnails(file_id: int) -> None: - """Generate thumbnails for a given file.""" + """Generate thumbnails for a given file, store them in thumbnail and metadata columns.""" file = files.models.File.objects.get(pk=file_id) args = {'pk': file_id, 'type': file.get_type_display()} if not file.is_image and not file.is_video: @@ -51,7 +50,7 @@ def make_thumbnails(file_id: int) -> None: source_path = output_path thumbnails = files.utils.make_thumbnails(source_path, file.hash) - thumbnail_default_path = next(_['path'] for _ in thumbnails if _['size'] == THUMBNAIL_SIZE_L) + thumbnail_default_path = thumbnails['l']['path'] update_fields = set() if thumbnail_default_path != thumbnail_field.name: diff --git a/files/templates/files/admin/thumbnails.html b/files/templates/files/admin/thumbnails.html index 39d4ed94..1e3c2242 100644 --- a/files/templates/files/admin/thumbnails.html +++ b/files/templates/files/admin/thumbnails.html @@ -1,10 +1,12 @@
-{% for thumb in file.metadata.thumbnails %} +{% for size_key, thumb in file.metadata.thumbnails.items %}
{{ thumb.size.0 }}x{{ thumb.size.1 }}px
{% endfor %} +{# alert about unexpected size of the default thumbnail, which might happen if source file was smaller than 1080p #} +{# TODO: do we want to make minimum resolusion a validation criteria during upload instead? #} {% if file.thumbnail.width != THUMBNAIL_SIZE_L.0 or file.thumbnail.height != THUMBNAIL_SIZE_L.1 %}

⚠ Expected {{ THUMBNAIL_SIZE_L.0 }}x{{ THUMBNAIL_SIZE_L.1 }}px, diff --git a/files/utils.py b/files/utils.py index c6e7c86e..f6a56159 100644 --- a/files/utils.py +++ b/files/utils.py @@ -227,14 +227,14 @@ def make_thumbnails(file_path: str, file_hash: str, output_format: str = THUMBNA """Generate thumbnail files for given file and a predefined list of dimensions. Resulting thumbnail paths a derived from the given file hash and thumbnail sizes. - Return a list of sizes and output paths of generated thumbnail images. + Return a dict of size keys to output paths of generated thumbnail images. """ thumbnail_ext = output_format.lower() if thumbnail_ext == 'jpeg': thumbnail_ext = 'jpg' base_path = get_base_path(get_thumbnail_upload_to(file_hash)) - thumbnails = [] - for w, h in THUMBNAIL_SIZES: + thumbnails = {} + for size_key, (w, h) in THUMBNAIL_SIZES.items(): output_path = f'{base_path}_{w}x{h}.{thumbnail_ext}' size = (w, h) with tempfile.TemporaryFile() as f: @@ -255,7 +255,7 @@ def make_thumbnails(file_path: str, file_hash: str, output_format: str = THUMBNA logger.warning('%s exists, overwriting', output_path) default_storage.delete(output_path) default_storage.save(output_path, f) - thumbnails.append({'size': size, 'path': output_path}) + thumbnails[size_key] = {'size': size, 'path': output_path} return thumbnails -- 2.30.2 From 2c3d157cb80673f527ae398150718516006635ab Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Tue, 23 Apr 2024 17:36:02 +0200 Subject: [PATCH 07/23] Typo --- files/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/files/admin.py b/files/admin.py index 91049166..b77e3e2f 100644 --- a/files/admin.py +++ b/files/admin.py @@ -66,7 +66,7 @@ class FileAdmin(admin.ModelAdmin): raise def get_form(self, request, obj=None, **kwargs): - """Override metadata help text for depending on type.""" + """Override metadata help text depending on file type.""" if obj and (obj.is_image or obj.is_video): help_text = 'Additional information about the file, e.g. existing thumbnails.' kwargs.update({'help_texts': {'metadata': help_text}}) -- 2.30.2 From 7f3ff2db613a5a4ad4f3400534ca4538a7dbe5c2 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Tue, 23 Apr 2024 18:15:33 +0200 Subject: [PATCH 08/23] Minor changes --- files/admin.py | 10 ++-------- files/signals.py | 7 +++++-- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/files/admin.py b/files/admin.py index b77e3e2f..15f686e5 100644 --- a/files/admin.py +++ b/files/admin.py @@ -19,18 +19,12 @@ def scan_selected_files(self, request, queryset): files.signals.schedule_scan(instance) -def schedule_thumbnails(self, request, queryset): +def make_thumbnails(self, request, queryset): """Schedule thumbnails generationg for selected files.""" for instance in queryset.filter(type__in=(File.TYPES.IMAGE, File.TYPES.VIDEO)): files.tasks.make_thumbnails(file_id=instance.pk) -def make_thumbnails(self, request, queryset): - """Make thumbnails for selected files.""" - for instance in queryset.filter(type__in=(File.TYPES.IMAGE, File.TYPES.VIDEO)): - files.tasks.make_thumbnails.task_function(file_id=instance.pk) - - class FileValidationInlineAdmin(admin.StackedInline): model = FileValidation readonly_fields = ('date_created', 'date_modified', 'is_ok', 'results') @@ -146,7 +140,7 @@ class FileAdmin(admin.ModelAdmin): ) inlines = [FileValidationInlineAdmin] - actions = [scan_selected_files, schedule_thumbnails, make_thumbnails] + actions = [scan_selected_files, make_thumbnails] def is_ok(self, obj): return obj.validation.is_ok if hasattr(obj, 'validation') else None diff --git a/files/signals.py b/files/signals.py index 6846b613..1a244d8f 100644 --- a/files/signals.py +++ b/files/signals.py @@ -51,9 +51,12 @@ def _schedule_thumbnails( if not created: return + if not instance.is_ok: + return + file = instance.file - if instance.is_ok and (file.is_image or file.is_video): - # Generate thumbnails if initial scan found no issues + # Generate thumbnails if initial scan found no issues + if file.is_image or file.is_video: schedule_thumbnails(file) -- 2.30.2 From f1da81b250f39aaaa65b3edc9e8caab0a125c2b4 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 11:15:17 +0200 Subject: [PATCH 09/23] Typos --- files/admin.py | 2 +- files/templates/files/admin/thumbnails.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/files/admin.py b/files/admin.py index 15f686e5..30c43687 100644 --- a/files/admin.py +++ b/files/admin.py @@ -20,7 +20,7 @@ def scan_selected_files(self, request, queryset): def make_thumbnails(self, request, queryset): - """Schedule thumbnails generationg for selected files.""" + """Schedule thumbnails generation for selected files.""" for instance in queryset.filter(type__in=(File.TYPES.IMAGE, File.TYPES.VIDEO)): files.tasks.make_thumbnails(file_id=instance.pk) diff --git a/files/templates/files/admin/thumbnails.html b/files/templates/files/admin/thumbnails.html index 1e3c2242..9def9f30 100644 --- a/files/templates/files/admin/thumbnails.html +++ b/files/templates/files/admin/thumbnails.html @@ -6,7 +6,7 @@

{% endfor %} {# alert about unexpected size of the default thumbnail, which might happen if source file was smaller than 1080p #} -{# TODO: do we want to make minimum resolusion a validation criteria during upload instead? #} +{# TODO: do we want to make minimum resolution a validation criteria during upload instead? #} {% if file.thumbnail.width != THUMBNAIL_SIZE_L.0 or file.thumbnail.height != THUMBNAIL_SIZE_L.1 %}

⚠ Expected {{ THUMBNAIL_SIZE_L.0 }}x{{ THUMBNAIL_SIZE_L.1 }}px, -- 2.30.2 From 728fb4a25c0795f009b2b015fd52fbc68070d02c Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 12:01:28 +0200 Subject: [PATCH 10/23] Minor changes --- files/tasks.py | 8 ++++---- files/tests/test_utils.py | 16 +++++++++++++++- files/utils.py | 37 ++++++++++++++++--------------------- 3 files changed, 35 insertions(+), 26 deletions(-) diff --git a/files/tasks.py b/files/tasks.py index 12321125..f305546f 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -41,18 +41,18 @@ def make_thumbnails(file_id: int) -> None: assert file.validation.is_ok, f'File pk={file_id} is flagged' source_path = os.path.join(settings.MEDIA_ROOT, file.source.path) - thumbnail_field = file.thumbnail if file.is_video: + f_path = os.path.join(settings.MEDIA_ROOT, files.utils.get_thumbnail_upload_to(file.hash)) # For a video, source of the thumbnail is some frame fetched with ffpeg - output_path = os.path.join(settings.MEDIA_ROOT, thumbnail_field.upload_to(file, 'x.png')) - files.utils.extract_frame(source_path, output_path) - source_path = output_path + files.utils.extract_frame(source_path, f_path) + source_path = f_path thumbnails = files.utils.make_thumbnails(source_path, file.hash) thumbnail_default_path = thumbnails['l']['path'] update_fields = set() + thumbnail_field = file.thumbnail if thumbnail_default_path != thumbnail_field.name: thumbnail_field.name = thumbnail_default_path update_fields.add('thumbnail') diff --git a/files/tests/test_utils.py b/files/tests/test_utils.py index 1a581841..e4fde404 100644 --- a/files/tests/test_utils.py +++ b/files/tests/test_utils.py @@ -1,6 +1,11 @@ from django.test import TestCase -from files.utils import find_path_by_name, find_exact_path, filter_paths_by_ext +from files.utils import ( + filter_paths_by_ext, + find_exact_path, + find_path_by_name, + get_thumbnail_upload_to, +) class UtilsTest(TestCase): @@ -98,3 +103,12 @@ class UtilsTest(TestCase): ] paths = filter_paths_by_ext(name_list, '.md') self.assertEqual(list(paths), []) + + def test_get_thumbnail_upload_to(self): + for file_hash, kwargs, expected in ( + ('foobar', {}, 'thumbnails/fo/foobar.png'), + ('deadbeaf', {'width': None, 'height': None}, 'thumbnails/de/deadbeaf.png'), + ('deadbeaf', {'width': 640, 'height': 360}, 'thumbnails/de/deadbeaf_640x360.png'), + ): + with self.subTest(file_hash=file_hash, kwargs=kwargs): + self.assertEqual(get_thumbnail_upload_to(file_hash, **kwargs), expected) diff --git a/files/utils.py b/files/utils.py index f6a56159..687c8d73 100644 --- a/files/utils.py +++ b/files/utils.py @@ -202,21 +202,23 @@ def delete_thumbnails(file_metadata: dict) -> None: delete_file_in_storage(path) -def get_base_path(file_path: str) -> str: - """Return the given path without its extension.""" - base_path, _ = os.path.splitext(file_path) - return base_path +def get_thumbnail_upload_to(file_hash: str, width: int = None, height: int = None) -> str: + """Return a full media path of a thumbnail. - -def get_thumbnail_upload_to(file_hash: str): + Optionally, append thumbnail dimensions to the file name. + """ prefix = 'thumbnails/' _hash = file_hash.split(':')[-1] - extension = f'.{THUMBNAIL_FORMAT.lower()}' - path = Path(prefix, _hash[:2], _hash).with_suffix(extension) - return path + thumbnail_ext = THUMBNAIL_FORMAT.lower() + if thumbnail_ext == 'jpeg': + thumbnail_ext = 'jpg' + suffix = f'.{thumbnail_ext}' + size_suffix = f'_{width}x{height}' if width and height else '' + path = Path(prefix, _hash[:2], f'{_hash}{size_suffix}').with_suffix(suffix) + return str(path) -def _resize(image: Image, size: tuple, output, output_format: str = 'PNG', **output_params): +def resize_image(image: Image, size: tuple, output, output_format: str = 'PNG', **output_params): """Resize a models.ImageField to a given size and write it into output file.""" source_image = image.convert('RGBA' if output_format == 'PNG' else 'RGB') source_image.thumbnail(size, Image.LANCZOS) @@ -229,18 +231,14 @@ def make_thumbnails(file_path: str, file_hash: str, output_format: str = THUMBNA Resulting thumbnail paths a derived from the given file hash and thumbnail sizes. Return a dict of size keys to output paths of generated thumbnail images. """ - thumbnail_ext = output_format.lower() - if thumbnail_ext == 'jpeg': - thumbnail_ext = 'jpg' - base_path = get_base_path(get_thumbnail_upload_to(file_hash)) thumbnails = {} for size_key, (w, h) in THUMBNAIL_SIZES.items(): - output_path = f'{base_path}_{w}x{h}.{thumbnail_ext}' + output_path = get_thumbnail_upload_to(file_hash, width=w, height=h) size = (w, h) with tempfile.TemporaryFile() as f: logger.info('Resizing %s to %s (%s)', file_path, size, output_format) image = Image.open(file_path) - _resize( + resize_image( image, size, f, @@ -259,17 +257,14 @@ def make_thumbnails(file_path: str, file_hash: str, output_format: str = THUMBNA return thumbnails -def extract_frame(source_path: str, output_path: str): +def extract_frame(source_path: str, output_path: str, at_time: str = '00:00:00.01'): """Extract a single frame of a video at a given path, write it to the given output path.""" try: ffmpeg = ( FFmpeg() .option('y') .input(source_path) - .output( - output_path, - {'ss': '00:00:00.01', 'frames:v': 1, 'update': 'true'}, - ) + .output(output_path, {'ss': at_time, 'frames:v': 1, 'update': 'true'}) ) output_dir = os.path.dirname(output_path) if not os.path.isdir(output_dir): -- 2.30.2 From db3d784cbfb4fcfde08af2b0c39fbc5f68eb834e Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 12:29:34 +0200 Subject: [PATCH 11/23] Tests --- files/tests/test_utils.py | 30 ++++++++++++++++++++++++++++++ files/utils.py | 2 +- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/files/tests/test_utils.py b/files/tests/test_utils.py index e4fde404..5ed4cf2a 100644 --- a/files/tests/test_utils.py +++ b/files/tests/test_utils.py @@ -1,3 +1,6 @@ +from unittest.mock import patch, ANY +from pathlib import Path + from django.test import TestCase from files.utils import ( @@ -5,8 +8,12 @@ from files.utils import ( find_exact_path, find_path_by_name, get_thumbnail_upload_to, + make_thumbnails, ) +# Reusing test files from the extensions app +TEST_FILES_DIR = Path(__file__).resolve().parent.parent.parent / 'extensions' / 'tests' / 'files' + class UtilsTest(TestCase): manifest = 'blender_manifest.toml' @@ -112,3 +119,26 @@ class UtilsTest(TestCase): ): with self.subTest(file_hash=file_hash, kwargs=kwargs): self.assertEqual(get_thumbnail_upload_to(file_hash, **kwargs), expected) + + @patch('files.utils.resize_image') + def test_make_thumbnails(self, mock_resize_image): + self.assertEqual( + { + 'l': {'path': 'thumbnails/fo/foobar_1920x1080.png', 'size': (1920, 1080)}, + 's': {'path': 'thumbnails/fo/foobar_640x360.png', 'size': (640, 360)}, + }, + make_thumbnails(TEST_FILES_DIR / 'test_preview_image_0001.png', 'foobar'), + ) + + self.assertEqual(len(mock_resize_image.mock_calls), 2) + for expected_size in ((1920, 1080), (640, 360)): + with self.subTest(expected_size=expected_size): + mock_resize_image.assert_any_call( + ANY, + expected_size, + ANY, + output_format='PNG', + quality=83, + optimize=True, + progressive=True, + ) diff --git a/files/utils.py b/files/utils.py index 687c8d73..816ed5d2 100644 --- a/files/utils.py +++ b/files/utils.py @@ -225,7 +225,7 @@ def resize_image(image: Image, size: tuple, output, output_format: str = 'PNG', source_image.save(output, output_format, **output_params) -def make_thumbnails(file_path: str, file_hash: str, output_format: str = THUMBNAIL_FORMAT) -> list: +def make_thumbnails(file_path: str, file_hash: str, output_format: str = THUMBNAIL_FORMAT) -> dict: """Generate thumbnail files for given file and a predefined list of dimensions. Resulting thumbnail paths a derived from the given file hash and thumbnail sizes. -- 2.30.2 From 4ff66ae241bfda8e495a5d8637ab9aa5c2e9aca4 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 13:24:29 +0200 Subject: [PATCH 12/23] Open image once, close it after thumbnails are done --- files/tests/test_utils.py | 4 ++-- files/utils.py | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/files/tests/test_utils.py b/files/tests/test_utils.py index 5ed4cf2a..0baa0500 100644 --- a/files/tests/test_utils.py +++ b/files/tests/test_utils.py @@ -114,8 +114,8 @@ class UtilsTest(TestCase): def test_get_thumbnail_upload_to(self): for file_hash, kwargs, expected in ( ('foobar', {}, 'thumbnails/fo/foobar.png'), - ('deadbeaf', {'width': None, 'height': None}, 'thumbnails/de/deadbeaf.png'), - ('deadbeaf', {'width': 640, 'height': 360}, 'thumbnails/de/deadbeaf_640x360.png'), + ('deadbeef', {'width': None, 'height': None}, 'thumbnails/de/deadbeef.png'), + ('deadbeef', {'width': 640, 'height': 360}, 'thumbnails/de/deadbeef_640x360.png'), ): with self.subTest(file_hash=file_hash, kwargs=kwargs): self.assertEqual(get_thumbnail_upload_to(file_hash, **kwargs), expected) diff --git a/files/utils.py b/files/utils.py index 816ed5d2..537f8c24 100644 --- a/files/utils.py +++ b/files/utils.py @@ -232,12 +232,12 @@ def make_thumbnails(file_path: str, file_hash: str, output_format: str = THUMBNA Return a dict of size keys to output paths of generated thumbnail images. """ thumbnails = {} + image = Image.open(file_path) for size_key, (w, h) in THUMBNAIL_SIZES.items(): output_path = get_thumbnail_upload_to(file_hash, width=w, height=h) size = (w, h) with tempfile.TemporaryFile() as f: logger.info('Resizing %s to %s (%s)', file_path, size, output_format) - image = Image.open(file_path) resize_image( image, size, @@ -254,6 +254,7 @@ def make_thumbnails(file_path: str, file_hash: str, output_format: str = THUMBNA default_storage.delete(output_path) default_storage.save(output_path, f) thumbnails[size_key] = {'size': size, 'path': output_path} + image.close() return thumbnails -- 2.30.2 From 3f824d8b0acaae00b6eb83d149f0833c34eec6b2 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 13:38:39 +0200 Subject: [PATCH 13/23] Tests --- files/tests/test_tasks.py | 66 +++++++++++++++++++++++++++++++++++++++ files/tests/test_utils.py | 18 ++++++++++- 2 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 files/tests/test_tasks.py diff --git a/files/tests/test_tasks.py b/files/tests/test_tasks.py new file mode 100644 index 00000000..c05a044d --- /dev/null +++ b/files/tests/test_tasks.py @@ -0,0 +1,66 @@ +from unittest.mock import patch +from pathlib import Path + +from django.test import TestCase, override_settings + +from common.tests.factories.files import FileFactory +from files.tasks import make_thumbnails +import files.models + +TEST_MEDIA_DIR = Path(__file__).resolve().parent / 'media' + + +# Media file are physically deleted when files records are deleted, hence the override +@override_settings(MEDIA_ROOT=TEST_MEDIA_DIR) +class TasksTest(TestCase): + @patch('files.utils.resize_image') + @patch('files.utils.Image') + def test_make_thumbnails_for_image(self, mock_image, mock_resize_image): + file = FileFactory(original_hash='foobar', source='file/original_image_source.jpg') + files.models.FileValidation.objects.create(file=file, is_ok=True, results={}) + + make_thumbnails.task_function(file_id=file.pk) + + mock_image.open.assert_called_once_with( + str(TEST_MEDIA_DIR / 'file' / 'original_image_source.jpg') + ) + mock_image.open.return_value.close.assert_called_once() + file.refresh_from_db() + self.assertEqual( + file.metadata, + { + 'thumbnails': { + 'l': {'path': 'thumbnails/fo/foobar_1920x1080.png', 'size': [1920, 1080]}, + 's': {'path': 'thumbnails/fo/foobar_640x360.png', 'size': [640, 360]}, + }, + }, + ) + self.assertEqual(file.thumbnail.name, 'thumbnails/fo/foobar_1920x1080.png') + + @patch('files.utils.resize_image') + @patch('files.utils.Image') + @patch('files.utils.FFmpeg') + def test_make_thumbnails_for_video(self, mock_ffmpeg, mock_image, mock_resize_image): + file = FileFactory( + original_hash='deadbeef', source='file/path.mp4', type=files.models.File.TYPES.VIDEO + ) + files.models.FileValidation.objects.create(file=file, is_ok=True, results={}) + + make_thumbnails.task_function(file_id=file.pk) + + mock_ffmpeg.assert_called_once_with() + mock_image.open.assert_called_once_with( + str(TEST_MEDIA_DIR / 'thumbnails' / 'de' / 'deadbeef.png') + ) + mock_image.open.return_value.close.assert_called_once() + file.refresh_from_db() + self.assertEqual( + file.metadata, + { + 'thumbnails': { + 'l': {'path': 'thumbnails/de/deadbeef_1920x1080.png', 'size': [1920, 1080]}, + 's': {'path': 'thumbnails/de/deadbeef_640x360.png', 'size': [640, 360]}, + }, + }, + ) + self.assertEqual(file.thumbnail.name, 'thumbnails/de/deadbeef_1920x1080.png') diff --git a/files/tests/test_utils.py b/files/tests/test_utils.py index 0baa0500..6dd2a6c2 100644 --- a/files/tests/test_utils.py +++ b/files/tests/test_utils.py @@ -1,9 +1,11 @@ -from unittest.mock import patch, ANY from pathlib import Path +from unittest.mock import patch, ANY +import tempfile from django.test import TestCase from files.utils import ( + extract_frame, filter_paths_by_ext, find_exact_path, find_path_by_name, @@ -142,3 +144,17 @@ class UtilsTest(TestCase): optimize=True, progressive=True, ) + + @patch('files.utils.FFmpeg') + def test_extract_frame(self, mock_ffmpeg): + with tempfile.TemporaryDirectory() as output_dir: + extract_frame('path/to/source/video.mp4', output_dir + '/frame.png') + mock_ffmpeg.return_value.option.return_value.input.return_value.output.assert_any_call( + output_dir + '/frame.png', {'ss': '00:00:00.01', 'frames:v': 1, 'update': 'true'} + ) + + self.assertEqual(len(mock_ffmpeg.mock_calls), 5) + mock_ffmpeg.assert_any_call() + mock_ffmpeg.return_value.option.return_value.input.assert_any_call( + 'path/to/source/video.mp4' + ) -- 2.30.2 From 8d156244c59aebd2f20da7c4931975fd0bc5d200 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 14:29:25 +0200 Subject: [PATCH 14/23] Break if suddendly asked to make thumbnails for other file types --- files/tasks.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/files/tasks.py b/files/tasks.py index f305546f..cd10b924 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -39,12 +39,14 @@ def make_thumbnails(file_id: int) -> None: return assert file.validation.is_ok, f'File pk={file_id} is flagged' + assert file.is_image or file.is_video, f'File pk={file_id} is neither image nor video' + # For an image, source of the thumbnails is the original image source_path = os.path.join(settings.MEDIA_ROOT, file.source.path) if file.is_video: f_path = os.path.join(settings.MEDIA_ROOT, files.utils.get_thumbnail_upload_to(file.hash)) - # For a video, source of the thumbnail is some frame fetched with ffpeg + # For a video, source of the thumbnails is a frame extracted with ffpeg files.utils.extract_frame(source_path, f_path) source_path = f_path -- 2.30.2 From 4fb859e687dc00ceb6b4c433ac8d00a841e4c0e2 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 14:30:07 +0200 Subject: [PATCH 15/23] Minor change --- files/tests/test_tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/files/tests/test_tasks.py b/files/tests/test_tasks.py index c05a044d..530f33c7 100644 --- a/files/tests/test_tasks.py +++ b/files/tests/test_tasks.py @@ -10,7 +10,6 @@ import files.models TEST_MEDIA_DIR = Path(__file__).resolve().parent / 'media' -# Media file are physically deleted when files records are deleted, hence the override @override_settings(MEDIA_ROOT=TEST_MEDIA_DIR) class TasksTest(TestCase): @patch('files.utils.resize_image') -- 2.30.2 From e44ada439c50579f2fd5a0ee6c22264e3f357e54 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 15:38:51 +0200 Subject: [PATCH 16/23] Utils should take of prepending MEDIA_ROOT; save video frame as File.thumbnail --- files/models.py | 30 +++++++++++++----------------- files/tasks.py | 19 +++++++++++-------- files/tests/test_tasks.py | 32 ++++++++++++++++++++++++++++++-- files/utils.py | 15 ++++++++++----- 4 files changed, 64 insertions(+), 32 deletions(-) diff --git a/files/models.py b/files/models.py index 11b6a310..72cc0ccb 100644 --- a/files/models.py +++ b/files/models.py @@ -197,32 +197,28 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): def get_submit_url(self) -> str: return self.extension.get_draft_url() - @property - def thumbnail_l_url(self) -> str: - """Return absolute path portion of the URL of the large thumbnail of this file. + def get_thumbnail_of_size(self, size_key: str) -> str: + """Return absolute path portion of the URL of a thumbnail of this file. Fall back to the source file, if no thumbnail is stored. Log absence of the thumbnail file instead of exploding somewhere in the templates. """ + if not self.is_image and not self.is_video: + return '' try: - return self.thumbnail.url - except ValueError: - log.exception(f'File pk={self.pk} is missing a large thumbnail') + path = self.metadata['thumbnails'][size_key]['path'] + return self.thumbnail.storage.url(path) + except (KeyError, TypeError): + log.exception(f'File pk={self.pk} is missing thumbnail "{size_key}": {self.metadata}') return self.source.url + @property + def thumbnail_l_url(self) -> str: + return self.get_thumbnail_of_size('l') + @property def thumbnail_s_url(self) -> str: - """Return absolute path portion of the URL of the small thumbnail of this file. - - Fall back to the source file, if no thumbnail is stored. - Log absence of the thumbnail file instead of exploding somewhere in the templates. - """ - try: - s = self.metadata['thumbnails']['s']['path'] - return self.thumbnail.storage.url(s) - except (KeyError, TypeError): - log.exception(f'File pk={self.pk} is missing a small thumbnail: {self.metadata}') - return self.source.url + return self.get_thumbnail_of_size('s') class FileValidation(CreatedModifiedMixin, TrackChangesMixin, models.Model): diff --git a/files/tasks.py b/files/tasks.py index cd10b924..13e3bd69 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -42,21 +42,24 @@ def make_thumbnails(file_id: int) -> None: assert file.is_image or file.is_video, f'File pk={file_id} is neither image nor video' # For an image, source of the thumbnails is the original image - source_path = os.path.join(settings.MEDIA_ROOT, file.source.path) + source_path = file.source.path + thumbnail_field = file.thumbnail + unchanged_thumbnail = thumbnail_field.name if file.is_video: - f_path = os.path.join(settings.MEDIA_ROOT, files.utils.get_thumbnail_upload_to(file.hash)) + frame_path = files.utils.get_thumbnail_upload_to(file.hash) # For a video, source of the thumbnails is a frame extracted with ffpeg - files.utils.extract_frame(source_path, f_path) - source_path = f_path + files.utils.extract_frame(source_path, frame_path) + thumbnail_field.name = frame_path + source_path = frame_path thumbnails = files.utils.make_thumbnails(source_path, file.hash) - thumbnail_default_path = thumbnails['l']['path'] + + if not thumbnail_field.name: + thumbnail_field.name = thumbnails['l']['path'] update_fields = set() - thumbnail_field = file.thumbnail - if thumbnail_default_path != thumbnail_field.name: - thumbnail_field.name = thumbnail_default_path + if thumbnail_field.name != unchanged_thumbnail: update_fields.add('thumbnail') if file.metadata.get('thumbnails') != thumbnails: file.metadata.update({'thumbnails': thumbnails}) diff --git a/files/tests/test_tasks.py b/files/tests/test_tasks.py index 530f33c7..fdd94266 100644 --- a/files/tests/test_tasks.py +++ b/files/tests/test_tasks.py @@ -12,11 +12,33 @@ TEST_MEDIA_DIR = Path(__file__).resolve().parent / 'media' @override_settings(MEDIA_ROOT=TEST_MEDIA_DIR) class TasksTest(TestCase): + def test_make_thumbnails_fails_when_no_validation(self): + file = FileFactory(original_hash='foobar', source='file/original_image_source.jpg') + + with self.assertRaises(files.models.File.validation.RelatedObjectDoesNotExist): + make_thumbnails.task_function(file_id=file.pk) + + def test_make_thumbnails_fails_when_validation_not_ok(self): + file = FileFactory(original_hash='foobar', source='file/original_image_source.jpg') + files.models.FileValidation.objects.create(file=file, is_ok=False, results={}) + + with self.assertRaises(AssertionError): + make_thumbnails.task_function(file_id=file.pk) + + def test_make_thumbnails_fails_when_not_image_or_video(self): + file = FileFactory( + original_hash='foobar', source='file/source.zip', type=files.models.File.TYPES.THEME + ) + + make_thumbnails.task_function(file_id=file.pk) + @patch('files.utils.resize_image') @patch('files.utils.Image') def test_make_thumbnails_for_image(self, mock_image, mock_resize_image): file = FileFactory(original_hash='foobar', source='file/original_image_source.jpg') files.models.FileValidation.objects.create(file=file, is_ok=True, results={}) + self.assertIsNone(file.thumbnail.name) + self.assertEqual(file.metadata, {}) make_thumbnails.task_function(file_id=file.pk) @@ -24,7 +46,9 @@ class TasksTest(TestCase): str(TEST_MEDIA_DIR / 'file' / 'original_image_source.jpg') ) mock_image.open.return_value.close.assert_called_once() + file.refresh_from_db() + self.assertEqual(file.thumbnail.name, 'thumbnails/fo/foobar_1920x1080.png') self.assertEqual( file.metadata, { @@ -34,7 +58,6 @@ class TasksTest(TestCase): }, }, ) - self.assertEqual(file.thumbnail.name, 'thumbnails/fo/foobar_1920x1080.png') @patch('files.utils.resize_image') @patch('files.utils.Image') @@ -44,6 +67,8 @@ class TasksTest(TestCase): original_hash='deadbeef', source='file/path.mp4', type=files.models.File.TYPES.VIDEO ) files.models.FileValidation.objects.create(file=file, is_ok=True, results={}) + self.assertIsNone(file.thumbnail.name) + self.assertEqual(file.metadata, {}) make_thumbnails.task_function(file_id=file.pk) @@ -52,7 +77,11 @@ class TasksTest(TestCase): str(TEST_MEDIA_DIR / 'thumbnails' / 'de' / 'deadbeef.png') ) mock_image.open.return_value.close.assert_called_once() + file.refresh_from_db() + # Check that the extracted frame is stored instead of the large thumbnail + self.assertEqual(file.thumbnail.name, 'thumbnails/de/deadbeef.png') + # Check that File metadata and thumbnail fields were updated self.assertEqual( file.metadata, { @@ -62,4 +91,3 @@ class TasksTest(TestCase): }, }, ) - self.assertEqual(file.thumbnail.name, 'thumbnails/de/deadbeef_1920x1080.png') diff --git a/files/utils.py b/files/utils.py index 537f8c24..75dd54e4 100644 --- a/files/utils.py +++ b/files/utils.py @@ -11,6 +11,7 @@ import typing import zipfile from PIL import Image +from django.conf import settings from django.core.files.storage import default_storage from ffmpeg import FFmpeg, FFmpegFileNotFound, FFmpegInvalidCommand, FFmpegError from lxml import etree @@ -225,19 +226,22 @@ def resize_image(image: Image, size: tuple, output, output_format: str = 'PNG', source_image.save(output, output_format, **output_params) -def make_thumbnails(file_path: str, file_hash: str, output_format: str = THUMBNAIL_FORMAT) -> dict: +def make_thumbnails( + source_path: str, file_hash: str, output_format: str = THUMBNAIL_FORMAT +) -> dict: """Generate thumbnail files for given file and a predefined list of dimensions. Resulting thumbnail paths a derived from the given file hash and thumbnail sizes. Return a dict of size keys to output paths of generated thumbnail images. """ thumbnails = {} - image = Image.open(file_path) + abs_path = os.path.join(settings.MEDIA_ROOT, source_path) + image = Image.open(abs_path) for size_key, (w, h) in THUMBNAIL_SIZES.items(): output_path = get_thumbnail_upload_to(file_hash, width=w, height=h) size = (w, h) with tempfile.TemporaryFile() as f: - logger.info('Resizing %s to %s (%s)', file_path, size, output_format) + logger.info('Resizing %s to %s (%s)', abs_path, size, output_format) resize_image( image, size, @@ -261,13 +265,14 @@ def make_thumbnails(file_path: str, file_hash: str, output_format: str = THUMBNA def extract_frame(source_path: str, output_path: str, at_time: str = '00:00:00.01'): """Extract a single frame of a video at a given path, write it to the given output path.""" try: + abs_path = os.path.join(settings.MEDIA_ROOT, output_path) ffmpeg = ( FFmpeg() .option('y') .input(source_path) - .output(output_path, {'ss': at_time, 'frames:v': 1, 'update': 'true'}) + .output(abs_path, {'ss': at_time, 'frames:v': 1, 'update': 'true'}) ) - output_dir = os.path.dirname(output_path) + output_dir = os.path.dirname(abs_path) if not os.path.isdir(output_dir): os.mkdir(output_dir) ffmpeg.execute() -- 2.30.2 From f584f1a1bd4c41b074dbf742959e95e08dae8477 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 15:55:01 +0200 Subject: [PATCH 17/23] Use "pixel sizes" as keys in the metadata --- constants/base.py | 4 +--- .../templates/extensions/components/card.html | 6 +++--- .../extensions/components/galleria.html | 6 +++--- files/admin.py | 7 +------ files/models.py | 8 ++++---- files/tasks.py | 2 +- files/templates/files/admin/thumbnails.html | 20 ++++++------------- files/tests/test_tasks.py | 8 ++++---- files/tests/test_utils.py | 4 ++-- .../reviewers/extensions_review_detail.html | 4 ++-- 10 files changed, 27 insertions(+), 42 deletions(-) diff --git a/constants/base.py b/constants/base.py index e3f8cd67..de32a563 100644 --- a/constants/base.py +++ b/constants/base.py @@ -104,8 +104,6 @@ ABUSE_TYPE = Choices( # **N.B.**: thumbnail sizes are not intended to be changed on the fly: # thumbnails of existing images must exist in MEDIA_ROOT before # the code expecting thumbnails of new dimensions can be deployed! -THUMBNAIL_SIZE_L = (1920, 1080) -THUMBNAIL_SIZE_S = (640, 360) -THUMBNAIL_SIZES = {'l': THUMBNAIL_SIZE_L, 's': THUMBNAIL_SIZE_S} +THUMBNAIL_SIZES = {'1080p': [1920, 1080], '360p': [640, 360]} THUMBNAIL_FORMAT = 'PNG' THUMBNAIL_QUALITY = 83 diff --git a/extensions/templates/extensions/components/card.html b/extensions/templates/extensions/components/card.html index 689ae3e9..4dea6d6e 100644 --- a/extensions/templates/extensions/components/card.html +++ b/extensions/templates/extensions/components/card.html @@ -1,13 +1,13 @@ {% load common filters %} -{% with latest=extension.latest_version thumbnail_s_url=extension.previews.listed.first.thumbnail_s_url %} +{% with latest=extension.latest_version thumbnail_360p_url=extension.previews.listed.first.thumbnail_360p_url %}

{% if blur %} -
+
{% endif %} -
+
diff --git a/extensions/templates/extensions/components/galleria.html b/extensions/templates/extensions/components/galleria.html index 4be38f1a..66a6c011 100644 --- a/extensions/templates/extensions/components/galleria.html +++ b/extensions/templates/extensions/components/galleria.html @@ -3,15 +3,15 @@ {% if previews %}
{% for preview in previews %} - {% with thumbnail_l_url=preview.thumbnail_l_url %} + {% with thumbnail_1080p_url=preview.thumbnail_1080p_url %} - {{ preview.preview.caption }} + {{ preview.preview.caption }} {% endwith %} {% endfor %} diff --git a/files/admin.py b/files/admin.py index 30c43687..8120849b 100644 --- a/files/admin.py +++ b/files/admin.py @@ -7,7 +7,6 @@ import background_task.admin import background_task.models from .models import File, FileValidation -from constants.base import THUMBNAIL_SIZE_L import files.signals logger = logging.getLogger(__name__) @@ -47,11 +46,7 @@ class FileAdmin(admin.ModelAdmin): if not obj or not (obj.is_image or obj.is_video): return '' try: - context = { - 'file': obj, - 'MEDIA_URL': settings.MEDIA_URL, - 'THUMBNAIL_SIZE_L': THUMBNAIL_SIZE_L, - } + context = {'file': obj, 'MEDIA_URL': settings.MEDIA_URL} return render_to_string('files/admin/thumbnails.html', context) except Exception: # Make sure any exception happening here is always logged diff --git a/files/models.py b/files/models.py index 72cc0ccb..2500a3b1 100644 --- a/files/models.py +++ b/files/models.py @@ -213,12 +213,12 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): return self.source.url @property - def thumbnail_l_url(self) -> str: - return self.get_thumbnail_of_size('l') + def thumbnail_1080p_url(self) -> str: + return self.get_thumbnail_of_size('1080p') @property - def thumbnail_s_url(self) -> str: - return self.get_thumbnail_of_size('s') + def thumbnail_360p_url(self) -> str: + return self.get_thumbnail_of_size('360p') class FileValidation(CreatedModifiedMixin, TrackChangesMixin, models.Model): diff --git a/files/tasks.py b/files/tasks.py index 13e3bd69..05907b7b 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -56,7 +56,7 @@ def make_thumbnails(file_id: int) -> None: thumbnails = files.utils.make_thumbnails(source_path, file.hash) if not thumbnail_field.name: - thumbnail_field.name = thumbnails['l']['path'] + thumbnail_field.name = thumbnails['1080p']['path'] update_fields = set() if thumbnail_field.name != unchanged_thumbnail: diff --git a/files/templates/files/admin/thumbnails.html b/files/templates/files/admin/thumbnails.html index 9def9f30..4610fdf5 100644 --- a/files/templates/files/admin/thumbnails.html +++ b/files/templates/files/admin/thumbnails.html @@ -1,16 +1,8 @@
-{% for size_key, thumb in file.metadata.thumbnails.items %} -
- {{ thumb.size.0 }}x{{ thumb.size.1 }}px - -
-{% endfor %} -{# alert about unexpected size of the default thumbnail, which might happen if source file was smaller than 1080p #} -{# TODO: do we want to make minimum resolution a validation criteria during upload instead? #} -{% if file.thumbnail.width != THUMBNAIL_SIZE_L.0 or file.thumbnail.height != THUMBNAIL_SIZE_L.1 %} -

- ⚠ Expected {{ THUMBNAIL_SIZE_L.0 }}x{{ THUMBNAIL_SIZE_L.1 }}px, - got {{ file.thumbnail.width }}x{{ file.thumbnail.height }}px: resolution of the source file might be too low? -

-{% endif %} + {% for size_key, thumb in file.metadata.thumbnails.items %} +
+ {{ thumb.size.0 }}x{{ thumb.size.1 }}px + +
+ {% endfor %}
diff --git a/files/tests/test_tasks.py b/files/tests/test_tasks.py index fdd94266..74ab02d8 100644 --- a/files/tests/test_tasks.py +++ b/files/tests/test_tasks.py @@ -53,8 +53,8 @@ class TasksTest(TestCase): file.metadata, { 'thumbnails': { - 'l': {'path': 'thumbnails/fo/foobar_1920x1080.png', 'size': [1920, 1080]}, - 's': {'path': 'thumbnails/fo/foobar_640x360.png', 'size': [640, 360]}, + '1080p': {'path': 'thumbnails/fo/foobar_1920x1080.png', 'size': [1920, 1080]}, + '360p': {'path': 'thumbnails/fo/foobar_640x360.png', 'size': [640, 360]}, }, }, ) @@ -86,8 +86,8 @@ class TasksTest(TestCase): file.metadata, { 'thumbnails': { - 'l': {'path': 'thumbnails/de/deadbeef_1920x1080.png', 'size': [1920, 1080]}, - 's': {'path': 'thumbnails/de/deadbeef_640x360.png', 'size': [640, 360]}, + '1080p': {'path': 'thumbnails/de/deadbeef_1920x1080.png', 'size': [1920, 1080]}, + '360p': {'path': 'thumbnails/de/deadbeef_640x360.png', 'size': [640, 360]}, }, }, ) diff --git a/files/tests/test_utils.py b/files/tests/test_utils.py index 6dd2a6c2..58270be4 100644 --- a/files/tests/test_utils.py +++ b/files/tests/test_utils.py @@ -126,8 +126,8 @@ class UtilsTest(TestCase): def test_make_thumbnails(self, mock_resize_image): self.assertEqual( { - 'l': {'path': 'thumbnails/fo/foobar_1920x1080.png', 'size': (1920, 1080)}, - 's': {'path': 'thumbnails/fo/foobar_640x360.png', 'size': (640, 360)}, + '1080p': {'path': 'thumbnails/fo/foobar_1920x1080.png', 'size': (1920, 1080)}, + '360p': {'path': 'thumbnails/fo/foobar_640x360.png', 'size': (640, 360)}, }, make_thumbnails(TEST_FILES_DIR / 'test_preview_image_0001.png', 'foobar'), ) diff --git a/reviewers/templates/reviewers/extensions_review_detail.html b/reviewers/templates/reviewers/extensions_review_detail.html index b870e048..f3975e91 100644 --- a/reviewers/templates/reviewers/extensions_review_detail.html +++ b/reviewers/templates/reviewers/extensions_review_detail.html @@ -76,10 +76,10 @@

Previews Pending Approval

{% for preview in pending_previews %} - {% with thumbnail_l_url=preview.file.thumbnail_l_url %} + {% with thumbnail_1080p_url=preview.file.thumbnail_1080p_url %}
- {{ preview.caption }} + {{ preview.caption }} {% include "common/components/status.html" with object=preview.file class="d-block" %}
-- 2.30.2 From 381daa4177d440620423d8133734386a2430e436 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 16:01:01 +0200 Subject: [PATCH 18/23] Break when asked for thumbnail of an unexpected type --- files/models.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/files/models.py b/files/models.py index 2500a3b1..661c3b25 100644 --- a/files/models.py +++ b/files/models.py @@ -203,8 +203,9 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): Fall back to the source file, if no thumbnail is stored. Log absence of the thumbnail file instead of exploding somewhere in the templates. """ - if not self.is_image and not self.is_video: - return '' + # We don't (yet?) have thumbnails for anything other than images and videos. + assert self.is_image or self.is_video, f'File pk={self.pk} is neither image nor video' + try: path = self.metadata['thumbnails'][size_key]['path'] return self.thumbnail.storage.url(path) -- 2.30.2 From 6902a900880ee03fb3e568b9c450fbb4b65116c3 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 16:50:53 +0200 Subject: [PATCH 19/23] Avoid re-scheduling thumbnail tasks for non-media or flagged files --- files/tasks.py | 13 ++++++++----- files/tests/test_tasks.py | 29 ++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/files/tasks.py b/files/tasks.py index 05907b7b..e7ed671a 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -34,12 +34,13 @@ def make_thumbnails(file_id: int) -> None: """Generate thumbnails for a given file, store them in thumbnail and metadata columns.""" file = files.models.File.objects.get(pk=file_id) args = {'pk': file_id, 'type': file.get_type_display()} - if not file.is_image and not file.is_video: - logger.warning('File pk=%(pk)s is neither an image nor a video ("%(type)s")', args) - return - assert file.validation.is_ok, f'File pk={file_id} is flagged' - assert file.is_image or file.is_video, f'File pk={file_id} is neither image nor video' + if not file.is_image and not file.is_video: + logger.error('File pk=%(pk)s of type "%(type)s" is neither an image nor a video', args) + return + if not file.validation.is_ok: + logger.error("File pk={pk} is flagged, won't make thumbnails".format(**args)) + return # For an image, source of the thumbnails is the original image source_path = file.source.path @@ -65,4 +66,6 @@ def make_thumbnails(file_id: int) -> None: file.metadata.update({'thumbnails': thumbnails}) update_fields.add('metadata') if update_fields: + args['update_fields'] = update_fields + logger.info('Made thumbnails for file pk=%(pk)s, updating %(update_fields)s', args) file.save(update_fields=update_fields) diff --git a/files/tests/test_tasks.py b/files/tests/test_tasks.py index 74ab02d8..a0832c1a 100644 --- a/files/tests/test_tasks.py +++ b/files/tests/test_tasks.py @@ -1,5 +1,6 @@ -from unittest.mock import patch from pathlib import Path +from unittest.mock import patch +import logging from django.test import TestCase, override_settings @@ -18,19 +19,37 @@ class TasksTest(TestCase): with self.assertRaises(files.models.File.validation.RelatedObjectDoesNotExist): make_thumbnails.task_function(file_id=file.pk) - def test_make_thumbnails_fails_when_validation_not_ok(self): + @patch('files.utils.make_thumbnails') + def test_make_thumbnails_fails_when_validation_not_ok(self, mock_make_thumbnails): file = FileFactory(original_hash='foobar', source='file/original_image_source.jpg') files.models.FileValidation.objects.create(file=file, is_ok=False, results={}) - with self.assertRaises(AssertionError): + with self.assertLogs(level=logging.ERROR) as logs: make_thumbnails.task_function(file_id=file.pk) - def test_make_thumbnails_fails_when_not_image_or_video(self): + self.maxDiff = None + self.assertEqual( + logs.output[0], f"ERROR:files.tasks:File pk={file.pk} is flagged, won't make thumbnails" + ) + + mock_make_thumbnails.assert_not_called() + + @patch('files.utils.make_thumbnails') + def test_make_thumbnails_fails_when_not_image_or_video(self, mock_make_thumbnails): file = FileFactory( original_hash='foobar', source='file/source.zip', type=files.models.File.TYPES.THEME ) - make_thumbnails.task_function(file_id=file.pk) + with self.assertLogs(level=logging.ERROR) as logs: + make_thumbnails.task_function(file_id=file.pk) + + self.maxDiff = None + self.assertEqual( + logs.output[0], + f'ERROR:files.tasks:File pk={file.pk} of type "Theme" is neither an image nor a video', + ) + + mock_make_thumbnails.assert_not_called() @patch('files.utils.resize_image') @patch('files.utils.Image') -- 2.30.2 From 6d94e12cdd3e140d865349d28dc496af0618d46d Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 16:52:42 +0200 Subject: [PATCH 20/23] Wording --- files/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/files/utils.py b/files/utils.py index 75dd54e4..ec43f420 100644 --- a/files/utils.py +++ b/files/utils.py @@ -251,7 +251,7 @@ def make_thumbnails( optimize=True, progressive=True, ) - logger.info('Saving a new thumbnail to %s', output_path) + logger.info('Saving a thumbnail to %s', output_path) # Overwrite files instead of allowing storage generate a deduplicating suffix if default_storage.exists(output_path): logger.warning('%s exists, overwriting', output_path) -- 2.30.2 From b071559152b0bcbaf603fdfefde32995d4dc41aa Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 17:08:09 +0200 Subject: [PATCH 21/23] Clean up the unnecessary untupling --- files/tests/test_utils.py | 6 +++--- files/utils.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/files/tests/test_utils.py b/files/tests/test_utils.py index 58270be4..ca023530 100644 --- a/files/tests/test_utils.py +++ b/files/tests/test_utils.py @@ -126,14 +126,14 @@ class UtilsTest(TestCase): def test_make_thumbnails(self, mock_resize_image): self.assertEqual( { - '1080p': {'path': 'thumbnails/fo/foobar_1920x1080.png', 'size': (1920, 1080)}, - '360p': {'path': 'thumbnails/fo/foobar_640x360.png', 'size': (640, 360)}, + '1080p': {'path': 'thumbnails/fo/foobar_1920x1080.png', 'size': [1920, 1080]}, + '360p': {'path': 'thumbnails/fo/foobar_640x360.png', 'size': [640, 360]}, }, make_thumbnails(TEST_FILES_DIR / 'test_preview_image_0001.png', 'foobar'), ) self.assertEqual(len(mock_resize_image.mock_calls), 2) - for expected_size in ((1920, 1080), (640, 360)): + for expected_size in ([1920, 1080], [640, 360]): with self.subTest(expected_size=expected_size): mock_resize_image.assert_any_call( ANY, diff --git a/files/utils.py b/files/utils.py index ec43f420..11f63afa 100644 --- a/files/utils.py +++ b/files/utils.py @@ -237,9 +237,9 @@ def make_thumbnails( thumbnails = {} abs_path = os.path.join(settings.MEDIA_ROOT, source_path) image = Image.open(abs_path) - for size_key, (w, h) in THUMBNAIL_SIZES.items(): + for size_key, size in THUMBNAIL_SIZES.items(): + w, h = size output_path = get_thumbnail_upload_to(file_hash, width=w, height=h) - size = (w, h) with tempfile.TemporaryFile() as f: logger.info('Resizing %s to %s (%s)', abs_path, size, output_format) resize_image( -- 2.30.2 From e56a7c869485ae943e40e6f8012dc8dd2acda254 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 17:29:02 +0200 Subject: [PATCH 22/23] Remove unused css --- files/static/files/admin/file.css | 3 --- 1 file changed, 3 deletions(-) diff --git a/files/static/files/admin/file.css b/files/static/files/admin/file.css index f44f732b..80491326 100644 --- a/files/static/files/admin/file.css +++ b/files/static/files/admin/file.css @@ -9,6 +9,3 @@ padding-right: 0.5rem; padding-left: 0.5rem; } -.file-thumbnails .icon-alert { - color: red; -} -- 2.30.2 From 361c0490a6ad850416178e87ddddd6577309bd5e Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 25 Apr 2024 17:48:20 +0200 Subject: [PATCH 23/23] Time thumbnail processing --- files/utils.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/files/utils.py b/files/utils.py index 11f63afa..0ae1dcb7 100644 --- a/files/utils.py +++ b/files/utils.py @@ -1,4 +1,5 @@ from pathlib import Path +import datetime import hashlib import io import logging @@ -221,10 +222,16 @@ def get_thumbnail_upload_to(file_hash: str, width: int = None, height: int = Non def resize_image(image: Image, size: tuple, output, output_format: str = 'PNG', **output_params): """Resize a models.ImageField to a given size and write it into output file.""" + start_t = datetime.datetime.now() + source_image = image.convert('RGBA' if output_format == 'PNG' else 'RGB') source_image.thumbnail(size, Image.LANCZOS) source_image.save(output, output_format, **output_params) + end_t = datetime.datetime.now() + args = {'source': image, 'size': size, 'time': (end_t - start_t).microseconds / 1000} + logger.info('%(source)s to %(size)s done in %(time)sms', args) + def make_thumbnails( source_path: str, file_hash: str, output_format: str = THUMBNAIL_FORMAT @@ -234,6 +241,7 @@ def make_thumbnails( Resulting thumbnail paths a derived from the given file hash and thumbnail sizes. Return a dict of size keys to output paths of generated thumbnail images. """ + start_t = datetime.datetime.now() thumbnails = {} abs_path = os.path.join(settings.MEDIA_ROOT, source_path) image = Image.open(abs_path) @@ -259,12 +267,17 @@ def make_thumbnails( default_storage.save(output_path, f) thumbnails[size_key] = {'size': size, 'path': output_path} image.close() + + end_t = datetime.datetime.now() + args = {'source': source_path, 'time': (end_t - start_t).microseconds / 1000} + logger.info('%(source)s done in %(time)sms', args) return thumbnails def extract_frame(source_path: str, output_path: str, at_time: str = '00:00:00.01'): """Extract a single frame of a video at a given path, write it to the given output path.""" try: + start_t = datetime.datetime.now() abs_path = os.path.join(settings.MEDIA_ROOT, output_path) ffmpeg = ( FFmpeg() @@ -276,6 +289,10 @@ def extract_frame(source_path: str, output_path: str, at_time: str = '00:00:00.0 if not os.path.isdir(output_dir): os.mkdir(output_dir) ffmpeg.execute() + + end_t = datetime.datetime.now() + args = {'source': source_path, 'time': (end_t - start_t).microseconds / 1000} + logger.info('%(source)s done in %(time)sms', args) except (FFmpegError, FFmpegFileNotFound, FFmpegInvalidCommand) as e: logger.exception(f'Failed to extract a frame: {e.message}, {" ".join(ffmpeg.arguments)}') raise -- 2.30.2