Dial in criteria for speculatively decoding images [chromium/src : main]
Philip Rogers (Gerrit)
Philip Rogers added 2 comments![]( "Open in Gerrit")
Is this fixing the issue where we decode small placeholder images with large layout sizes? If so, can you add a test? I think you can just modify this one from one of your previous patches (the data url here is a 10x10 png):
```
TEST_F(WebFrameWidgetImplSimTest, SpeculativeImageDecodeBeforeLayout) {
// Check that a speculative decode can start as soon as an img element gets a
// src based on prior layout information, without waiting for a subsequent
// layout to happen.
base::test::ScopedFeatureList feature_list(
features::kSpeculativeImageDecodes);
SimRequest request("https://example.com/test.html", "text/html");
LoadURL("https://example.com/test.html");
request.Complete(R"HTML(
<!DOCTYPE html>
<html><body><img width=340 height=380/></body></html>
)HTML");
Compositor().BeginFrame();
HTMLImageElement* image =
To<HTMLImageElement>(GetDocument().QuerySelector(AtomicString("img")));
LayoutImage* layout_image = To<LayoutImage>(image->GetLayoutObject());
EXPECT_EQ(layout_image->CachedResourcePriority().visibility,
ResourcePriority::kVisible);
// Decode size should be based on layout size; note that this does not
// actually match the intrinsic size of the data URL below.
EXPECT_EQ(layout_image->CachedSpeculativeDecodeSize(), gfx::Size(340, 380));
image->setAttribute(
html_names::kSrcAttr,
AtomicString("data:image/"
"png;base64,iVBORw0KGgoAAAANSUhEUgAAAAoAAAAKCAYAAACNMs+"
"9AAAAAXNSR0IArs4c6QAAABhJREFUKFNjbGj48J+BCMA4qhBfKFE/"
"eACQKR1hvTllHQAAAABJRU5ErkJggg=="));
// The fetch is initiated synchronously from a microtask after src is set. For
// a data URL the load will also finish synchronously, and the speculative
// decode should have been triggered, based on pre-computed visibility.
EXPECT_CALL(*MockMainFrameWidget(), RequestDecode(_, _, true)).Times(1);
GetDocument().GetAgent().PerformMicrotaskCheckpoint();
}
```
if (!intrinsic_size.IsZero() &&should this be `if (intrinsic_size.IsZero() || ...)`?
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
if (!intrinsic_size.IsZero() &&should this be `if (intrinsic_size.IsZero() || ...)`?
The idea here is that if an image doesn't have an available intrinsic size then we should go ahead and do the speculative decode since the content size is above the threshold and that's the best signal we have.
Related details
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 2 comments![]( "Open in Gerrit")
// speculative decode. However, image instrinsic size is the strongestPlease fix this WARNING reported by Spellchecker: "instrinsic" is a possible misspelling of "intrinsic".
To bypass Spellchecker, ...
"instrinsic" is a possible misspelling of "intrinsic".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
if (!intrinsic_size.IsZero() &&Stefan Zagershould this be `if (intrinsic_size.IsZero() || ...)`?
The idea here is that if an image doesn't have an available intrinsic size then we should go ahead and do the speculative decode since the content size is above the threshold and that's the best signal we have.
The intrinsic size is the size in the image file itself. Don't we need that to do any decoding?
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
if (!intrinsic_size.IsZero() &&Stefan Zagershould this be `if (intrinsic_size.IsZero() || ...)`?
Philip RogersThe idea here is that if an image doesn't have an available intrinsic size then we should go ahead and do the speculative decode since the content size is above the threshold and that's the best signal we have.
The intrinsic size is the size in the image file itself. Don't we need that to do any decoding?
I do not know the answer to that, but looking at some of the image generators, it's not clear to me that the intrinsic size is always available at load time.
Related details
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 2 comments![]( "Open in Gerrit")
// Prior to image load, we use content size to qualify as a candidate forWDYT of moving this logic next to the layout size logic in ImageResource::IsAboveSpeculativeDecodeSizeThreshold?
if (!intrinsic_size.IsZero() &&Stefan Zagershould this be `if (intrinsic_size.IsZero() || ...)`?
Philip RogersThe idea here is that if an image doesn't have an available intrinsic size then we should go ahead and do the speculative decode since the content size is above the threshold and that's the best signal we have.
Stefan ZagerThe intrinsic size is the size in the image file itself. Don't we need that to do any decoding?
Philip RogersI do not know the answer to that, but looking at some of the image generators, it's not clear to me that the intrinsic size is always available at load time.
The intrinsic size is in the first few bytes of an image file. I am pretty sure that information is a hard requirement for decoding. I think we can actually CHECK here that the size isn't zero, because none of the image formats used on the web allow for 0x0 images.
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 4 comments![]( "Open in Gerrit")
I modified an existing test to cover this case.
// Prior to image load, we use content size to qualify as a candidate forWDYT of moving this logic next to the layout size logic in ImageResource::IsAboveSpeculativeDecodeSizeThreshold?
Done
// speculative decode. However, image instrinsic size is the strongestPlease fix this WARNING reported by Spellchecker: "instrinsic" is a possible misspelling of "intrinsic".
To bypass Spellchecker, ...
"instrinsic" is a possible misspelling of "intrinsic".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
Stefan Zagershould this be `if (intrinsic_size.IsZero() || ...)`?
Philip RogersThe idea here is that if an image doesn't have an available intrinsic size then we should go ahead and do the speculative decode since the content size is above the threshold and that's the best signal we have.
Stefan ZagerThe intrinsic size is the size in the image file itself. Don't we need that to do any decoding?
Philip RogersI do not know the answer to that, but looking at some of the image generators, it's not clear to me that the intrinsic size is always available at load time.
The intrinsic size is in the first few bytes of an image file. I am pretty sure that information is a hard requirement for decoding. I think we can actually CHECK here that the size isn't zero, because none of the image formats used on the web allow for 0x0 images.
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
1 comment
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |