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

No preview for meduza.io articles #2853

Closed
rkfg opened this issue Feb 6, 2018 · 7 comments
Closed

No preview for meduza.io articles #2853

rkfg opened this issue Feb 6, 2018 · 7 comments
Labels
good first issue Good for newcomers S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@rkfg
Copy link
Contributor

rkfg commented Feb 6, 2018

Description

There's no preview image for any article from meduza.io news site.

Steps to reproduce

Server drops a line in the log:

2018-02-06 15:31:22,234 - synapse.rest.media.v1.preview_url_resource - 238 - WARNING - GET-12191- Couldn't get dims for https://meduza.io/imgly/share/1517844117/slides/vokrug-shum-rekordnyy-snegopad-v-moskve

The image itself is quite fine, it's a PNG of size 1200x630.

Version information

  • Homeserver: own homeserver
  • Version: Synapse/0.26.0
  • Install method: package manager
  • Platform: set up on Debian 9.3 amd64.
@neilisfragile neilisfragile added z-p2 (Deprecated Label) z-minor (Deprecated Label) Z-Help-Wanted We know exactly how to fix this issue, and would be grateful for any contribution labels Feb 7, 2018
@neilisfragile
Copy link
Contributor

$ curl https://meduza.io/imgly/share/1517844117/slides/vokrug-shum-rekordnyy-snegopad-v-moskve > image
$ identify image
image JPEG 1200x630 1200x630+0+0 8-bit sRGB 126632B 0.000u 0:00.000

Not clear on first pass why dims cannot be extracted

@rkfg
Copy link
Contributor Author

rkfg commented Apr 7, 2021

So for this article above I have a preview image now. But this one's broken (as well as other more recent articles): https://meduza.io/feature/2021/04/07/proekt-rasskazal-o-vtoroy-zhene-ramzana-kadyrova-i-novoy-lichnosti-ego-pervoy-suprugi

The issue is that their server sends content-type as image/jpeg; charset=utf-8 which is weird but not against the spec I guess. It can be fixed in this file like this:

                 if b"Content-Type" in headers:
                     media_type = headers[b"Content-Type"][0].decode("ascii")
+                    scpos = media_type.find(';')
+                    if scpos > 0:
+                        media_type = media_type[:scpos]
                 else:
                     media_type = "application/octet-stream"

@clokep
Copy link
Member

clokep commented Apr 7, 2021

@rkfg Any chance you'd be willing to put up a PR? tests/rest/media/v1/test_url_preview.py has some tests which could be updated too.

@clokep clokep added good first issue Good for newcomers S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. and removed Z-Help-Wanted We know exactly how to fix this issue, and would be grateful for any contribution z-minor (Deprecated Label) z-p2 (Deprecated Label) labels Apr 7, 2021
@rkfg
Copy link
Contributor Author

rkfg commented Apr 7, 2021

I'll see what I can do. There's another issue that prevents the preview images for pikabu.ru from showing, around this line:

         jpeg_thumbnail = ThumbnailRequirement(width, height, method, "image/jpeg")
         png_thumbnail = ThumbnailRequirement(width, height, method, "image/png")
         requirements.setdefault("image/jpeg", []).append(jpeg_thumbnail)
+        requirements.setdefault("image/jpg", []).append(jpeg_thumbnail)
         requirements.setdefault("image/webp", []).append(jpeg_thumbnail)
         requirements.setdefault("image/gif", []).append(png_thumbnail)
         requirements.setdefault("image/png", []).append(png_thumbnail)

Their content-type for the previews is image/jpg which seems not to be standard conforming but the Internet is a mess and we have to deal with it. Should I also add this change to the PR?

@clokep
Copy link
Member

clokep commented Apr 7, 2021

It seems related enough to more robust handling of the Content-Type header, so sure.

@rkfg
Copy link
Contributor Author

rkfg commented Apr 11, 2021

Opened a PR #9788 that should fix this issue.

@clokep
Copy link
Member

clokep commented Apr 14, 2021

Fixed via #9788.

@clokep clokep closed this as completed Apr 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

No branches or pull requests

3 participants