Closed Bug 1312770 Opened 8 years ago Closed 7 years ago

Set image load priorities according to their position in viewport

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect, P1)

Product:
Core
Component:
Layout: Images, Video, and HTML Frames
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox52 --- wontfix
firefox55 --- fixed

People

(Reporter: mayhemer, Assigned: schien)

References

Details

Attachments

(1 file, 9 obsolete files)

bug1312770.patch
1.08 KB, patch
schien
: review+
Details | Diff | Splinter Review 
We want to set correct priorities to image loads according their position on the screen (in the view port) during initial page rendering.  There already is an api (nsISupportsPriority) for this, but the information is not set on the image load channels by layout.

Images that don't have size (width) set by markup should be raised priority to lower the number of reflows.

Stage 2 of this effort should be to react to scrolling, if that can even be separated to make the first implementation simpler.  Noting here to keep it in mind.
Blocks: 1312771
SC, you mentioned you know or have touched imagelib and layout in the past.  Could you take this one?
Assignee: nobody → schien
The size information can be either specified in <img> or CSS. https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp#287 should be a good place to look up size information and adjust the priority.
Here are the reflow we want to avoid:
https://dxr.mozilla.org/mozilla-central/rev/783356f1476eafd8e4d6fa5f3919cf6167e84f8d/layout/generic/nsImageFrame.cpp#581
https://dxr.mozilla.org/mozilla-central/rev/783356f1476eafd8e4d6fa5f3919cf6167e84f8d/layout/generic/nsImageFrame.cpp#693

The performance measurement can be done easily by repetitively loading our target test pages and counting the number of reflows triggered by these two places.
I noticed that imagelib doesn't notify size update to layout after image header decode completed, layout only get the size information after entire image decode completed. 

@tnikkel do you think updating the original size of the image to layout as early as possible can reduce the total number of reflow?
Flags: needinfo?(tnikkel)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #5)
> I noticed that imagelib doesn't notify size update to layout after image
> header decode completed, layout only get the size information after entire
> image decode completed. 

That shouldn't be true. We do a metadata-only decode (to get the size mainly) before a full decode of the image. The full decode and the metadata decode happen off-the-main thread, so they could both easily complete before the main thread has a chance to accept the results and send notifications. Try testing with a large image that takes a significant amount of time to decode?
Flags: needinfo?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #6)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #5)
> > I noticed that imagelib doesn't notify size update to layout after image
> > header decode completed, layout only get the size information after entire
> > image decode completed. 
> 
> That shouldn't be true. We do a metadata-only decode (to get the size
> mainly) before a full decode of the image. The full decode and the metadata
> decode happen off-the-main thread, so they could both easily complete before
> the main thread has a chance to accept the results and send notifications.
> Try testing with a large image that takes a significant amount of time to
> decode?

Thanks for the correction. I misunderstood how our metadata decoding works.
Metadata decode is fired at RasterImage::Init, which is triggered by the first available HTTP data received. Layout will be notified via nsImageFrame::Notify, which is the same callback function as full image decode.
Here is the place we know the size of the real image is important to layout
https://dxr.mozilla.org/mozilla-central/rev/79feeed4293336089590320a9f30a813fade8e3c/layout/generic/nsImageFrame.cpp#794

We can slightly turn up the priority of those image not too much, because we still want to load JS and CSS file at this very beginning stage.
Here is the place we know image is in viewport and ready to be rendered:
https://dxr.mozilla.org/mozilla-central/rev/13f49da109ea460665ad27c8497cb1489548450c/layout/generic/nsImageFrame.cpp#1764

We can raise the loading priority to high priority at this time.

Or, we can do it earlier while the visibility of the image frame is changed and request for decode:
https://dxr.mozilla.org/mozilla-central/rev/13f49da109ea460665ad27c8497cb1489548450c/layout/generic/nsImageFrame.cpp#710
Attached patch [WIP] image-priority.patch (obsolete) — — Splinter Review 
1. raise some priority for size-decode-required image if the image size is demanded by layout.
2. raise more priority for full-decode-required image if the image content is demanded by rendering.
Comment on attachment 8812098 [details] [diff] [review]
[WIP] image-priority.patch

Review of attachment 8812098 [details] [diff] [review]:
-----------------------------------------------------------------

I'll try to find time and test this patch locally.  It's great you found the right spots!

::: layout/generic/nsImageFrame.cpp
@@ +898,5 @@
> +          if (!imageBroken && mLoadUrgency < NEED_SIZE) {
> +            mLoadUrgency = NEED_SIZE;
> +            nsCOMPtr<nsISupportsPriority> p = do_QueryInterface(currentRequest);
> +            if (p) {
> +              p->AdjustPriority(1);

is this a typo?  you are actually lowering the priority a bit here.
(In reply to Honza Bambas (:mayhemer) from comment #11)
> Comment on attachment 8812098 [details] [diff] [review]
> [WIP] image-priority.patch
> 
> Review of attachment 8812098 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll try to find time and test this patch locally.  It's great you found the
> right spots!
> 
> ::: layout/generic/nsImageFrame.cpp
> @@ +898,5 @@
> > +          if (!imageBroken && mLoadUrgency < NEED_SIZE) {
> > +            mLoadUrgency = NEED_SIZE;
> > +            nsCOMPtr<nsISupportsPriority> p = do_QueryInterface(currentRequest);
> > +            if (p) {
> > +              p->AdjustPriority(1);
> 
> is this a typo?  you are actually lowering the priority a bit here.

should be -1, WIP updated.
Attached patch [WIP] image-priority.patch (obsolete) — — Splinter Review 

        Attachment #8812098 -
        Attachment is obsolete: true

    
    

    
  
sc, what is the status here?  What updated the wip patch needs?  

(This is one of the most important CDP bugs.)
Flags: needinfo?(schien)

    
    

    
  
Sorry for the late response, putting most of my time on PBackground-ify bugs.

Tried to verify if my WIP works today and found it doesn't work as expected.
1. AdjustPriority() doesn't take effect because we don't allow priority changed by other than the first imageRequestProxy. https://searchfox.org/mozilla-central/rev/848c29538ab007fb95dc6cff194f0e3d3809613d/image/imgRequest.cpp#539
2. The timing of AdjustPriority() is still to late (even if we workaround 1), so that it is hard to have impact on the loading order.
3. The code for updating priority for image size request doesn't filter the case that height/width is already provided on the <img> element.
4. Priority doesn't affect the loading sequence if image is loaded from cache.

I'll provide a patch to fix 2) and 3) this week.
Flags: needinfo?(schien)

    
    

    
  
Here is the test case I used to verify the order of image loading.
https://people-mozilla.org/~schien/image-test/img-load-order.html

It is hosted on HTTP1.1 server and cache is disabled for all the resources so we can monitor the order of pending transactions to see if our algorithm takes effect.

There are totally 10 images in this test case, each of them has different attributes.
The perfect loading order in theory will be:

8no-size.png (need both image size and content for layout and rendering)
7known-size.png (only need image content for rendering)
rnd-3m.png (background image, need image content for rendering)
1hidden.png (need image size for layout)
4not-in-view.png (need image size for layout)

(follow the order in html since we don't need both size and image content for these <img>)
2display-none.png
3hidden-size.png
5sure-not-in-view.png
6end-of-doc.png
9zero-size.png

    
    

    
  
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #15)
> 2. The timing of AdjustPriority() is still to late (even if we workaround
> 1), so that it is hard to have impact on the loading order.
This is actually caused by bug 1338096.

        Attachment #8835951 -
        Attachment is obsolete: true

        Attachment #8835952 -
        Attachment is obsolete: true

    
    

    
  
Here is the summary of latest status:

1. In patch part 1 I introduce `forceAdjustPriority` in imgIRequest in order to by pass the first proxy restriction in imgRequestProxy[1], since the first proxy is usually created by nsHtml5SpeculativeLoad. You'll not be able to adjust priority via nsISupportsPriority in layout since it'll always get the second one.

2. Priority boost for size query is moved to nsImageFrame::Init, this will be the earliest timing I can find.

3. In my experiment, most of the image-rich websites already specify the height/width of each <img>. Therefore, most of the load priority boost is made by the visibility in viewport.

    
    

    
  
From the test result of WebPageTest this patch shows performance improvement on Alexa top 100. I'll start the review process for my patches.

    
    

    
      
  

    
    

    
      
  

        Attachment #8812638 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #23)
> 3. In my experiment, most of the image-rich websites already specify the
> height/width of each <img>. Therefore, most of the load priority boost is
> made by the visibility in viewport.

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #24)
> From the test result of WebPageTest this patch shows performance improvement
> on Alexa top 100. I'll start the review process for my patches.

Where are we getting the performance increase from this? How much? What are we measuring? How long it takes to get the page load event?

Why do we want to increase the priority of all images (even those that layout doesn't depend on)? Aren't things like css and fonts more important?

Is the goal to improve page load time? Or improve the user experience by having visible things load sooner?

    
    

    
  
(In reply to Timothy Nikkel (:tnikkel) from comment #27)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #23)
> > 3. In my experiment, most of the image-rich websites already specify the
> > height/width of each <img>. Therefore, most of the load priority boost is
> > made by the visibility in viewport.
> 
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #24)
> > From the test result of WebPageTest this patch shows performance improvement
> > on Alexa top 100. I'll start the review process for my patches.
> 
> Where are we getting the performance increase from this? 

Not sure I understand the question - does "where" means on "which web sites?"  Or what?

> How much? 

Gary would know better.  Gary?

> What are
> we measuring? 

AFAIK, speed index and perceived speed index as part of Web Page Test.

> How long it takes to get the page load event?

Can't answer that.

> 
> Why do we want to increase the priority of all images (even those that
> layout doesn't depend on)? 

We shouldn't.  How do we recognize that layout doesn't depend on an element/image?

> Aren't things like css and fonts more important?

Depends, but head referenced sync scripts definitely are more important than images.  Do you see us going against that?

> 
> Is the goal to improve page load time? 

No.

> Or improve the user experience by
> having visible things load sooner?

Yes.
Flags: needinfo?(xeonchen)

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #28)
> > Why do we want to increase the priority of all images (even those that
> > layout doesn't depend on)? 
> 
> We shouldn't.  How do we recognize that layout doesn't depend on an
> element/image?

The HaveSpecifiedSize check should do that.

> > Aren't things like css and fonts more important?
> 
> Depends, but head referenced sync scripts definitely are more important than
> images.  Do you see us going against that?

There is only a finite amount of bandwidth to go around, if we increase images we implicitly decrease priority of everything else.

In nsImageFrame::Init we should only increase the priority of images that do not have a specified size.

nsImageFrame::BuildDisplayList is relatively late, we know that the image is visible sooner than that. If you put the priority adjustment in nsImageFrame::MaybeDecodeForPredictedSize then that will be called at important times: after reflow, when the image is determined to be visible, as well as BuildDisplayList (if we haven't already).

Also, Init can be called more than once because we can re-create the frame. So the priority would keep getting adjusted higher each time. We'd want to avoid that. We could either keep track either on the content node (nsImageLoadingContent) or on the imgRequestProxy.

So there seems to be two separate things we want to do:
1) increase priority of images without a specified size because layout depends on them
2) increase priority of visible images so the page appears to be visibly complete faster

We should separate out these changes and measure their effect on the testsuite you are using. So we know how much they each improve the results.

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #28)
> (In reply to Timothy Nikkel (:tnikkel) from comment #27)
> > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> > #23)
> > > 3. In my experiment, most of the image-rich websites already specify the
> > > height/width of each <img>. Therefore, most of the load priority boost is
> > > made by the visibility in viewport.
> > 
> > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> > #24)
> > > From the test result of WebPageTest this patch shows performance improvement
> > > on Alexa top 100. I'll start the review process for my patches.
> > 
> > Where are we getting the performance increase from this? 
> 
> Not sure I understand the question - does "where" means on "which web
> sites?"  Or what?
> 
> > How much? 
> 
> Gary would know better.  Gary?
> 

We're running top-100 sites ([1]) on WebPageTest private instance to check performance results.
Every site for 30 times of firstView (no cache) & repeateView (with cache).

It suggested that this patch improves the Speed Index metric, but after investigation, we found it needs to re-test because the tests were not run at the same day, and the contents may vary.

I'm afraid it may take more than a week (1-min * 30-time * 2-view * 100-site * 2-browser) for the result.

> > What are
> > we measuring? 
> 
> AFAIK, speed index and perceived speed index as part of Web Page Test.
> 

Only speed index. [2] lists the values we have.


[1] https://docs.google.com/a/mozilla.com/spreadsheets/d/1xErRcxgK2eXdJMZAmSwD6wSjz12xoK8rPpIIHdJK87g/edit?usp=sharing
[2] https://sites.google.com/a/webpagetest.org/docs/advanced-features/raw-test-results
Flags: needinfo?(xeonchen)

    
    

    
  
To move forward with this I think we need:

1) split the change into two parts: one part that increases the priority of images who's sizes are needed for layout, and one part that increases priority of images that are visible to the user
2) we need to do some tests to show that this improves our speed and doesn't hurt us anywhere else.

    
    

    
  
(In reply to Timothy Nikkel (:tnikkel) from comment #31)
> 2) we need to do some tests to show that this improves our speed and doesn't
> hurt us anywhere else.

Testing the two parts separately to see what effect each part has.

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8837980 [details]
Bug 1312770 - Part 2, slightly increase loading priority for images that need size information for layout.

https://reviewboard.mozilla.org/r/112990/#review124292

::: layout/generic/nsImageFrame.cpp:287
(Diff revision 3)
>    // Give image loads associated with an image frame a small priority boost!
>    nsCOMPtr<imgIRequest> currentRequest;
>    imageLoader->GetRequest(nsIImageLoadingContent::CURRENT_REQUEST,
>                            getter_AddRefs(currentRequest));
>    if (currentRequest) {
> -    currentRequest->ForceAdjustPriority(-1);
> +    int32_t delta = -1;

We should just increase the priority when !HaveSpecifiedSize. Increasing priority of all images could work against us.

    
    

    
  
Comment on attachment 8849371 [details]
Bug 1312770 - Part 3, increase loading priority for images that are visible in viewport.

https://reviewboard.mozilla.org/r/122158/#review124294

::: layout/generic/nsImageFrame.cpp:1793
(Diff revision 1)
>          if (!(status & imgIRequest::STATUS_DECODE_COMPLETE)) {
>            MaybeDecodeForPredictedSize();
>          }
> +
> +        // Increase loading priority if the image is ready to be displayed.
> +        if (!(status & imgIRequest::STATUS_LOAD_COMPLETE) &&

This seems like a big priority boost (10). Why not just increase by 1 like in Init?

    
    

    
  
Comment on attachment 8837980 [details]
Bug 1312770 - Part 2, slightly increase loading priority for images that need size information for layout.

https://reviewboard.mozilla.org/r/112990/#review124292

> We should just increase the priority when !HaveSpecifiedSize. Increasing priority of all images could work against us.

This is the logic we have in tree, see https://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/layout/generic/nsImageFrame.cpp#288. Apperently it was introduced by bug 278531 long time ago. Do you think we should remove it and just (-1) for `!HaveSpecifiedSize`?

    
    

    
  
Comment on attachment 8849371 [details]
Bug 1312770 - Part 3, increase loading priority for images that are visible in viewport.

https://reviewboard.mozilla.org/r/122158/#review124294

> This seems like a big priority boost (10). Why not just increase by 1 like in Init?

All the image are initially loaded in low priority (+10) [1]. I think we should raise the priority of visible images back to normal priority (0) so they have same priority as XHR.

JS/CSS are marked as blocking items and fonts are initially loaded in high priority (-10) [2]. They'll still be loaded before images.

[1] https://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/image/imgLoader.cpp#857
[2] https://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/layout/style/FontFaceSet.cpp#643

    
    

    
  
Comment on attachment 8837980 [details]
Bug 1312770 - Part 2, slightly increase loading priority for images that need size information for layout.

https://reviewboard.mozilla.org/r/112990/#review124348

::: layout/generic/nsImageFrame.cpp:287
(Diff revision 3)
>    // Give image loads associated with an image frame a small priority boost!
>    nsCOMPtr<imgIRequest> currentRequest;
>    imageLoader->GetRequest(nsIImageLoadingContent::CURRENT_REQUEST,
>                            getter_AddRefs(currentRequest));
>    if (currentRequest) {
> -    currentRequest->ForceAdjustPriority(-1);
> +    int32_t delta = -1;

The bug that your part 1 patches fixes means that we don't adjust the priority of any image, right?

So currently we don't adjust priority of any image.

If we want to increase the priority of all images in nsImageFrame::Init we should have some data to back that up.

    
    

    
  
Comment on attachment 8849371 [details]
Bug 1312770 - Part 3, increase loading priority for images that are visible in viewport.

https://reviewboard.mozilla.org/r/122158/#review124294

> All the image are initially loaded in low priority (+10) [1]. I think we should raise the priority of visible images back to normal priority (0) so they have same priority as XHR.
> 
> JS/CSS are marked as blocking items and fonts are initially loaded in high priority (-10) [2]. They'll still be loaded before images.
> 
> [1] https://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/image/imgLoader.cpp#857
> [2] https://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/layout/style/FontFaceSet.cpp#643

That's a valid argument. But why should visible images be increased by 10 when images whose size information is needed to layout the page (!HaveSpecifiedSize) only get a boost of 1? Those images would seem to be just as important.

    
    

    
  
Comment on attachment 8837980 [details]
Bug 1312770 - Part 2, slightly increase loading priority for images that need size information for layout.

https://reviewboard.mozilla.org/r/112990/#review124352

::: layout/generic/nsImageFrame.cpp:290
(Diff revision 3)
>                            getter_AddRefs(currentRequest));
>    if (currentRequest) {
> -    currentRequest->ForceAdjustPriority(-1);
> +    int32_t delta = -1;
> +
> +    // Increase load priority further if intrinsic size might be important for layout.
> +    if (!HaveSpecifiedSize(StylePosition())) {

We should also check if the currentRequest has size available already. If the size is available then we don't need to increase the priority. There is already a function to check that: SizeIsAvailable.

    
    

    
  
Comment on attachment 8837980 [details]
Bug 1312770 - Part 2, slightly increase loading priority for images that need size information for layout.

https://reviewboard.mozilla.org/r/112990/#review124348

> The bug that your part 1 patches fixes means that we don't adjust the priority of any image, right?
> 
> So currently we don't adjust priority of any image.
> 
> If we want to increase the priority of all images in nsImageFrame::Init we should have some data to back that up.

The `AdjustPriority(-1)` in current code doesn't take effect because the nsImageFrame is not holding the first imgRequestProxy. So, yes, the runtime behavior of current Firefox will not increase the priority.

However I think this is an unawared side effect after speculative image loading in HTML parser was introduced in bug 482919. This patch changes the owner of first proxy. That's the reason why I try reactivate this priority change.

I'm fine with removing the `AdjustPriority(-1)` since it's a dead code for a while. We can focus on the new algorithm based on current behavior.

    
    

    
  
Comment on attachment 8849371 [details]
Bug 1312770 - Part 3, increase loading priority for images that are visible in viewport.

https://reviewboard.mozilla.org/r/122158/#review124294

> That's a valid argument. But why should visible images be increased by 10 when images whose size information is needed to layout the page (!HaveSpecifiedSize) only get a boost of 1? Those images would seem to be just as important.

My theory is that complete rendering an image on viewport will stable a large chunk of pixels on screen. This can improve more on our metrics because SpeedIndex prefers pixels that rendered as early as possible. Obtaining size information earlier might not contribute that much pixels, so I only add small boost to create a local order among images without affecting the global order.

    
    

    
  
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #36)
> @xeonchen can you run WebPageTest against these three versions?

Sure, it's running :)
Flags: needinfo?(xeonchen)

    
    

    
  
(In reply to Timothy Nikkel (:tnikkel) from comment #29)
> nsImageFrame::BuildDisplayList is relatively late, we know that the image is
> visible sooner than that. If you put the priority adjustment in
> nsImageFrame::MaybeDecodeForPredictedSize then that will be called at
> important times: after reflow, when the image is determined to be visible,
> as well as BuildDisplayList (if we haven't already).
> 
> Also, Init can be called more than once because we can re-create the frame.
> So the priority would keep getting adjusted higher each time. We'd want to
> avoid that. We could either keep track either on the content node
> (nsImageLoadingContent) or on the imgRequestProxy.
Based on these two information, I'm thinking about providing boostForSizeQuery/boostForDisplay in imgIRequest and let imgRequest ensure priority is only increased once for each scenario.

    
    

    
  
Few notes:

- FYI, I want to change fonts to load as head-blocking (will file a bug)
- View port visible images should be raised by -10
- Missing size images should be increased by say -1 or -5
=> then, an image w/o size info visible in the view port will be raised by -15
=> load priorities will then be in the following order: visible-no-size, visible-size, invisible-no-size, invisible

Maybe the patches already do that :)  (Haven't yet taken a look)

    
    

    
  
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #44)
> The `AdjustPriority(-1)` in current code doesn't take effect because the
> nsImageFrame is not holding the first imgRequestProxy. So, yes, the runtime
> behavior of current Firefox will not increase the priority.
> 
> However I think this is an unawared side effect after speculative image
> loading in HTML parser was introduced in bug 482919. This patch changes the
> owner of first proxy. That's the reason why I try reactivate this priority
> change.

Okay, so we haven't been doing it for 7+ years

> I'm fine with removing the `AdjustPriority(-1)` since it's a dead code for a
> while. We can focus on the new algorithm based on current behavior.

I'm not advocating either way, but we need some kind of evidence to make these decisions otherwise we are just flying blindly and could be making things worse.

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #45)
> My theory is that complete rendering an image on viewport will stable a
> large chunk of pixels on screen. This can improve more on our metrics
> because SpeedIndex prefers pixels that rendered as early as possible.
> Obtaining size information earlier might not contribute that much pixels, so
> I only add small boost to create a local order among images without
> affecting the global order.

We need some kind of evidence to back this up. It could be argued that the page contents moving around (because we had to re-layout the page because we got needed size information from an image) is a worse user experience then an image not being loaded as quickly: the user can't read or interact with the page in any way until it stops moving around.

    
    

    
  


  
This is another approach that follows comment #29. I keep the priority boost for display in BuildDisplayList to keep the same result I described in comment #16.

I tried to move that part in to MaybeDecodeForPredictedSize with following code:


>nsCOMPtr<nsIImageLoadingContent> imageLoader = do_QueryInterface(mContent);
>if (imageLoader && IsVisibleForPainting()) {
>  nsCOMPtr<imgIRequest> currentRequest;
>  imageLoader->GetRequest(nsIImageLoadingContent::CURRENT_REQUEST,
>                          getter_AddRefs(currentRequest));
>  if (currentRequest) {
>    currentRequest->BoostPriority(imgIRequest::CATEGORY_DISPLAY);
>  }
>}

However I found the "4not-in-view.png" and "5sure-not-in-view.png" will still request for display boost. Those two 30x30 images are placed (-80,-80) so it is not in the viewport. Don't know if I misunderstand the meaning of `IsVisibleForPainting()`.

        Attachment #8849984 -
        Flags: feedback?(tnikkel)

    
    

    
  
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #50)
> Created attachment 8849984 [details] [diff] [review]
> store-request-in-imgrequest.patch
> 
> This is another approach that follows comment #29. I keep the priority boost
> for display in BuildDisplayList to keep the same result I described in
> comment #16.
> 
> I tried to move that part in to MaybeDecodeForPredictedSize with following
> code:
> 
> 
> >nsCOMPtr<nsIImageLoadingContent> imageLoader = do_QueryInterface(mContent);
> >if (imageLoader && IsVisibleForPainting()) {
> >  nsCOMPtr<imgIRequest> currentRequest;
> >  imageLoader->GetRequest(nsIImageLoadingContent::CURRENT_REQUEST,
> >                          getter_AddRefs(currentRequest));
> >  if (currentRequest) {
> >    currentRequest->BoostPriority(imgIRequest::CATEGORY_DISPLAY);
> >  }
> >}
> 
> However I found the "4not-in-view.png" and "5sure-not-in-view.png" will
> still request for display boost. Those two 30x30 images are placed (-80,-80)
> so it is not in the viewport. Don't know if I misunderstand the meaning of
> `IsVisibleForPainting()`.

I don't quite understand, IsVisibleForPainting can only be called with a nsDisplayListBuilder argument, so it has to be called during painting, ie in BuildDisplayList.

If you're putting your code into MaybeDecodeForPredictedSize then I think I know the problem. MaybeDecodeForPredictedSize tries to decode images that are "close to visible" in that they could be scrolled into view without much scrolling. This is the menaing of APPROXIMATELY_VISIBLE here

https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/layout/generic/nsImageFrame.cpp#722

    
    

    
  
(In reply to Timothy Nikkel (:tnikkel) from comment #51)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> > However I found the "4not-in-view.png" and "5sure-not-in-view.png" will
> > still request for display boost. Those two 30x30 images are placed (-80,-80)
> > so it is not in the viewport. Don't know if I misunderstand the meaning of
> > `IsVisibleForPainting()`.
> 
> I don't quite understand, IsVisibleForPainting can only be called with a
> nsDisplayListBuilder argument, so it has to be called during painting, ie in
> BuildDisplayList.
There is one `IsVisibleForPainting()` that takes no parameter, only two usages found in current code base.
https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/layout/generic/nsFrame.cpp#7229
> 
> If you're putting your code into MaybeDecodeForPredictedSize then I think I
> know the problem. MaybeDecodeForPredictedSize tries to decode images that
> are "close to visible" in that they could be scrolled into view without much
> scrolling. This is the menaing of APPROXIMATELY_VISIBLE here
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/layout/generic/nsImageFrame.cpp#722
Yeah I'm also suspecting that APPROXIMATELY_VISIBLE check is too loose to be used in my case.
To be noticed that images with "visibility: hidden" will also pass the APPROXIMATELY_VISIBLE check. By using `IsVisibleForPainting` I mentioned above, I can filter out those images.

    
    

    
  
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #52)
> There is one `IsVisibleForPainting()` that takes no parameter, only two
> usages found in current code base.
> https://dxr.mozilla.org/mozilla-central/rev/
> e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/layout/generic/nsFrame.cpp#7229

Oh, that's a weird one. Looks like the only check it does besides visibility: hidden is when printing is limited to a selection.

> Yeah I'm also suspecting that APPROXIMATELY_VISIBLE check is too loose to be
> used in my case.
> To be noticed that images with "visibility: hidden" will also pass the
> APPROXIMATELY_VISIBLE check. By using `IsVisibleForPainting` I mentioned
> above, I can filter out those images.

Hmm, I think you might have found a bug. Looks like nsIFrame::UpdateVisibilitySynchronously (which we call to update visibility of one frame when we know that one frame changed and want to know it's visibility right away) doesn't consider the style visibility of the frame, but PresShell::MarkFramesInSubtreeApproximatelyVisible (which we call less frequently to update visibility of all frames) does consider the style visibility of frames.

Anyways, we can probably make a version of nsIFrame::UpdateVisibilitySynchronously that doesn't consider images that are just scrolled out of view to be visible that you could use for this. The difference would be that it would return true/false for visibile/not visible instead of calling EnsureFrameInApproximatelyVisibleList/RemoveFrameFromApproximatelyVisibleList, and then change the IsRectNearlyVisible call to only intersect with the scrollport without calling ExpandRectToNearlyVisible.

    
    

    
  
(In reply to Timothy Nikkel (:tnikkel) from comment #53)
> Hmm, I think you might have found a bug. Looks like
> nsIFrame::UpdateVisibilitySynchronously (which we call to update visibility
> of one frame when we know that one frame changed and want to know it's
> visibility right away) doesn't consider the style visibility of the frame,
> but PresShell::MarkFramesInSubtreeApproximatelyVisible (which we call less
> frequently to update visibility of all frames) does consider the style
> visibility of frames.

-> bug 1350463

    
    

    
  
(In reply to Timothy Nikkel (:tnikkel) from comment #53)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> Anyways, we can probably make a version of
> nsIFrame::UpdateVisibilitySynchronously that doesn't consider images that
> are just scrolled out of view to be visible that you could use for this. The
> difference would be that it would return true/false for visibile/not visible
> instead of calling
> EnsureFrameInApproximatelyVisibleList/
> RemoveFrameFromApproximatelyVisibleList, and then change the
> IsRectNearlyVisible call to only intersect with the scrollport without
> calling ExpandRectToNearlyVisible.
It'll be great if we can differentiate the loading priority between *really* visible elements and approximately visible elements. Is there a bug for this modification?

We can have three categories of image boost to fine tune the image loading order.
 - boost for size query
 - boost for approximately visible items
 - boost for really visible items

    
    

    
  
Just finish the result analysis of the previous WPT run.

1. Most of websites shows no major difference on SpeedIndex. (note: plus means better performance)
    - Boost for size query:
        51/60 sites are within ±5% range,
        6 sites within -10%~-5%,
        1 site within +5%~+10%,
        3 sites within +10%~+20%.
    - Boost for display:
        46/60 sites are within ±5% range,
        1 site within -10%~-20%,
        5 sites within -10%~-5%,
        8 sites within +5%~+10%.

My reading from the WPT test result is that loading order among images within the same origin doesn't have much influence on the SpeedIndex, the main difference is usually cause by switching order with cross-origin JS. The cross-origin JS is usually for Ad, social network gadget, and SEO. It will consume both network bandwidth and CPU cycle for downloading and executing them. If images happens to load before that, SpeedIndex will have better results. However, our current prioritization algorithm doesn't affect the global load order. Thus we cannot observe an obvious improvement on single prioritization algorithm.

2. Loading major images first can improve the SpeedIndex in some case.
   Take https://techcrunch.com/2016/05/10/please-dont-learn-to-code/ as example. The codecode.jpg occupies a significant amount of viewport. Loading that image first will improve SpeedIndex, in this case around 10%

Loading sequence with boost for display: https://people-mozilla.org/~schien/image-test/major-pic-first.png
Loading sequence of current m-c: https://people-mozilla.org/~schien/image-test/major-pic-later.png

The codecode.jpg loads 0.09s earlier can still make differences on the performance.

    
    

    
  
These results almost seem like a wash. Do we still want to proceed with this?

    
    

    
  
The second bullet point in comment #56 still shows positive effect under certain circumstances. To make the performance improvement more observable, we need to do more flow control and prioritization among all the connections. I think this is the path we are taking for the entire CDP project. I'll suggest to continue the review process.

    
    

    
  
Too late for firefox 52, mass-wontfix.

          status-firefox52:
          affected → wontfix

    
  
Priority: -- → P1

    
    

    
  
Timothy, ping on review here.  We would like to take the patch as is ASAP to start getting a field results (for potential regressions mainly).  If there are any major changes that could be done in followups, let's file them and fix separately.

Thanks.
Flags: needinfo?(tnikkel)

    
    

    
  
We need something like store-request-in-imgrequest.patch before we can make any of these changes otherwise more than one copy of the image in the same document, or in other tabs can increase the priority by huge amounts. So lets start with the patch that does that, then implement the rest on top of that.

We should land each proposed change separately (so that they go onto different nightlies) so that if we get talos or telemetry changes (or other improvements or regressions reported) we can attribute them to a specific change. The three separate changes are:
1) giving all images a small boost (like we used to a very long time ago, but is currently broken)
2) giving visible images a big boost
3) giving images whose size is needed a boost.
Flags: needinfo?(tnikkel)

    
    

    
  
Comment on attachment 8849984 [details] [diff] [review]
store-request-in-imgrequest.patch

Yes, something like this makes sense. Let's base the other patches on this.

        Attachment #8849984 -
        Flags: ui-review+

        Attachment #8849984 -
        Flags: feedback?(tnikkel)

        Attachment #8849984 -
        Flags: feedback+

        Attachment #8849984 -
        Flags: ui-review+

    
    

    
  
Comment on attachment 8837979 [details]
Bug 1312770 - Part 1, allow any imgRequestProxy to adjust load priority.

I would like to see the patches rebased on top of something like store-request-in-imgrequest.patch

        Attachment #8837979 -
        Flags: review?(tnikkel)

        Attachment #8837980 -
        Flags: review?(tnikkel)

        Attachment #8849371 -
        Flags: review?(tnikkel)

    
    

    
  
(In reply to Timothy Nikkel (:tnikkel) from comment #63)
> Comment on attachment 8837979 [details]
> Bug 1312770 - Part 1, allow any imgRequestProxy to adjust load priority.
> 
> I would like to see the patches rebased on top of something like
> store-request-in-imgrequest.patch

Thanks @tnikkel, I'll split my patch and file separated bugs for each of them.

    
    

    
  
Just a note we would like to land the patches as soon as possible, since this is an important part of the CDP project.  I definitely don't want to land each patch on a different release number.  Landing them (in an order to be determined) in few days intervals is preferred.  Thanks.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug1312770.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Conflict will need to be resolved if landed after bug 1357327.

        Attachment #8837979 -
        Attachment is obsolete: true

        Attachment #8837980 -
        Attachment is obsolete: true

        Attachment #8849371 -
        Attachment is obsolete: true

        Attachment #8849984 -
        Attachment is obsolete: true

        Attachment #8859144 -
        Flags: review?(tnikkel)
No longer blocks: 1312771

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #65)
> Just a note we would like to land the patches as soon as possible, since
> this is an important part of the CDP project.  I definitely don't want to
> land each patch on a different release number.  Landing them (in an order to
> be determined) in few days intervals is preferred.  Thanks.

I definitely did not want to land them in different release _number_, thats way too far apart. Just landed one day apart so they are on different nightlies.

        Attachment #8859144 -
        Flags: review?(tnikkel) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug1312770.patch
        — 
          — Splinter Review
      

    


  
rebase to latest m-c, carry r+

        Attachment #8859144 -
        Attachment is obsolete: true

        Attachment #8861260 -
        Flags: review+

    
    

    
  
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abb953c4e429
Slightly increase loading priority for images that need size information for layout. r=tnikkel
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/abb953c4e429
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox55:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

    
  
Product: Core → Core Graveyard

    
  
Product: Core Graveyard → Core

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: