Allowing mosaic code to attempt all valid arrangements and pick the best

[Deer-Spangle]

This was discussed over on the FixTweet repo here: FixTweet/FxTwitter#48
and I had been wanting to try my hoof at Rust, so I figured I would give it a go, and it was good fun and seemed to go okay!

So basically, this is swapping out the entire mosaic selection code, and now it will choose from almost all possible arrangements, scale them appropriately, and select the best one.

Definitions:

• "almost all possible arrangements"
  So, for 2 images, there's 2 arrangements. They can either be side by side, or atop eachother. For 3 images there are 6 arrangements, and for 4 images there are 14 arrangements. I've detailed all possible arrangements in this scrappy diagram doodled in mspaint. All arrangements are Red, Blue, Green, Purple.
  
  But looking over them, I actually realised that 4 of the 4-image arrangements are not very reader-friendly. Having 2 columns of 2 images, and having 3 columns where one column contains 2 images, are both not very clear for reading order (as a western left-to-right reading person, at least.) So I've removed those 4 options. The code is still there, and can be commented back in in the best_4_mosaic() function if you want them back.

• "scale them appropriately"
  This will now ensure that no images will be scaled down in size unless the mosaic hits the maximum size. Previously in a 2-image mosaic, the second image would be scaled up or down to match the first image. Now the smaller image will be scaled up to match the larger, unless it hits the maximum mosaic size.

• "the best one":
  At first I was selecting just based on squareness, but realised that could lead to images being wildly out of scale with eachother.
  An example is that if you have 4 images: 100x100, 300x100, 300x100, 100x100, it would rather scale the first one up 700% and put the other 3 in a row below it, resulting in a 700x800 image, rather than having 2 rows of 2 and resulting in a 400x200 image. That seemed very silly. So now it checks which mosaic has the smallest ratio between largest and smallest scaling factor, and only considers mosaics which have a similar (within 50%p) scaling factor ratio, and then selects the most square option from those.

I added a bunch of tests too, checking it all assembles fixed size images the way I would expect it to. The tests will also create a mosaic_tests directory and save the resulting images there, for manual inspection. Like the above diagrams, the images always go Red, Blue, Green, Purple.

Downsides:

• The spacing actually ends up scaled to the first image, which might not be ideal. If the first image is 100x100 and the second is 200x200, the mosaic will end up scaling the first image to 200x200, but also scaling the spacing from 10px to 20px. Not ideal, but seems fine?
• There's a lot of manually defining mosaic layouts in here. Theoretically, one could generate all possible layouts for N-images by saying that all N-image mosaics are a mosaic of 1 image and N-1 images, and that any 2 image mosaic can either be side-by-side, or top-and-bottom, but... generating all possible mosaics that way seemed like overkill when we know N is a maximum of 4. And it would not allow us to omit the layouts which do not have a clear reading order for a (western) reader.
• The tests are a bit fuzzy, not precisely checking every pixel is correct. But they would still need updating if spacing size or max size is ever changed. I'm also not sure if the "loop x and y ranges and get each pixel and check it" method is very time efficient. Maybe there is a better way.
• It generates the mosaic structs for every arrangement, then checks which is the best arrangement. Maybe that could be more time efficient by doing that in parallel, or it could be more memory efficient by checking each and discarding mosaics which are less good than a current best option. But both of those seemed to raise complexity.

The commit history is a bit of a mess, as I was learning rust as I was going, so there's a lot of mess in there. If you would like me to rebase and neaten it up, just say so. All feedback is very much appreciated too! (And many thanks to the various Rust furries who helped me out on telegram!)

Thanks for giving me a chance to try out some rust on an interesting little problem! ^_^

[Antonio32A]

Thanks for the PR and sorry for the late response, I wrote a bunch of stuff a few days ago but apparently never pressed comment.

The spacing actually ends up scaled to the first image, which might not be ideal. If the first image is 100x100 and the second is 200x200, the mosaic will end up scaling the first image to 200x200, but also scaling the spacing from 10px to 20px. Not ideal, but seems fine?

This is an original bug with the typescript reference that I also decided to keep in. It's barely visible unless the resolutions are extremely wacky. I think you shouldn't bother with it unless it's an easy fix.

There's a lot of manually defining mosaic layouts in here. Theoretically, one could generate all possible layouts for N-images by saying that all N-image mosaics are a mosaic of 1 image and N-1 images, and that any 2 image mosaic can either be side-by-side, or top-and-bottom, but... generating all possible mosaics that way seemed like overkill when we know N is a maximum of 4. And it would not allow us to omit the layouts which do not have a clear reading order for a (western) reader.

I feel like there's a bit cleaner way to generate all layouts (similar to MosaicImageDims but bit compacter and unified) but your approach is more than good enough.

The tests are a bit fuzzy, not precisely checking every pixel is correct. But they would still need updating if spacing size or max size is ever changed. I'm also not sure if the "loop x and y ranges and get each pixel and check it" method is very time efficient. Maybe there is a better way.

Tests can be ran in parallel and they don't exactly need to be fast. Your tests are fine but ideally we'd have some real tweet examples too.

It generates the mosaic structs for every arrangement, then checks which is the best arrangement. Maybe that could be more time efficient by doing that in parallel, or it could be more memory efficient by checking each and discarding mosaics which are less good than a current best option. But both of those seemed to raise complexity.

I'd personally just multithread it if it becomes a problem but since the calculation is probably fast I doubt that's gonna be necessary.

The commit history is a bit of a mess, as I was learning rust as I was going, so there's a lot of mess in there.

Yeah, you should squash most of those commits.

Code style stuff:

• Move all of the tests into /test.
• Split up mosaic functions into multiple directories (including tests, test/twos.rs): src/mosaic/twos.rs, src/mosaic/threes.rs and src/mosaic/fours.rs so we don't have one big 1.7k line file.
• Minor but add an empty line after every function declaration so it's more readable and not that squashed.

[Deer-Spangle]

No worries on reply time, I've got plenty of other things to keep me occupied!

This is an original bug with the typescript reference that I also decided to keep in. It's barely visible unless the resolutions are extremely wacky. I think you shouldn't bother with it unless it's an easy fix.

Yup, I've noticed it happening now and then with the current code. This isn't a regression, just a comment on a bug carried forward. I can't think of a particularly pleasant way to handle it.

Tests can be ran in parallel and they don't exactly need to be fast. Your tests are fine but ideally we'd have some real tweet examples too.

Good point! I should add some system level tests. I'll work on adding some of those.

Yeah, you should squash most of those commits.

Sounds good, I'll try and rebase and neaten it. Definitely those semi-colon and comma ones (from where I was editing in github.dev without IDE help)

Code style stuff:

* Move all of the tests into `/test`.

* Split up mosaic functions into multiple directories (including tests, `test/twos.rs`): `src/mosaic/twos.rs`, `src/mosaic/threes.rs` and `src/mosaic/fours.rs` so we don't have one big 1.7k line file.

* Minor but add an empty line after every function declaration so it's more readable and not that squashed.

Ah, I did try moving the tests into a separate file, but it didn't seem happy with that. Rust folks I spoke to said to just keep the tests in the same file, and browsing a couple other Rust repos, that's the same pattern I saw there. Splitting the different image counts into different files makes sense to me. I was certainly not comfy with a 1700 line file.

[Antonio32A]

Yup, I've noticed it happening now and then with the current code. This isn't a regression, just a comment on a bug carried forward. I can't think of a particularly pleasant way to handle it.

That's fine, don't worry about it since as said before it's very minor.

Ah, I did try moving the tests into a separate file, but it didn't seem happy with that. Rust folks I spoke to said to just keep the tests in the same file, and browsing a couple other Rust repos, that's the same pattern I saw there. Splitting the different image counts into different files makes sense to me. I was certainly not comfy with a 1700 line file.

Ah, I have limited Rust experience and you usually split them in other languages. In that case just split up the different image counts into different files and then have tests for them there.

[Deer-Spangle]

Apologies, life stress levels here have been insane lately.

By this comment:

Your tests are fine but ideally we'd have some real tweet examples too.

Do you mean full-up system tests using some actual tweets? Or just mosaic level tests using actual tweet images? I kind pof assumed the former, but got all sorts of tangled trying to implement that and had no real luck at it (Not sure how to build a Path<HandlePath> and stuff)

I'm gonna try and proceed assuming the latter, and try and get this neatened up asap

[Antonio32A]

Oops, sorry for the late response again - was handling a lot of stuff and forgot about it.
By real tweet examples I meant having like a few tweets on Twitter posted by an alt account which have images and then we can refetch and recompute the tweets and compare the images. So basically a full unit test for mosaic.

[Deer-Spangle]

Ahh, yeah, hmm, that's a fair amount trickier. Do you mean that it should be a full system test, where it runs main() and then queries the API and checks the returned image? Or just call handle() with a given path of tweet image IDs?

Secondary issue:
I wrote a few tests (currently uncommitted) which just fetch tweet images for a few example tweets, and then check the resulting mosaic is correct.
I used the three tweets from the original issue:

• https://twitter.com/oryxspioenkop/status/1562177315529973761
• https://twitter.com/emollick/status/1562143652180365323
• https://twitter.com/duolingostrange/status/1562118519160020992

And the latter two worked as expected:

but the first one did not do what I had hoped:

I had wanted those to stack vertically, but instead, they went side by side.

So, the mosaic script calculates 2 variables when choosing which mosaic arrangement to use:

• scale factor ratio: (What's the difference between the most scaled-up and least scaled-up images)
• unsquareness: (Basically, the aspect ratio of the resulting image)

For the tank tweet, we have this:
First image: https://pbs.twimg.com/media/Fa33_LAXoAE8JTO?format=jpg&name=large, 35186 bytes, 468px × 312px
Second image: https://pbs.twimg.com/media/Fa33_K_XkAAq-tp?format=jpg&name=large, 435629 bytes, 1,920px × 864px

And there are two mosaic options:
Stacked top/bottom: scale factor ratio= 4.1025643, unsquaredness= 1.1390625
Stacked left/right: scale factor ratio= 2.7713675, unsquaredness= 3.7546296

The algorithm as it stands is:

1. Find the mosaic with the smallest scale factor ratio
2. Ignore all mosaics with a scale factor ratio 0.5 above the lowest one
3. Choose the most square of the remaining options

But since the top/bottom stacking has a scale factor of 4.10, which is bigger than 2.77+0.5, it gets ignored.
I'm not sure the best way to solve that. Options that come to mind:

1. Increase that allowable scale factor ratio difference to at least 1.5
2. Change it to allow double the smallest scale factor ratio
3. Maybe multiply both together, and take the smallest
   • so you weight both against eachother in a way? That would give 4.1025643*1.1390625=4.67307714797 for the top/bottom, and 2.7713675*3.7546296=10.405458448 for left/right
   • I'm not sure how this would impact other mosaics
     It's a classic weighting problem, and I was joking with my partner how it would be a good candidate for machine learning.. but generating a tonne of mosaics and classifying which are good in order to train a weighting would be very tedious!

TL;DR: So yeah, two questions remain, basically:

1. What level of system test do you mean? An actual all-up system test, or an integration test of the handle() method?
2. The mosaic selection didn't work as expected for the tank tweet which originally triggered this issue. Do you have any idea of a good way to do the weighting for the final selection?

[Antonio32A]

1. I'd say we should go with an all-up system test. Heck, you could even have something run the mosaic API in the background and have a completely separate script to test it.
2. Like you said, this is a weighting problem. You also have to consider that the best layout of images is kind of subjective. Using ML for this is probably not worth it because you would need multiple people to do it for probably hundreds of samples. Honestly I'm not really sure how to properly weigh this to make everybody happy, but I don't think you really need to. Most users won't mind that much or even notice. For now I suggest just trying out all three on a bunch samples and picking the one you think feels the best to you.

[Deer-Spangle]

1. That does seem neat, but also seems like a bunch more than I was initially planning to do here, and I'm not sure how to do it. I'm not sure whether all-up system tests would be a separate issue and PR to this one. This one is already getting a bit big and long
2. Yeah, I'm quite torn on it, but finding a big selection of tweets which would be useful for it also seems quite tricky, to be honest. I'm not sure how best to do that

[Antonio32A]

1. Yup, that's perfectly fine. I might even look into this myself at a later date.
2. Honestly I think for now you should just leave as it is since your resizing algorithm is much much better than the one that's currently active. We can always adjust it later in the future if we don't like it.

[Deer-Spangle]

Whatever happened with this in the end btw? I figured it was mostly done, but not sure whether you're expecting more on my side, or if this is ready to go, or what

[Antonio32A]

I honestly forgot about this and got carried away with work and school stuff over the last half year - sorry for that.
I believe I originally requested changes with this and was waiting for you to push them, but I don't think you ever did so this PR just got buried and forgotten. I'll make sure to review it this weekend and will probably also make slight modifications to the style so it matches the rest of the project.

[Deer-Spangle]

That's very fair! Life comes first. I had some busy times too. There's no rush

I thought I had done all the changes, other than creating a suite of system-level tests, which seemed like a bigger issue than I could manage with my Rust knowledge.

[Antonio32A]

Refactored the code a bit, this should be a bit more easy to navigate. Everything else looks good on my end, so I'll just pass it onto danagered to test it a bit more.

[dangeredwolf]

Sorry I forgot to update the issue here, but this branch is available to test currently with canary.fxtwitter.com, specifically the mosaic endpoint is mosaic-canary.fxtwitter.com

[Deer-Spangle]

Oh, I missed that @Antonio32A had looked over this and refactored. Thanks for splitting those files out, I couldn't figure out how to do that ^_^;

I'll have to try out the canary endpoint a bit more and see how it goes

[Deer-Spangle]

Hmm, tried the canary endpoint in telegram with the 3 example tweets higher in the thread, and they all just gave the first image of each tweet :(

[dangeredwolf]

@Deer-Spangle Oh shoot. I just realized that's an unrelated bug caused by something else I'm testing (Telegram Instant View), thank you for pointing that out! Try it on Discord. I'll update it in a sec for Telegram.

[dangeredwolf]

... Actually, no, I thought it was broken, but it seems to be working OK for me. At least this one, anyway.

[Deer-Spangle]

Oh yeah, my bad. Mistake with web telegram, turned out my links said "canary.fxtwitter.com..." but were actually just "twitter.com" links still.

Though, the Instant View thing kind of breaks it? Because you can't see the combo image at all without clicking through to instant view, and can't view the images in there without clicking through to twitter

Not sure whether that kind of defeats the point?
I wonder whether the instant view could have the photos and the full text, but not sure whether that's useful

I've seen that with nsfw tweets, there's no instant view, which makes sense.

EDIT: Errr, apologies for the massive image previews there

[dangeredwolf]

The Instant View thing is a bit off topic for this repository, but TL;DR it'll let you view a full conversation thread without having to open up the URL or the Twitter app. It's still an experimental feature, hence it's only available on Canary.

[dangeredwolf]

What version of Telegram is that? (i.e. iOS, Android). I suspect Android cuz iOS doesn't cut off the images

[Deer-Spangle]

Ahh, Instant View is for the threading thing. I figured it was orthogonal and not for this repo, but it seems to block this stuff

I was testing on webk and on android (plus messenger). The (much too large) screenshots are from android