/thumbnail should return more obvious error when dynamic_thumbnails is disabled

[MadLittleMods]

Description:

Currently /thumbnail will return a 404 M_NOT_FOUND when dynamic_thumbnails is disabled in homeserver.yaml (it is disabled by default). The response code is pretty confusing because the media does exist, it just can't be dynamically thumbnailed. We should instead return a 400 M_UNKNOWN with a friendly error explaining why (already in spec).

Maybe this also depends on no thumbnail_sizes being defined in homeserver.yaml as well.

Actual

✅ Works: http://localhost:8008/_matrix/media/r0/download/hs1/tefQeZhmVxoiBfuFQUKRzJxc

❌ Doesn't load, 404: http://localhost:8008/_matrix/media/r0/thumbnail/hs1/tefQeZhmVxoiBfuFQUKRzJxc?width=400&height=195&method=scale

{
  "errcode": "M_NOT_FOUND",
  "error": "Not found [b'hs1', b'tefQeZhmVxoiBfuFQUKRzJxc']"
}

This really confused me because obviously the media does exist. The Synapse logs show:

2022-06-13 19:15:45,125 - synapse.rest.media.v1.thumbnail_resource - 382 - INFO - GET-319 - Failed to find any generated thumbnails

Expected

When dynamic_thumbnails is turned off, http://localhost:8008/_matrix/media/r0/thumbnail/hs1/tefQeZhmVxoiBfuFQUKRzJxc?width=400&height=195&method=scale should return 400 M_UNKNOWN

400: The request does not make sense to the server, or the server cannot thumbnail the content. For example, the client requested non-integer dimensions or asked for negatively-sized images.

-- https://spec.matrix.org/v1.1/client-server-api/#get_matrixmediav3thumbnailservernamemediaid

{
  "errcode": "M_UNKNOWN",
  "error": "Cannot find existing thumbnail for the requested content and cannot generate one because `dynamic_thumbnails` is disabled (see `homeserver.yaml`)"
}

[richvdh]

Confused. If dynamic_thumbnails is off, the thumbnails should be generated when the media is uploaded. Why does the thumbnail not exist if the media does?

[MadLittleMods]

Why does the thumbnail not exist if the media does?

Maybe it's an additional separate bug.

I am seeing this behavior with this homeserver.yaml config. There is no thumbnail_sizes defined but I assume the DEFAULT_THUMBNAIL_SIZES should be kicking in?

Whenever media is uploaded, there are no errors in the logs (docker logs -f --tail 10 matrix_public_archive_test_hs1_1):

2022-06-10 17:08:41,464 - synapse.rest.media.v1.media_repository - 210 - INFO - POST-53869 - Stored local media in file '/data/media_store/local_content/oW/ho/GoMjhWIZRVfeGLasLZnQ'
2022-06-10 17:08:41,484 - synapse.rest.media.v1.upload_resource - 104 - INFO - POST-53869 - Uploaded content with URI 'mxc://hs1/oWhoGoMjhWIZRVfeGLasLZnQ'
2022-06-10 17:08:41,486 - synapse.access.http.8008 - 450 - INFO - POST-53869 - ::ffff:172.22.0.1 - 8008 - {@archiver:hs1} Processed request: 0.022sec/0.001sec (0.001sec, 0.000sec) (0.011sec/0.009sec/1) 52B 200 "POST /_matrix/media/v3/upload HTTP/1.1" "node-fetch/1.0 (+https://github.com/bitinn/node-fetch)" [0 dbevts]

This is from running the end-to-end tests at test/e2e-tests.js#L206-L233. Any additional logging I should try turning on?

As a note, if you want to run those tests (npm test), the latest homeserver.yaml is "fixed" by adding dynamic_thumbnails: true so comment that out to see the true behavior although I still think it's not adding the thumbnails on upload. It only works now because were generating the thumbnail dynamically on request to /thumbnail.

[q234rty]

Can confirm that currently matrix.org doesn't generate thumbnails in many cases, while servers running synapse 1.60.0 seem to work correctly. Should we move this to a new issue?

[reivilibre]

I agree that this seems to be a bug, but I'm not sure I agree entirely with the proposed solution — the wording in the spec makes it sound like the homeserver should return a larger thumbnail in one of the handful of specced sizes.

e.g. requesting 400×195 should give me a 640×480 thumbnail.

I guess this leaves the case where the client requests a thumbnail that is larger than the original, or is larger than the largest specced thumbnail size — maybe 400 would make sense here.

[MadLittleMods]

the wording in the spec makes it sound like the homeserver should return a larger thumbnail in one of the handful of specced sizes.

Agreed 👍 and I think Synapse already does this in all of the _select_thumbnail logic.

But in this case, Synapse isn't generating any thumbnails on upload and we should have a better error message on why that may be. Created #13038 with a proposal. 404 isn't even a specced response anyway but it probably should at least be for a mediaId that does not exist -> matrix-org/matrix-spec#1122

We can also separately fix up the Synapse bugs which are causing it not to generate the thumbnails -> #13039