1724808 - nsImageFrame::MaybeDecodeForPredictedSize() might predict the wrong decoded image size, with fission

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

The nsImageFrame code quoted below might not be quite right in a fission world, because GetTransformToAncestorScaleExcludingAnimated doesn't actually get our "scale to the screen", if we're an image inside of a cross-origin iframe.

Here's the code in question (note the GetTransformToAncestorScaleExcludingAnimated invocation in particular):

void nsImageFrame::MaybeDecodeForPredictedSize() {
[...]
  // OK, we're ready to decode. Compute the scale to the screen...
  mozilla::PresShell* presShell = PresContext()->PresShell();
  LayoutDeviceToScreenScale2D resolutionToScreen(
      presShell->GetCumulativeResolution() *
      nsLayoutUtils::GetTransformToAncestorScaleExcludingAnimated(this));

[...]
  // ...and use them to compute our predicted size in screen pixels.
[...]
  // Determine the optimal image size to use.
[...]
  // Request a decode.
  mImage->RequestDecodeForSize(predictedImageSize, flags);

tnikkel, do you know how much of an issue this is? The discussion/motivating-comments in bug 1151359 seem relevant here.

If we really want "screen coordinates", then we'll probably need to add some additional logic / communication here. But I'm not sure we do... Seth says in bug 1151359 comment 1, "Ideally we'd use layer coordinates", and if that's the case, then maybe we're actually fine? I suspect the parent-process-transform-agnostic sizing that we'll be getting here might be philosophically analogous to "layer coordinates"...

I suspect the real question is what size we expect to eventually paint the image at. In a scenario where we're in a transformed iframe, I'm not sure whether that painted size will match the size that we're predicting here or not. (If it will, then that's great and there's nothing to do, I suspect.)

Anyway: tnikkel, hoping you can weigh in here & close this or suggest/take action as-needed.

Comment 1

[Timothy Nikkel (:tnikkel)]

Thanks for filing. Yes I think we will need a fix here, we have the EffectsInfo mechanism already set up to pass this info, we just need to pass the info with webrender and use it.

Unfortunately as I was working on this bug I discovered a larger issue that encompasses this, bug 1724901. I'll fix this first and then we can figure out how to fix bug 1724901.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1724808. Take into account ancestor documents in other processes' resolution when pre-decoding images. r?hiro

Depends on D122212

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1e7bb4e3fb2
Take into account ancestor documents in other processes' resolution when pre-decoding images. r=hiro

Comment 4

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/b1e7bb4e3fb2

Comment 5

[Chris Peterson [:cpeterson]]

Setting status-firefox92=wontfix because we don't need to uplift this fix to Beta 92 for our Fission experiment.