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

Return ThumbnailInfo in more places #16438

Merged
merged 4 commits into from
Oct 6, 2023
Merged

Return ThumbnailInfo in more places #16438

merged 4 commits into from
Oct 6, 2023

Conversation

clokep
Copy link
Member

@clokep clokep commented Oct 5, 2023

Use attrs classes in more spots instead of dictionaries.

Kind of related to #16431.

@clokep clokep marked this pull request as ready for review October 5, 2023 17:47
@clokep clokep requested a review from a team as a code owner October 5, 2023 17:47
@@ -616,6 +616,7 @@ async def generate_local_exact_thumbnail(
height=t_height,
method=t_method,
type=t_type,
length=t_byte_source.tell(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Sanity check: is t_byte_source definitely going to be fully written with the cursor at the end of the buffer, so that .tell() is the length? Both .crop() and .scale() say that they return "bytes ... ready to be written to disk" so I assume this is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

My understand is that it will be written to and the cursor will be at the end of buffer; we could do something like:

cur = t_byte_source.tell()
t_byte_source.seek(0, os.SEEK_END)
end = t_byte_source.tell()
t_byte_source.seek(cur, os.SEEK_SET)

If you're concerned about it not being at the end of the buffer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reading back through the code I think this is fine TBH.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was my understanding too, I just wanted to check this through. Tinkering in an interpreter, our understanding seems to be correct:

 20230728 (Red Hat 13.2.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from tests.test_utils import SMALL_PNG
>>> from io import BytesIO
>>> src = Image.open(BytesIO(SMALL_PNG))
>>> output = BytesIO()
>>> src.save(output, "png")
>>> output.tell()
70
>>> len(output.getvalue())
70

Though curiously this is slightly larger than the original PNG:

>>> len(SMALL_PNG)
67

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway LGTM, :shipit:

synapse/rest/media/thumbnail_resource.py Show resolved Hide resolved
synapse/rest/media/thumbnail_resource.py Show resolved Hide resolved
type_quality = desired_type != info["thumbnail_type"]
length_quality = info["thumbnail_length"]
type_quality = desired_type != info.type
length_quality = info.length
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this is duplicated out of the info struct because everything but the last field is used as a sort criterion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right -- I guess we could just inline it below, but I'm trying to avoid too much refactoring at the same time as this?

"thumbnail_method": method,
"thumbnail_type": self.test_image.content_type,
"thumbnail_length": 256,
"filesystem_id": f"thumbnail1{self.test_image.extension.decode()}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Was filesystem_id just redundant here? (Guessing it should have been in FileInfo and was missed in some previous refactor?)

Copy link
Member Author

Choose a reason for hiding this comment

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

get_remote_media_thumbnails no longer returns it, we're just matching the return type of that function here.

@clokep clokep merged commit 7615e2b into develop Oct 6, 2023
38 checks passed
@clokep clokep deleted the clokep/thumbnail-info branch October 6, 2023 14:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants