Split find_image_or_metadata API into two separate APIs

[jdm]

find_image_or_metadata serves two purposes today - learn about the state of the cache right now, and reserve a spot in the cache if there is not already an entry. This makes understanding how callers use it complicated, and it makes it easy to forget to add listeners for existing incomplete cache entries. We should redesign it:

• get_image should return an Option<Image> which indicates that there is either a cached, fully loaded image, or not
• track_image should accept a sender argument, which will be stored in the image cache just like the ImageCache::add_listener API. track_image will return a ImageCacheResult enum with the following states:
• Available(ImageOrMetadataAvailable) (if only metadata is available)
• LoadError
• Pending
• ReadyForRequest(PendingImageId)

The Available(ImageOrMetadataAvailable::Image) and LoadError states will result in the sender being dropped, since no more updates for that cache entry are expected. All other states will result in the sender being stored in the cache entry. This will make it impossible for a consumer to accidentally forget to add a cache listener for any state.

[jdm]

It's possible we could split this further into APIs that are allowed to load a placeholder and those that aren't, and reduce the set of possible responses that be received in the case where a placeholder image is not allowed.

[BK1603]

I'd like to work on this!
Thanks a lot @jdm for helping me find this

[BK1603]

@jdm
Just to let you know:
I am still working on this, i've read a lot of the codebase in order to know about what i need to know, one of the functions is done, doing the second one and figuring out calls, will probably need help with that

And I apologoze for taking too long 😅

Thanks once again for all the help

[jdm]

No need to apologise! Thanks for letting us know.

[BK1603]

@jdm,
I am not able to build servo on my machine. I have tried a lot of stuff but I still am stuck 😅
Here is the error code, can you help me please? 😅

error: linking with cc failed: exit code: 1

= note: collect2: fatal error: ld terminated with signal 6 [Aborted], core dumped
compilation terminated.
terminate called after throwing an instance of 'std::bad_alloc'
what(): std::bad_alloc

[jdm]

That looks like you're running out of RAM while building.

[BK1603]

@jdm,
Even at 6.5 gigs??
I have a minimal linux installation in an 8 gigs machine, it should get at least 6.5 in all cases, how much ram do i need?
Is this some bug?
What do i do now? 😅

[jdm]

We definitely exceed 6gb of used RAM at times during compilation. I don't know what to suggest :/

[BK1603]

@jdm, oh okay
Got it
I will think of something then 😅
Like i will tell you soon if i am able to do sonething, else you can just unassign this issue again 😅

I'll try something and let you know

Thanks once again 😄

[BK1603]

@jdm,
I tried doing a lot of stuff and ended up increasing the swap partition of my linux system, for which I had to format my laptop and reinstall everything, it does work but right now building on my laptop is like hours long, cause when the ram is completely filled up the system gets really slow, I don't think I will be able to build and debug as efficiently then 😅

It won't be a problem if you unassign the issue for now, I am trying out a few more things (one of them being convincing my parents for giving me the money for upgrading the RAM 😅 ), but I think they will take a lot of time, and I already have taken 15 days on this, it won't be a problem if someone else is able to fix this before me 😄

I hope I will be able to fix everything soon and make a PR, like I really want to do it 😅 but for now I guess I have to try and find a workaround, or get my RAM upgraded.

[jdm]

Ok! Sorry it's caused so much trouble for you!

[BK1603]

@jdm,
It's absolutely fine, I don't think it's any of ours fault 😄

Also it was quite fun interacting there on irc and working on such a huge project, I'd say
Thanks a lot! 😄

[germangb]

Hi @BK1603 Are you still interested in working on this issue?

[iulianR]

Hi! Is it fine if I work on this issue? Thank you!

[jdm]

@iulianR Yes please!

[iulianR]

@highfive: assign me

[highfive]

Hey @iulianR! Thanks for your interest in working on this issue. It's now assigned to you!

[iulianR]

Started working on this and I have some questions related to the sender argument for track_image.

The way I see it, currently htmlimagelement.rs and htmlvideoelement.rs use add_cache_listener_for_element when find_image_or_metadata returns Pending, which creates an ImageResponder and calls add_listener with that image listener as sender.

Should I rename add_cache_listener_for_element and have it return the ImageResponder which I would then pass to track_image, which in turn calls add_listener? Also, should add_listener still be public?

[jdm]

@iulianR Yes, that sounds like a reasonable way to structure the code. There's one other use of add_listener in window.rs right now which it's not clear if we can replace with the new APIs, so I don't think we can make it private yet.

[jdm]

@iulianR Are you still working on this?

[iulianR]

I was unable to finish it in the free days I had then and unfortunately, I do not have the required time to finish it now. It can be assigned to someone else.

[rileylyman]

Hi, I would like to take this issue. Let me know if that's okay!

[jdm]

Please do!

[julientregoat]

hey since it looks like you haven't started yet, mind if I do @rileylyman?

@highfive: assign me

EDIT: ps @jdm this may take me some time since I'm still really fresh to the codebase. if there's someone more appropriate to fill this issue, I'm happy to concede it!

[highfive]

Hey @julientregoat! Thanks for your interest in working on this issue. It's now assigned to you!

[jdm]

You are welcome to take your time with it :)

[julientregoat]

hey @jdm , wrote some code tonight, and I think I got it going. can you tell me if I'm heading in the right direction by checking my code? building correctly so that's good at least. I plan on updating to to the v2 you mentioned later on. anyway, thanks a ton, appreciate it! wanted to confirm before I go arounf fixing the others

link to code

[jdm]

Yes, that seems like the right idea!

[julientregoat]

@jdm Okay so I'm refactoring the functions that previously called find_image_or_meta atm. Currently, it looks like if any of them require access to the image, it's generally as Arc<Image>. It seems like it would be better to return an Option<Arc<Image>> from ImageCache::get_image, which unwraps it from an Arc anyway - does that work?

Also, a few of those same functions usually do something with the PendingImageId in the event that the image is still pending. Would it be ok to have the enum as ImageCacheResult::Pending(PendingImageId)?

[jdm]

@julientregoat If the consumers want an Arc, then that seems like a reasonable change. As for the question about the Pending variant, do the functions do anything besides call add_listener? I would expect that functionality to become part of track_image automatically.

[julientregoat]

@jdm

• In LayoutContext::get_or_request_image_or_meta, it uses the id to construct a PendingImage and add it to its internal pending_images vector.
• In HTMLImageElement::react_to_environment_changes_sync_steps, on Pending it does call add_cache_listener_for_element, which I recognize also calls ImageCache::add_listener inside like ImageCache::track_image, however I wasn't sure if that served a different purpose because of the extra logic within the function - seemed like custom ipc messaging & routing logic
• Same as above for HTMLVideoElement::fetch_poster_frame

[jdm]

Good point! Seems reasonable to include it for the time being, in that case.