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

Use image metadata to lay out images before the complete image data is available #7047

Closed
jdm opened this issue Aug 6, 2015 · 7 comments
Closed
Labels
A-content/images Interacting with images from web content A-layout/inline C-assigned There is someone working on resolving the issue

Comments

@jdm
Copy link
Member

jdm commented Aug 6, 2015

The immeta library can provide metadata about an image given a buffer. We should use this to retrieve the dimensions of an image as soon as possible while the image is downloading.

@jdm jdm added A-content/images Interacting with images from web content A-layout/inline labels Aug 6, 2015
@jdm
Copy link
Member Author

jdm commented Aug 6, 2015

This is equivalent to Gecko's SIZE_AVAILABLE notification for imagelib.

@metajack
Copy link
Contributor

metajack commented Aug 6, 2015

Note that Seth has told me that this reading of image metadata (and dealing with embedded JPEG screenshots) is a prime target for Rust in Geckoness.

@jdm jdm added the C-assigned There is someone working on resolving the issue label Dec 7, 2015
@jdm
Copy link
Member Author

jdm commented Dec 7, 2015

@jmr0 said they're interested in looking into this.

@jdm
Copy link
Member Author

jdm commented Dec 7, 2015

Relevant code to look at for servo involves ImageListener::notify in components/net/image_cache_task.rs, and ImageResponse in components/net_traits/image_cache_task.rs. We'll want a way to notify consumers (presumably via another entry in ImageResponse) that the metadata was successfully retrieved. The logic in HTMLImageElement::update_image will need to become more complex, and HTMLImageElement will need a separate field for the metadata that we care about. Finally, the ImageFragmentInfo code in components/layout/fragment.rs is where we'll end up using the new metadata.

@jmr0
Copy link
Contributor

jmr0 commented Dec 11, 2015

@jdm I take it the goal is for this metadata to be immediately accessible to the layout task? I was looking at this today through the debugger and noticed the image fragment's metadata is, as an example, accessed in layout/inline.rs through a call to assign_replaced_inline_size. A few steps after this there's an attempt to add a DisplayItem to a display list if the image data is available within the fragment.

I'm curious as to whether modifying the image fragment's API (image_block_size and image_inline_size) to work with the pre-fetched metadata will right away yield some benefit, or if we should be looking at some other piece to avoid reflows?

Also, I noticed that ImageResponse is used in other places (like dom/webglrenderingcontext.rs and dom/canvasrenderingcontext2d.rs) and wasn't sure if this work would be applicable to those. Should a pre-fetched metadata response be ignored in such places (for now, at least)?

@jdm
Copy link
Member Author

jdm commented Dec 11, 2015

@jmr0 Yes, the code in image_inline_size and image_block_size should use the metadata if it's available, instead of deferring to the sizes present in the complete image data as in the current code.

You are also correct, the users in the webgl and canvas code are specifically concerned with the pixel data, so the metadata won't be enough for them.

@jmr0
Copy link
Contributor

jmr0 commented Dec 16, 2015

I think I've made some progress. At this point, HTMLImageElement and ImageFragment receive notifications that the metadata is available, and in the fragment's case it'll respond to image_inline_size and image_block_size using it. The biggest changes were to the image cache API, as I added support for just requesting an image or for separate metadata+image messages if metadata is available first -- this made it easier to handle any users that didn't care for the metadata alone.

I wasn't sure what this block of code was about -- seemed to be for testing or sending requests in sync mode. I'm currently handling it by only requesting a full image and not its metadata if we land here.

I'm also wondering if this is the best way to handle this (whether to perform the image metadata scan in another task as that would involve some synchronization around PendingLoad's byte buffer or cloning).

I'm not entirely sure how HTMLImageElement can use the metadata directly yet -- I'll take a look at the spec soon and probably send some more questions your way. Thanks again :)

The whole commit is here: jmr0@83be712

bors-servo pushed a commit that referenced this issue Jan 28, 2016
Use image metadata to lay out images

Fixes #7047.

cc: @jdm

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9208)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 30, 2016
Use image metadata to lay out images

Fixes #7047.

cc: @jdm

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9208)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 30, 2016
Use image metadata to lay out images

Fixes #7047.

cc: @jdm

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9208)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 30, 2016
Use image metadata to lay out images

Fixes #7047.

cc: @jdm

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9208)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/images Interacting with images from web content A-layout/inline C-assigned There is someone working on resolving the issue
Projects
None yet
Development

No branches or pull requests

3 participants