Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display previews and inline images for reddit-hosted images. #738

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikupls
Copy link
Contributor

@mikupls mikupls commented Mar 3, 2023

This version does not change the svg behaviour in the non-ireddit cases in an attempt to reduce breakage.

This is a rebased version of the >1 year old PR #390.

The main difference is that there is no modification to the html in the non-ireddit case to prevent breaking random things.
The outcome of this PR is still a highly improved user-experience with regard to subreddits that use ireddit content (see e.g. r/anime_irl), while introducing minor amount of content-reflow in these cases. All subs/pages that do not make use of ireddit content will continue to be reflow-free (no change).

The underlying problem has not changed since a year ago: ireddit content does not advertise (via the json API) its dimentions. Therefore we can only know its size when we actually load the image.
I checked against all the breakage I caused in the old PR, and they seem to not break from this PR (though testing frontends is always tricky. I only tested on desktop for example.)

Opening this PR to hopefully revive some discussion.

This version does not change the svg behaviour in the other cases in an attempt to reduce breakage.
@Daniel-Valentine
Copy link
Member

Daniel-Valentine commented Mar 4, 2023

Thanks for raising this and for your research.

I am wondering if we can mitigate the content reflow caused by ireddit content by having Libreddit do a GET of an image once and cache the dimensions. There are downsides to this approach, namely increased memory consumption and traffic to Reddit, but that would reduce if not eliminate reflow.

@spikecodes, @sigaloid: Thoughts?

@sigaloid
Copy link
Member

sigaloid commented Mar 4, 2023

I think that might be a good approach. There's likely to be a larger cache with more popular instances - we don't want it to be too big. Any ideas on how to mitigate runaway memory consumption?

@mikupls
Copy link
Contributor Author

mikupls commented Mar 4, 2023

Thanks for taking a look. Daniel, can you elaborate on how this approach would work? Here is what I'm guessing:

(current code)

  1. browser opens r/somecoolsub.
  2. libreddit generates the html data with links to the proxied urls (ireddit content proxy urls).
  3. html content with missing image dimensions is served to the browser.
  4. browser requests the images via proxy urls (content reflow happens once these succeed).

Proposed alternate version is to add a step 2b, which makes libreddit request the image data already in the backend, leading to a new step 3 in which the image dimensions are known ahead of time (avoiding content reflow).

I could see that working, but:

  • pages with ireddit content now likely load substantially slower, because we cannot serve html content to the user before we GET possibly a large amount of images.
  • code gets more complex because
    • we conditionally fetch some image data in the backend
    • we have to calculate image dimensions
    • we have to cache image data somehow in the backend? (is that already implemented?) Or GET ireddit images twice (once in the backend, once when the browser requests it)

Overall, I would personally take the rare reflow cases, but others might of course disagree :)


I checked again how reddit itself is doing it these days. It looks like (e.g. visiting r/anime_irl) reddit only ever serves preview images that have dimensions in the url (unless you open the post and then click the image, at which point you get the full source image in a new tab). However it also requires some hash in the URL which seems to only be served from reddit's html version (not from the json API). I did not investigate this in detail. Maybe there is some viable alternative approach in here.

While we wait for a solution, I'll maintain my patch on https://github.com/mikupls/libreddit/tree/experimental for anyone interested.

@sigaloid
Copy link
Member

sigaloid commented Mar 5, 2023

we have to cache image data somehow in the backend? (is that already implemented?) Or GET ireddit images twice (once in the backend, once when the browser requests it)

Just the dimensional data would be cached (otherwise cache would grow rapidly for popular instances). I suppose that means we get twice. That does complicate things, I see what you mean.

I tend to agree with the reflow argument in this case...

@Daniel-Valentine
Copy link
Member

Daniel-Valentine commented Mar 9, 2023

@mikupls: I did a little bit of research into this. The hope was that we could GET a small amount of data for each image, something around 20 or so bytes, to get image headers that could provide dimension info, which we could cache temporarily (we used cached extensively to create timed caches for data we get from Reddit). Unfortunately, this doesn't appear to be possible across the board; JFIF headers, for example, will tell you the pixel density of an image but not the dimensions thereof.

Another complication is that cached's TimedCache evicts stale items at fetch time, but it is not garbage-collected. That means that a TimedCache can experience unbounded growth if it is requested to store but never furnish upon request items within. In normal operation, this would be an edge case, but this is also an exploit causing a crash due to lack of memory waiting to happen.

That being said, I'm not sure if we would need to do two GETs for each image. Maybe there is a way we can harness the proxy to get the dimensions from the image and then supply that to the HTTP handler for the Reddit post route somehow? I will continue to explore.

Personally though I'm not averse to reflow if it's neither common nor major. Since @spikecodes was the one to raise the issue first, I'll let him offer an opinion.

@mikupls
Copy link
Contributor Author

mikupls commented Mar 10, 2023

Personally though I'm not averse to reflow if it's neither common nor major. Since @spikecodes was the one to raise the issue first, I'll let him offer an opinion.

I encourage everyone to just try the patch :) I've been running it for the last weeks (my fork), and found no problems so far (e.g. distorted images, like in the old PR). Granted that I used a stable internet connection (not mobile) most of the time, where reflow is a much smaller problem.
Spot-checking many popular subs makes me think that ireddit content is not that common, i.e. many subs are unaffected by this change. It seems like there is a (small?) group of subs that rely heavily on ireddit content (such as r/anime_irl) that will be impacted by this change (both positively and negatively).

The most realistic (reflow-free) alternative I can see is still somehow making use of the preview images (like reddit itself does it).

@oifj34f34f
Copy link

Any updates?

@mikupls
Copy link
Contributor Author

mikupls commented Nov 4, 2023

@oifj34f34f Thanks for showing interest in this patch. I think development in this repo is stuck/abandoned at the moment, so I don't think this will get merged anytime soon.

If you want to self-host, I maintain a version with my patches (mainly this one and some other small stuff) on https://github.com/mikupls/libreddit/tree/experimental

@azizLIGHT
Copy link

@oifj34f34f Thanks for showing interest in this patch. I think development in this repo is stuck/abandoned at the moment, so I don't think this will get merged anytime soon.

If you want to self-host, I maintain a version with my patches (mainly this one and some other small stuff) on https://github.com/mikupls/libreddit/tree/experimental

How can I use your specific fork? I have docker but I don't see a image

@mikupls
Copy link
Contributor Author

mikupls commented Nov 10, 2023

@azizLIGHT hey 👋

Not sure which platform you are on. On a Linux OS this should roughly work:

git clone https://github.com/mikupls/libreddit
cd libreddit
git checkout experimental
docker build -f Dockerfile
# wait for 2-3 minutes
# it will output some $ID
docker run -d -p 127.0.0.1:8080:8080 $ID
# go to http://127.0.0.1:8080 in your browser and it should work.
# at the bottom it should say `v0.30.1-mikupls-3`.

Let me know if you need more help. If you are looking for any specific patch to be added, I can also consider it.

@azizLIGHT
Copy link

azizLIGHT commented Nov 21, 2023

Let me know if you need more help. If you are looking for any specific patch to be added, I can also consider it.

@mikupls I've been using this tree happily after using your instructions. Thanks for that. I seem to recall in previous comments/PR you had one version that was even more inlined/previewed and this is the toned down version because of the resulting display issues. You had said that you didn't experience those issues that much. I'd like to try that version if it's possible?

I'm not sure how to ask you that so I replied here

freedit-dev pushed a commit to freedit-dev/libreddit that referenced this pull request Nov 21, 2023
@oifj34f34f
Copy link

oifj34f34f commented Dec 9, 2023

@mikupls I tried

git clone https://github.com/mikupls/libreddit
cd libreddit
git checkout experimental
cargo build --release

And then

$ ./libreddit 
Starting Libreddit...
Running Libreddit v0.30.1-mikupls-3 on 0.0.0.0:8080!

But the images still don't show up

Am I doing something wrong?

@azizLIGHT
Copy link

azizLIGHT commented Dec 9, 2023

But the images still don't show up

Am I doing something wrong?

Images in comments of a post

As far as I understand, that is a comment on a post page. I'm not sure if this version by mikupls acts on comments. You can try imagus mod (Firefox / Chrome) for images in comments. Works well for subreddits whose posts are very image heavy, like /r/photoshoprequests or /r/photoshopbattles where all top-level comments are images. Helps on other sites too

image

Kooha-2023-12-09-04-51-20.mp4

Images in the post itself

The old PR is talking about the post itself and is supposed to embed that.
These are the pics from the old PR.
After (left side) and Before (right side)

when browsing:
when browsing

when inside the post:
when inside the post

I have mikupls patch working and embedding images on the post just like the pic in the old PR's after pics:
when browsing::
image

when inside the post:
image

@oifj34f34f
Copy link

@azizLIGHT Thanks for the clarification. I mistakenly thought it was a PR to fix the comment images.. xD

@sigaloid
Copy link
Member

Hi @mikupls, I'm working on a fork right now, I merged this into it. Just to confirm, as is, this will result in cache growing more with popularity as it will cache images, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants