Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix thumbnailing remote files #2789

Merged
merged 9 commits into from
Jan 17, 2018
Merged

Fix thumbnailing remote files #2789

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a4c5e4a
Fix thumbnailing remote files
erikjohnston Jan 16, 2018
c5b589f
Log when we respond with 404
erikjohnston Jan 16, 2018
9795b9e
Correctly use server_name/file_id when generating/fetching remote thu…
erikjohnston Jan 16, 2018
307f88d
Fix up log lines
erikjohnston Jan 16, 2018
5dfc837
Fix typo
erikjohnston Jan 16, 2018
0a90d9e
Move setting of file_id up to caller
erikjohnston Jan 16, 2018
6368e5c
Change _generate_thumbnails to take media_type
erikjohnston Jan 16, 2018
d863f68
Use local vars
erikjohnston Jan 16, 2018
d728c47
Add docstring
erikjohnston Jan 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 42 additions & 10 deletions synapse/rest/media/v1/media_repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,9 @@ def create_content(self, media_type, upload_name, content, content_length,
"media_length": content_length,
}

yield self._generate_thumbnails(None, media_id, media_info)
yield self._generate_thumbnails(
None, media_id, media_id, media_info["media_type"],
Copy link
Member

Choose a reason for hiding this comment

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

s/media_info["media_type"]/media_type/

media_info now appears to be redundant here

)

defer.returnValue("mxc://%s/%s" % (self.server_name, media_id))

Expand Down Expand Up @@ -227,6 +229,34 @@ def get_remote_media(self, request, server_name, media_id, name):
else:
respond_404(request)

@defer.inlineCallbacks
def get_remote_media_info(self, server_name, media_id):
"""Gets the media info associated with the remote file, downloading
if necessary.

Args:
server_name (str): Remote server_name where the media originated.
media_id (str): The media ID of the content (as defined by the
remote server).

Returns:
Deferred[dict]: The media_info of the file
"""
# We linearize here to ensure that we don't try and download remote
# media multiple times concurrently
key = (server_name, media_id)
with (yield self.remote_media_linearizer.queue(key)):
responder, media_info = yield self._get_remote_media_impl(
server_name, media_id,
)

# Ensure we actually use the responder so that it releases resources
if responder:
with responder:
pass

defer.returnValue(media_info)

@defer.inlineCallbacks
def _get_remote_media_impl(self, server_name, media_id):
"""Looks for media in local cache, if not there then attempt to
Expand Down Expand Up @@ -257,6 +287,7 @@ def _get_remote_media_impl(self, server_name, media_id):
# If we have an entry in the DB, try and look for it
if media_info:
if media_info["quarantined_by"]:
logger.info("Media is quarantined")
raise NotFoundError()

responder = yield self.media_storage.fetch_media(file_info)
Expand Down Expand Up @@ -384,7 +415,7 @@ def _download_remote_file(self, server_name, media_id, file_id):
}

yield self._generate_thumbnails(
server_name, media_id, media_info
server_name, media_id, file_id, media_info["media_type"],
Copy link
Member

Choose a reason for hiding this comment

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

just media_type

)

defer.returnValue(media_info)
Expand Down Expand Up @@ -496,21 +527,22 @@ def generate_remote_exact_thumbnail(self, server_name, file_id, media_id,
defer.returnValue(output_path)

@defer.inlineCallbacks
def _generate_thumbnails(self, server_name, media_id, media_info, url_cache=False):
def _generate_thumbnails(self, server_name, media_id, file_id, media_type,
url_cache=False):
"""Generate and store thumbnails for an image.

Args:
server_name(str|None): The server name if remote media, else None if local
media_id(str)
media_info(dict)
url_cache(bool): If we are thumbnailing images downloaded for the URL cache,
server_name (str|None): The server name if remote media, else None if local
media_id (str): The media ID of the content. (This is the same as
the file_id for local content)
file_id (str): Local file ID
media_type (str)
Copy link
Member

Choose a reason for hiding this comment

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

doc, please!

url_cache (bool): If we are thumbnailing images downloaded for the URL cache,
used exclusively by the url previewer

Returns:
Deferred[dict]: Dict with "width" and "height" keys of original image
"""
media_type = media_info["media_type"]
file_id = media_info.get("filesystem_id")
requirements = self._get_thumbnail_requirements(media_type)
if not requirements:
return
Expand Down Expand Up @@ -568,7 +600,7 @@ def _generate_thumbnails(self, server_name, media_id, media_info, url_cache=Fals
try:
file_info = FileInfo(
server_name=server_name,
file_id=media_id,
file_id=file_id,
thumbnail=True,
thumbnail_width=t_width,
thumbnail_height=t_height,
Expand Down
8 changes: 6 additions & 2 deletions synapse/rest/media/v1/preview_url_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,10 @@ def _do_preview(self, url, user, ts):
logger.debug("got media_info of '%s'" % media_info)

if _is_media(media_info['media_type']):
file_id = media_info['filesystem_id']
dims = yield self.media_repo._generate_thumbnails(
None, media_info['filesystem_id'], media_info, url_cache=True,
None, file_id, file_id, media_info["media_type"],
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to factor out a local media_type here too, given the number of times it's referenced, but shrug

url_cache=True,
)

og = {
Expand Down Expand Up @@ -231,8 +233,10 @@ def _do_preview(self, url, user, ts):

if _is_media(image_info['media_type']):
# TODO: make sure we don't choke on white-on-transparent images
file_id = image_info['filesystem_id']
dims = yield self.media_repo._generate_thumbnails(
None, image_info['filesystem_id'], image_info, url_cache=True,
None, file_id, file_id, image_info["media_type"],
url_cache=True,
)
if dims:
og["og:image:width"] = dims['width']
Expand Down
28 changes: 20 additions & 8 deletions synapse/rest/media/v1/thumbnail_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,11 @@ def _respond_local_thumbnail(self, request, media_id, width, height,
method, m_type):
media_info = yield self.store.get_local_media(media_id)

if not media_info or media_info["quarantined_by"]:
if not media_info:
respond_404(request)
return
if media_info["quarantined_by"]:
logger.info("Media is quarantined")
Copy link
Member

Choose a reason for hiding this comment

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

we don't know for sure it is quarantined here

respond_404(request)
return

Expand All @@ -111,6 +115,7 @@ def _respond_local_thumbnail(self, request, media_id, width, height,
responder = yield self.media_storage.fetch_media(file_info)
yield respond_with_responder(request, responder, t_type, t_length)
else:
logger.info("Couldn't find any generated thumbnails")
respond_404(request)

@defer.inlineCallbacks
Expand All @@ -119,7 +124,11 @@ def _select_or_generate_local_thumbnail(self, request, media_id, desired_width,
desired_type):
media_info = yield self.store.get_local_media(media_id)

if not media_info or media_info["quarantined_by"]:
if not media_info:
respond_404(request)
return
if media_info["quarantined_by"]:
logger.info("Media is quarantined")
Copy link
Member

Choose a reason for hiding this comment

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

ditto

respond_404(request)
return

Expand Down Expand Up @@ -149,7 +158,7 @@ def _select_or_generate_local_thumbnail(self, request, media_id, desired_width,
yield respond_with_responder(request, responder, t_type, t_length)
return

logger.debug("We don't have a local thumbnail of that size. Generating")
logger.debug("We don't have a thumbnail of that size. Generating")

# Okay, so we generate one.
file_path = yield self.media_repo.generate_local_exact_thumbnail(
Expand All @@ -159,13 +168,14 @@ def _select_or_generate_local_thumbnail(self, request, media_id, desired_width,
if file_path:
yield respond_with_file(request, desired_type, file_path)
else:
logger.warn("Failed to generate thumbnail")
respond_404(request)

@defer.inlineCallbacks
def _select_or_generate_remote_thumbnail(self, request, server_name, media_id,
desired_width, desired_height,
desired_method, desired_type):
media_info = yield self.media_repo.get_remote_media(server_name, media_id)
media_info = yield self.media_repo.get_remote_media_info(server_name, media_id)

thumbnail_infos = yield self.store.get_remote_media_thumbnails(
server_name, media_id,
Expand All @@ -181,7 +191,7 @@ def _select_or_generate_remote_thumbnail(self, request, server_name, media_id,

if t_w and t_h and t_method and t_type:
file_info = FileInfo(
server_name=None, file_id=media_id,
server_name=server_name, file_id=media_info["filesystem_id"],
thumbnail=True,
thumbnail_width=info["thumbnail_width"],
thumbnail_height=info["thumbnail_height"],
Expand All @@ -197,7 +207,7 @@ def _select_or_generate_remote_thumbnail(self, request, server_name, media_id,
yield respond_with_responder(request, responder, t_type, t_length)
return

logger.debug("We don't have a local thumbnail of that size. Generating")
logger.debug("We don't have a thumbnail of that size. Generating")

# Okay, so we generate one.
file_path = yield self.media_repo.generate_remote_exact_thumbnail(
Expand All @@ -208,6 +218,7 @@ def _select_or_generate_remote_thumbnail(self, request, server_name, media_id,
if file_path:
yield respond_with_file(request, desired_type, file_path)
else:
logger.warn("Failed to generate thumbnail")
respond_404(request)

@defer.inlineCallbacks
Expand All @@ -216,7 +227,7 @@ def _respond_remote_thumbnail(self, request, server_name, media_id, width,
# TODO: Don't download the whole remote file
# We should proxy the thumbnail from the remote server instead of
# downloading the remote file and generating our own thumbnails.
yield self.media_repo.get_remote_media(server_name, media_id)
media_info = yield self.media_repo.get_remote_media_info(server_name, media_id)

thumbnail_infos = yield self.store.get_remote_media_thumbnails(
server_name, media_id,
Expand All @@ -227,7 +238,7 @@ def _respond_remote_thumbnail(self, request, server_name, media_id, width,
width, height, method, m_type, thumbnail_infos
)
file_info = FileInfo(
server_name=None, file_id=media_id,
server_name=server_name, file_id=media_info["filesystem_id"],
thumbnail=True,
thumbnail_width=thumbnail_info["thumbnail_width"],
thumbnail_height=thumbnail_info["thumbnail_height"],
Expand All @@ -241,6 +252,7 @@ def _respond_remote_thumbnail(self, request, server_name, media_id, width,
responder = yield self.media_storage.fetch_media(file_info)
yield respond_with_responder(request, responder, t_type, t_length)
else:
logger.info("Failed to find any generated thumbnails")
respond_404(request)

def _select_thumbnail(self, desired_width, desired_height, desired_method,
Expand Down